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

[RISCV] Add GNU note property and CFI support #587

Merged
merged 3 commits into from
Dec 27, 2024

Conversation

jerryzj
Copy link
Contributor

@jerryzj jerryzj commented Dec 17, 2024

@jerryzj jerryzj marked this pull request as draft December 17, 2024 07:38
@sevaa
Copy link
Contributor

sevaa commented Dec 17, 2024

Looks like the RISCV people took a page from the ARM people. :) No objections - once the autotest passes.

EDIT: now that I look at that - no new binaries. I take it, one of the existing binaries in the corpus had RISCV notes, and GNU readelf doesn't dump them properly, and neither did pyelftools, but now pyelftools does and there is a discrepancy in readelf output? If that's the case, deal with it how we normally deal with readelf deficiencies: file a bug against binutils (https://sourceware.org/bugzilla/), and exclude the offending file from the relevant readelf test (with a comment mentioning bug no). Like it was done in #563.

@jerryzj jerryzj force-pushed the dev/jerryzj/riscv-cfi branch from f46e97e to 8164ccb Compare December 20, 2024 02:51
@jerryzj
Copy link
Contributor Author

jerryzj commented Dec 20, 2024

Looks like the RISCV people took a page from the ARM people. :) No objections - once the autotest passes.

EDIT: now that I look at that - no new binaries. I take it, one of the existing binaries in the corpus had RISCV notes, and GNU readelf doesn't dump them properly, and neither did pyelftools, but now pyelftools does and there is a discrepancy in readelf output? If that's the case, deal with it how we normally deal with readelf deficiencies: file a bug against binutils (sourceware.org/bugzilla), and exclude the offending file from the relevant readelf test (with a comment mentioning bug no). Like it was done in #563.

@sevaa yeap, since CFI is still a new feature for RISC-V, the binutils support is still not upstreamed by us. But my local testing shows the newly added binary test can PASS

  • readelf
riscv64-unknown-linux-gnu-readelf -n  ./test/testfiles_for_readelf/riscv-cfi-zicfilp-zicfiss.elf

Displaying notes found in: .note.gnu.property
  Owner                Data size 	Description
  GNU                  0x00000010	NT_GNU_PROPERTY_TYPE_0
      Properties: RISC-V AND feature: ZICFILP, ZICFISS
  • pyelftools
python3 ./scripts/readelf.py -n  ./test/testfiles_for_readelf/riscv-cfi-zicfilp-zicfiss.elf

Displaying notes found in: .note.gnu.property
  Owner                Data size        Description
  GNU                  0x00000010	NT_GNU_PROPERTY_TYPE_0 (program properties)
      Properties: RISC-V AND feature: ZICFILP, ZICFISS

Maybe we should keep this as a draft until the binutils are supported.

@jerryzj jerryzj force-pushed the dev/jerryzj/riscv-cfi branch from 8164ccb to 6be2cd4 Compare December 20, 2024 02:59
@sevaa
Copy link
Contributor

sevaa commented Dec 20, 2024

If we keep this as a draft until the next release of binutils (and there is no guarantee the fix will make it into their next release), the main branch of pyelftools will get more commits and this PR would need a rebase and maybe even a manual merge. If you ask me, we should exclude the binary/test combination with a comment, merge now, and remove the test exclusion when GNU readelf catches up. @eliben what do you think?

@eliben
Copy link
Owner

eliben commented Dec 20, 2024

@sevaa yep that sounds reasonable to me

@jerryzj jerryzj force-pushed the dev/jerryzj/riscv-cfi branch from 6be2cd4 to 62cfc3f Compare December 23, 2024 08:22
@jerryzj jerryzj marked this pull request as ready for review December 23, 2024 08:24
@jerryzj
Copy link
Contributor Author

jerryzj commented Dec 23, 2024

@sevaa @eliben okay, I've dropped the test and will have another PR for it.

@eliben eliben merged commit 12034a5 into eliben:main Dec 27, 2024
4 checks passed
@jerryzj jerryzj deleted the dev/jerryzj/riscv-cfi branch December 27, 2024 14:15
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