Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the problem that sve functions cannot be debugged by gdb #127

Closed
wants to merge 1 commit into from

Conversation

chenxuqiang
Copy link
Contributor

fix issues #125

@pablodelara
Copy link
Contributor

Thanks @chenxuqiang. Anyone from ARM that can review this?

@docularxu
Copy link

docularxu commented Nov 27, 2023

This change makes sense to me.
Without explicit specify a '.text' section in .S files, it will leave this decision to gcc (or GNU as)'s default behavior. Actually, default behavior cannot make sure the code always be put into section '.text'. Such as this file ( what @chenxuqiang modified)

sm3_mb/aarch64/sm3_mb_sve.S , everything was put into '.rodata.cst16' section, because '.rodata.cst16' is declared in sm3_mb_common.S, which is #includeed by 'sm3_mb_sve.S', right before the start of its code section.

In conclusion, adding a '.text' section is a good catch.

For safety reasons, probably we should explicitly add '.text' in all .S assembly files.

Eg. here we can list all such files:
find . -depth -type f -path '*aarch64*' -name '*S' -exec sh -c 'grep -q "\.text" "$1" || echo "$1"' sh {} \;

@docularxu
Copy link

so, @chenxuqiang would you mind to modify the commit's subject to make it more accurate?
"Fix the problem that sve functions cannot be debugged by gdb" is misleading in my humble opinion. The issue can happen in all .S files if missing '.text' declaration.

@pablodelara
Copy link
Contributor

Hi @chenxuqiang. Could you update this PR to address @docularxu comments? Adding .text in all .S files sounds like a good idea.

@pablodelara
Copy link
Contributor

This is now merged, but @chenxuqiang, could you look into the other files and open another PR addressing @docularxu comments?

@pablodelara
Copy link
Contributor

Closing this PR, as this code is merged. Feel free to open another one with more fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants