RISC-V: prepend --no-relax-gp to linker arguments

[TechnoPorg]

As suggested in #t-compiler/linker > RISC-V global pointer relaxations, this prepends --no-relax-gp to the link args on RISC-V to be sure we're not performing global pointer relaxations on a target that may not support them. --no-relax-gp is currently the default on lld but not ld. This is with the goal of eventually stabilizing the relax target feature, as afaict behaviour around gp is the remaining blocker.

I also have a branch where relax is enabled by default on these targets (#t-compiler/risc-v > Should `-C target-feature=+relax` become the default?), but the benefits are less clear-cut than I thought they would be, so this is a safer starting point.

In the future, this could be switched to --relax-gp on targets known to support global pointer relaxations. If people want to opt in (say, for a bare metal target where they've independently set up gp), they can just add -Clink-arg={-Wl,}--relax-gp.

r? wg-linker

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

[rustbot]

Thanks for the pull request, and welcome! The Rust Project has assigned @mati865 (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Please see the contribution instructions and our LLM policy for more information.

Why was this reviewer chosen?

The reviewer was selected based on:

• wg-linker expanded to davidlattimore, jyn514, madsmtm, mati865
• Random selection from davidlattimore, mati865

[TechnoPorg]

@rustbot label O-riscv

[mati865]

I have no idea on how RISC-V works, but I guess it should be discussed with maintainers of each individual target.
I'll try pinging maintainer of what appears to be the most prominent target (riscv64gc-unknown-linux-gnu) for some advice. But you might need to split it into smaller chunks and get the approval from each one.

cc @kito-cheng @michaelmaitland @robin-randhawa-sifive @topperc

[mati865]

A suggested on Zulip, pinging RISC-V team: @denisvasilik @kraj @sweiglbosker

I have no idea on how RISC-V works, but I guess it should be discussed with maintainers of each individual target.
I'll try pinging [..] for some advice.

Also nominating for the next triage meeting.

[TechnoPorg]

Another option which has occurred to me is to make it a new flag -Crelax={off,yes,gp}. This would be more aligned with other codegen options like -Cforce-frame-pointers which aren't related to a CPU feature, but feels quite invasive to add for this RISC-V-specific thing.

[apiraino]

This issue was nominated for T-compiler discussion but it will be useful if by then we get some opinions from people familiar with this compile target, see previous comment (thanks)

[kraj]

On Linux the C startup code defines __global_pointer$ and initializes gp, GNU ld already disables GP relaxation for shared objects, so GP relaxation in executables is a supported and valid optimization.

• Rust objects aren't relaxed today anyway (relax isn't enabled), so the flag mainly takes the optimization away from C objects in mixed Rust/C links. It prevents no failure there.
• Where gp really is used for something else (shadow call stack on Android and Fuchsia, some bare-metal setups), lld already defaults to not relaxing.

View changes since this review

[kraj]

GNU ld only accepts --relax-gp / --no-relax-gp from binutils 2.41 (July 2023+); in 2.40 the RISC-V part of ld has no extra options at all

Rust's dist-riscv64-linux-gnu builder, which uses crosstool-ng with CT_BINUTILS_V_2_40 and also builds riscv64a23. That job only runs in the full bors CI, which is why the PR checks pass.

For the -linux- targets the linker is the user's gcc and its ld, so rustc can't assume a new enough binutils.

mold linker only has --relax/--no-relax

System lld linker older than LLVM 17 doesn't know it either

rust-lld is fine but check Fuschia it uses Gnu(Cc::No, Lld::Yes), by default and it might be losing its linker arguments after this change.

[TechnoPorg]

Currently the PR merges these args with the fuchsia base args, so as far as I can tell nothing will be lost.

[topperc]

I don't think clang or gcc ever pass --no-relax-gp to the linker today. Why would rust need to pass it if C/C++ doesn't?

If a baremetal environment doesn't use GP as a global pointer the __global_pointer symbol won't exist. I think that should prevent GP relaxation in the linker, but I'm not sure.

[TechnoPorg]

Ah. I had been purely going off the comments in #109860 without cross-checking, and it does indeed appear that gp relaxation is not performed if __global_pointer is not defined, which clears up the problem of targets not supporting it. Ty for pointing that out and sorry for not having checked myself.

That PR also mentioned a bug in LLD with handling GP relaxations, which @topperc you would know more about than me: llvm/llvm-project#72405. So I suppose it would still need to be decided how we want to handle GP relaxations before relaxation can be stabilized.

[topperc]

Ah. I had been purely going off the comments in #109860 without cross-checking, and it does indeed appear that gp relaxation is not performed if __global_pointer is not defined, which clears up the problem of targets not supporting it. Ty for pointing that out and sorry for not having checked myself.

That PR also mentioned a bug in LLD with handling GP relaxations, which @topperc you would know more about than me: llvm/llvm-project#72405. So I suppose it would still need to be decided how we want to handle GP relaxations before relaxation can be stabilized.

I don't think anyone has demonstrated the compiler generating code that hits that bug and GP relaxation is disabled by default in LLD. So enabling +relax wouldn't expose that bug unless the user also enabled GP relaxation.

[TechnoPorg]

Based on @kraj and @topperc's comments above, it seems this change is both undesirable (flag isn't universally supported) and unnecessary (global pointer relaxation is only performed if __global_pointer is defined, so enabling linker relaxation won't cause UB on targets without it).

I'll leave this PR open for a few more days in case any target maintainers want to chime in, but otherwise it seems the PR can be closed and the relax target feature can be stabilized as-is?

[mati865]

I was away last week, so thank you everyone who replied here.

otherwise it seems the PR can be closed and the relax target feature can be stabilized as-is?

I'm not familiar with the history of that target feature but the name seems ambiguous to me. --relax and --relax-gp are two distinct linker flags, and while the discussion here is cantered around --relax-gp, the feature name suggests --relax. Rustc help doesn't clear that up either:

✦ ❯ rustc --print target-features --target riscv64gc-unknown-linux-gnu | rg relax
    relax                                - Enable Linker relaxation.

This should be brought up in the stabilisation report to make sure we are fine with that.