-
Notifications
You must be signed in to change notification settings - Fork 187
riscv: fix relocation constants numbering #507
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
Conversation
m4b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are breaking changes, and I need to understand why several constants were deleted
| pub const R_RISCV_GNU_VTINHERIT: u32 = 41; | ||
| /// GNU C++ vtable member usage | ||
| pub const R_RISCV_GNU_VTENTRY: u32 = 42; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why were these deleted and also constant changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#relocations
The documentation shows that this conforms to the latest RISC-V ELF psABI specification.
for now:
-
Change number 41 from
GNU_VTINHERITtoGOT32_PCREL -
Change number 42 to reserved
discuss: riscv-non-isa/riscv-elf-psabi-doc#323
This has been discussed in the community.
They appeared to be copied from other ports while there is no assembly syntax
for them. Just remove them like we remove R_RISCV_GPREL_[IS].
Removing R_RISCV_RVC_LUI looks good, like we removed
R_RISCV_GNU_VTINHERIT,R_RISCV_GNU_VTENTRY,R_RISCV_GPREL_I,R_RISCV_GPREL_Sthat were not produced by assemblers.
It seems that since numbers 41-42 were never used before and were initially set as reserved, and now that number 41 has a new number, I think it's necessary to modify them.
| /// Absolute address (CI-Type) | ||
| pub const R_RISCV_RVC_LUI: u32 = 46; | ||
| /// GP-relative reference (I-Type) | ||
| pub const R_RISCV_GPREL_I: u32 = 47; | ||
| /// GP-relative reference (S-Type) | ||
| pub const R_RISCV_GPREL_S: u32 = 48; | ||
| /// TP-relative TLS LE load (I-Type) | ||
| pub const R_RISCV_TPREL_I: u32 = 49; | ||
| /// TP-relative TLS LE store (S-Type) | ||
| pub const R_RISCV_TPREL_S: u32 = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete why were these all deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are the same as above. 46-50 should be reserved here.
same here:
- R_RISCV_RVC_LUI: Remove R_RISCV_RVC_LUI riscv-non-isa/riscv-elf-psabi-doc#400
- R_RISCV_GPREL_{I/S}: Revert "Add GP-relative relocations" riscv-non-isa/riscv-elf-psabi-doc#427
- R_RISCV_TPREL_{I/S}:
- https://patchwork.ozlabs.org/project/glibc/patch/mvmzfrrrj83.fsf@suse.de/
-
Update the list of RISC-V relocations from the ELF psABI as of June 2024.
It removes binutils-internal only relocations that were never part of
actual object files. The GNU_VTINHERIT and GNU_VTENTRY relocations were
never used because the corresponding GCC option -fvtable-gc was never
supported on RISC-V.
|
If you think this is reasonable, I'd like to amend the PR again, because I'm reviewing the code again and found that some annotations seem to need to be changed as well. |
|
Of course feel free to push or force push as much as you as want, it’s your PR :) so just fyi removing the pub constants is technically a breaking change and it would be pushed back a bit as I like to roll up breaking changes. Alternatively can leave them in and it won’t be and will be in a minor release |
Here's a suggestion: I'll dedicate a separate PR to the destructive changes and address it when you feel the time is right. In this PR, I'll be making changes to the non-destructive parts. What do you think?😎 |
|
Sounds great ! |
references: 1. https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc 2. https://riscv-non-isa.github.io/riscv-elf-psabi-doc/ version 1.1, November 26, 2025 Signed-off-by: Jvle <keke.oerv@isrc.iscas.ac.cn>
|
I have removed the parts that caused the breaking changes. you can see #510. |
This PR is a breaking change to m4b#507. Signed-off-by: Jvle <keke.oerv@isrc.iscas.ac.cn>
m4b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome thank you!
References:
RISC-V ELF psABI version 1.1 (November 26, 2025).
Given the current psABI specification, the #PR165 is no longer applicable.