[lld] [llvm] Reapply "[RISCV] Support RISCV Atomics ABI attributes (#84597)" (PR #90266)

Paul Kirth via llvm-commits llvm-commits at lists.llvm.org
Mon Apr 29 17:39:48 PDT 2024


ilovepi wrote:

> > Seems like @MaskRay reported the bfd bug quite a while ago. [sourceware.org/bugzilla/show_bug.cgi?id=29823](https://sourceware.org/bugzilla/show_bug.cgi?id=29823)
> 
> Hopefully @Nelson1225 can take a look.
> 
> As we add more build attributes, `llvm-readelf -A riscv64.o` starts to resemble a typical AArch32 relocatable file with over 10 attributes. This might not be ideal for everyone, and I haven't found another architecture doing this.
> 
> The GNU ld error suggests the atomic attribute should be opt-in. This allows groups who find these attributes useful to enable them explicitly.
> 

I'm not sure I follow, are you refering to the current ToT GNU ld behavior or the segfault reported? Second, the psABI isn't ambiguous about the requirement here https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#tag_riscv_atomic_abi-14-uleb128version, and the merge policy is clearly spelled out
```
Merge policy
 The linker should report errors if object files with incompatible atomics ABIs are merged; the compatibility rules for atomic ABIs can be found in the compatibility column in the following table.
```
The wording there doesn't sound optional to me.

> On the LLVM side, we can have a `cl::opt<bool>` option to control whether such attributes should be emitted. 

I don't think this is very practical. CL opts are probably not enough, and tend to cause issues, particularly when they don't get forwarded to the linker correctly in LTO builds. 

>We can replace this with a driver option after we gain more experience and agree with GCC. It is possible that we will eventually go the AArch32 way and a typical relocatable file will contain 10+ attributes, but that's probably not the immediate focus.

I'm not aware of any reason we need to maintain strict compatibility w/ GCC at all points in time, when we're all supposed to be implementing the same spec. Note: I'm not saying we shouldn't maintain compatibility at all, but I don't see why Clang can't be leading the way here. Also, whether we emit attributes or not, doesn't seem to be the kind of thing you should control w/ a driver option if its mandated by the ABI you're targeting.

Lastly, I'd like to emphasize, enabling the correct Atomics ABI has been blocked on this feature for a long time. This is required for Android and other systems that do not want to maintain **any** compatibility with the old/broken A6C ABI, since A6C and A7 are completely incompatible.

https://github.com/llvm/llvm-project/pull/90266


More information about the llvm-commits mailing list