search

intel / isa-l_crypto

Other
267 stars 80 forks source link

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

Closed chenxuqiang closed 5 months ago

chenxuqiang commented 10 months ago

fix issues #125

pablodelara commented 10 months ago

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

docularxu commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

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

pablodelara commented 5 months ago

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

pablodelara commented 5 months ago

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