Introduce a new tag, Tag_RISCV_x3_reg_usage to record how gp/x3 begin …
#387
Conversation
|
@luismarques applied, thanks :) |
There was a problem hiding this comment.
I've left some minor suggestions for phrasing, and a question regarding merging.
This is the solution we were hoping for in Fuchsia, so I'm glad to see this PR. Thanks for taking the time to do this @kito-cheng.
kito-cheng
changed the title
Tag_RISCV_gp_reg_usage to record how gp begin …Tag_RISCV_x3_reg_usage to record how gp/x3 begin …
|
Changes:
|
|
Thank you for making GP more useful, especially when many folks don't consider re-purposing GP as an "ABI break" on their platforms. I still hold the opinion from #371 (comment) that a tag in the attribute section may not be very useful, more so if we emit it by default for At the moment, I don't have a strong opinion on whether a software shadow stack based on GP should emit a tag by default. If a user chooses to do so and enables --relax-gp (which is the current default in GNU ld), they will encounter an error. However, for ld.lld, which doesn't enable global pointer relaxation by default, this would be more like unnecessary work. (There are many combinations of linker options that can produce broken output, and it is simply not feasible to diagnose every invalid case.) |
|
Why would |
|
To ask a slightly higher level question - are we sure we want to go down the route of picking out every potential ABI option and specifying it in attributes (which seems to be the route this and the atomics PR seems to be going down). I can believe it's better this way, but it would be reassuring to know if people spent time considering an alternative that e.g. just specified the platform ABI targeted by a given ELF (potentially with an ABI version). |
That's kind of a broader issue, though: coupling together features into a set requires getting consensus on what those sets should be and there's just no mechanism for doing that in RISC-V land. It's essentially the same problem that's lead to an explosion of extensions, just more visible because it's ABI and we're all used to ABI proliferation being a mess. So I think we just need to suck it up and live with the mess, it's already defacto there from the extensions. We can just punt to the distros to group features into sets and then go deprecate everything else in a few years when it's sufficiently broken. |
|
This issue could break into two separate parts IMO: bare-metal and UNIX-like platforms (Linux, *BSD, Android...) For bare-metal: The answer is yes; we are going to identify potential ABI options and add attributes to let users configure what they want; that's is the scope of EABI; this part will drive by IAR folks, and I believe this is not the issue we concern here, so I would not spend too much word here :) For UNIX-like platforms: I would like to minimalize this IF possible; however, as you mentioned, we already have atomic and this PR:
Do we have more ABI options for UNIX-like platforms? It's hard to guarantee to say no more, but this will be carefully considered when introducing new ABI options. But could we prevent those EABI options from being applied on UNIX-like platforms? It's hard to stop people from doing so; from the psABI aspect, we can add NOTE to mention this is EABI, and not recommended applied on UNIX-like platforms.
That would be useful; we could create a separated issue to track that |
…used. This tag is designed to prevent the linker from merging objects that use `gp` for different purposes. Additionally, it can also serve to help users identify the usage within the executable.
kito-cheng
force-pushed
the
tag_riscv_gp-usage
branch
from
0db2b34
to
47674b0
Compare
|
Changes: -Rebase and squash patches |
|
binutils PoC: kito-cheng/riscv-binutils-gdb@d13849c |
This is exactly my concern. In another thread (I don't recall which) I mentioned that compilers cannot determine whether inline assembly uses x3 and in what way. I am sorry that I haven't replied earlier. I think I am ok if psABI adds If folks loving gp wants to an opt-in option to emit |
We can't really expect compilers to be able to tell if inline assembly violates any ABI constraint. It's inline assembly anything goes.
|
Yeah, I think user could write any ABI violation code as they want, so I don't really concern about that - user should handle those inline asm very carefully since most compiler didn't really know what inline asm write - we all rely on the info what user give. And my thought on that tag other than the binutils patch, the binutils patch is just basic tag parsing and merge rule, which not cover all thing we should do:
|
This is actually my argument against Tag_RISCV_x3_reg_usage. Let's visit the current defined 4 values. 2 (shadow stack) can be determined by a driver option, while the other 3 are really indistinguishable from the compiler.
OK, if IMHO, an option that does nothing, but just mark something, is rarely a good design. It just restricts choices. If the compiler emits an "These days we could probably change the default: we could probably say that if an object file does not have a .note.GNU-stack section, then it does not require an executable stack." GNU ld didn't do this, and we are still getting churn in binutils for Let me conclude with an end note from https://www.openwall.com/lists/musl/2023/05/03/1 : how musl rejected glibc-style symbol twidding idea for C23 binary integer binary compatibility. """There are not going to be different versions of scanf/strto* because Our GP case does is not the same as that scenario, but we can a lot of insights from the reasoning above. For global pointer relaxation users who may use x3 for other purposes, they get crash as they desire. |
You mean folks who are using the ABI as specified? We all know you dislike the ABI, but you should characterize your position a bit more forthrightly. |
I am forthright that I don't think global pointer relaxation provides sufficient valuable. I do worry that Clang produced relocatable object files will be annotated as |
|
@MaskRay I feel there's two things mixed in your comment, one concern for Let me describe the ELF attribute first. I think we have discussed this in different PRs/issues, and I know you are not really convinced that we need that. There are many different containers in ELF format which can carry information like e_flags, properties, ELF attributes, and many different .note.* sections. And we pick ELF attributes at beginning due to following reason:
However I know it’s come with some drawback like:
So we may consider introducing ELF properties to make dynamic linker easier to read, however I think we could keep using ELF attributes, and define some guidelines to clarify how to decide what information/tag should add to attribute or property. The guideline in my mind is:
Back to The first use case I expect is (soft) shadow stack, compiler will emit Without that tag, users must be careful to make sure For inline asm and hand craft assembly files…I tend to ignore those cases since I believe no matter what mechanism is used, users could play tricks to violate or ignore that. |
My two concerns are:
I do agree that if we ever need an ABI marker, build attributes is the best choice. Actually, no other alternative is ever close to be competitive. I am more concerned with the second point. I re-read the comments and think my
OK, a bit of busy work in the linker to me, but I think this is acceptable.
I hope that in the absence of build attributes, the linker doesn't need to set Tag_RISCV_x3_reg_usage.
0 is compatible with global pointer relaxation, then what does the 0 value do? In the default mode (whether -mrelax or -mno-relax, but there is no -m[no-]gp-relax), is there a Tag_RISCV_x3_reg_usage attribute?
This is small overhead, but I think it's fine. |
The original discussion, riscv-non-isa/riscv-elf-psabi-doc#387 This patch is just the first edition that I implemented after discussing it with Kito Cheng in another thread. So it will definitly have later editions in the future. I start by writing down the details that I remember. Maybe there are some riscv-psabi that haven't been upadted or mentioned yet. * Defined values of Tag_RISCV_x3_reg_usage. For compiler/assembler, 0: default, don't set anything, so allow x3 for gp relaxations. 1: relaxation, allow x3 for gp relaxations. 4: reserved, don't allow x3 for gp relaxations. others: unknown, report warning. For linker, 1: relaxation, if doing any gp relaxations, then update/add elf x3_reg_usage attribute to output files. 4: reserved, if we don't do any gp relaxations, then update/add elf x3_reg_usage to output files. If input files have unknown Tag_RISCV_x3_reg_usage values, then regard them as default in linker, which means we allow to do gp relaxations. Otherwise, report error when input files have different Tag_RISCV_x3_reg_usage values. * The new linker will always generate Tag_RISCV_x3_reg_usage for the output files * The priority of elf x3_reg_usage attribute and ld option --[no-]gp-relax. Same as elf arch attribute, elf x3_reg_usage attribute > ld option --[no-]gp-relax. * ... Co-authored-by: Kito Cheng <kito.cheng@sifive.com> bfd/ * cpu-riscv.c (riscv_elf_is_unknown_x3_reg_usage): New function. Check if the value of Tag_RISCV_x3_reg_usage is defined or not. * cpu-riscv.h (enum riscv_x3_reg_usage): Defined to record the usage of x3. * elfnn-riscv.c (riscv_elf_link_hash_table): New int x3_reg_usage, set to X3_RELAX if doing any gp relaxation. Otherwise set it to X3_RESERVED. (riscv_merge_attributes): Handle Tag_RISCV_x3_reg_usage. Regared the undefined value as 0, which is the default value. Report error if two objects have different Tag_RISCV_x3_reg_usage values, except default 0. Disable gp relaxations if Tag_RISCV_x3_reg_usage value is X3_RESERVED. (_bfd_riscv_relax_lui): Set x3_reg_usage of riscv_elf_link_hash_table to X3_RELAX if doing any gp relaxation. (_bfd_riscv_relax_pc): Likewise. (bfd_elfNN_riscv_update_x3_reg_usage): New function. Called by after_allocation, after all the relaxations (ldelf_map_segments) finishing, to update the value of output Tag_RISCV_x3_reg_usage. * elfxx-riscv.h: Defined extern bfd_elf[32|64]_riscv_update_x3_reg_usage. binutils/ * readelf.c (riscv_attr_tag_t): Added T(x3_reg_usage). (display_riscv_attribute): Print correct Tag_RISCV_x3_reg_usage information. gas/ * config/tc-riscv.c (riscv_convert_symbolic_attribute): Added T(x3_reg_usage). (s_riscv_attribute): Report warning if value of Tag_RISCV_x3_reg_usage is unknown. * testsuite/gas/riscv/attribute-x3-reg-usage-*: New testcases. include/ * elf/riscv.h (Tag_RISCV_x3_reg_usage): Defined to 16. ld/ * emultempl/riscvelf.em: Called bfd_elf[32|64]_riscv_update_x3_reg_usage in after_allocation, after ldelf_map_segments. * testsuite/ld-riscv-elf/attr-merge-arch-*: Updated since we always generate Tag_RISCV_x3_reg_usage now for the output in ld. * testsuite/ld-riscv-elf/attr-merge-priv-spec-*: Likewise. * testsuite/ld-riscv-elf/attr-merge-strict-align-*: Likewise. * testsuite/ld-riscv-elf/attr-merge-user-ext-01.d: Likewise. * testsuite/ld-riscv-elf/attr-merge-x3-reg-usage-*: New testcases. * testsuite/ld-riscv-elf/ld-riscv-elf.exp: Updated.
…used.
This tag is designed to prevent the linker from merging objects that use
gpfor different purposes. Additionally, it can also serve to help users identify the usage within the executable.