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

[Fangrui Song via llvm-commits]

MaskRay 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

The trend of liberally including attributes in .riscv.attributes raises concerns.
While specific attributes can be helpful, overabundance similar to aarch32's style could lead to clutter and maintenance challenges.
Removing now-unnecessary attributes might become difficult due to some users favoring stronger compatibility.

I'll likely be more concerned if a future linker option is proposed to suppress incompatibility errors.

> ```
> 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.

I don't think we should read too much from the use of "should" here. This requirement level is probably between "should" and "may".
It should certainly be fine for a linker to ignore `.riscv.attributes` completely.

RFC2119
```
3. SHOULD   This word, or the adjective "RECOMMENDED", mean that there
   may exist valid reasons in particular circumstances to ignore a
   particular item, but the full implications must be understood and
   carefully weighed before choosing a different course.
```

> > 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. 

Using `cl::opt<bool>` looks good to me.
The LTO status is not a practical issue. Many codegen options are not recorded in LLVM IR, and it's possibly inpractical to record every one (a lot don't even have clear merge policy).

> >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.

A `cl::opt` looks good to me. By using an internal option `cl::opt<bool>`, we don't commit to the stability of the option.
It could be named, though we probably won't without a good reason. Users need to be ready to change.


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