This is an archive of the discontinued LLVM Phabricator instance.

[clang][driver] Disable GP relaxation with RISC-V ShadowCallStack
Needs RevisionPublic

Authored by paulkirth on Apr 11 2023, 11:11 AM.

Details

Summary

GP linker relaxation conflicts with the use of x3 (gp) register as
the SCS register on RISC-V. To avoid incompatibility, we pass the
--no-relax-gp to the linker.

Depends on D147983, D148031

Diff Detail

Event Timeline

paulkirth created this revision.Apr 11 2023, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2023, 11:11 AM
paulkirth requested review of this revision.Apr 11 2023, 11:11 AM
MaskRay requested changes to this revision.EditedApr 11 2023, 1:22 PM

Sorry, we cannot do this. See https://reviews.llvm.org/D147983#4259132
I added --[no-]relax-gp to GNU ld master branch recently, which isn't in a released binutils version yet.
Passing --no-relax-gp will break almost all GNU ld users.

For lld, we have made a decision to not enable global pointer relaxation by default, so this option isn't really needed.
It doesn't hurt to receive the option, though, once lld supports the options.

I wonder whether we can let GCC autoconf for certain riscv*- target triples (perhaps just some Linux's) detect ld --relax-gp support and set --relax-gp in linker specs and change GNU ld to default to --no-relax-gp in the future,
given someone's stance on https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/371

On Clang and lld sides, we just assume --no-relax-gp is the default. Don't pass any --[no-]relax-gp to the linker (which we don't know the version).

This revision now requires changes to proceed.Apr 11 2023, 1:22 PM
asb added a comment.Apr 12 2023, 2:01 AM

Will --[no-]relax-gp make its way into a minor gcc point release or do we need to wait for the next major release?

In terms of this breaking GNU users - isn't it the case that without this option, they may get silently broken code when using the shadow call stack? Breaking loudly and early seems preferable, though of course it would be best if it's easily fixable by e.g. updating to a newer released binutils.

One slight tweak might be to avoid adding --no-relax-gp if linker relaxation is already disabled, though it's not going to matter once binutils gets support for --no-relax-gp.

I'm not sure I want to suggest this, but could we disable the emission of the relocations that can be GP relaxed?

I'm not sure I want to suggest this, but could we disable the emission of the relocations that can be GP relaxed?

That would also lose the ability to do x0-relative and LUI -> C.LUI relaxation... though given the former only has a simm12 field that's rather limited in use.

I'm not sure I want to suggest this, but could we disable the emission of the relocations that can be GP relaxed?

That would also lose the ability to do x0-relative and LUI -> C.LUI relaxation... though given the former only has a simm12 field that's rather limited in use.

I assume x0-relative wouldn't be useful for Android/Linux since nothing should be mapped to the zero page.

Will --[no-]relax-gp make its way into a minor gcc point release or do we need to wait for the next major release?

In terms of this breaking GNU users - isn't it the case that without this option, they may get silently broken code when using the shadow call stack? Breaking loudly and early seems preferable, though of course it would be best if it's easily fixable by e.g. updating to a newer released binutils.

Yes, -fsanitize=shadow-call-stack using gp users will get silently broken code if linked with GNU ld, unless GNU ld is specified, or -Wl,--no-relax or -Wl,--no-relax-gp is specified.
This is an instance of the guideline proposed in https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/371/commits/bb0df41a4f2626fa039173c2a975039905dce99c

For such platforms, care must be taken to ensure all code (compiler generated or otherwise) avoids using gp in a way incompatible with the platform specific purpose, and that global pointer relaxation is disabled in the toolchain.

Personally I think most -fsanitize=shadow-call-stack users do not use GNU ld, so this incompatibility is actually minor.

-fsanitize=shadow-call-stack is already a quite specific configuration. For GNU ld users, I think placing the burden more on user education is fine (sorry, just that we don't have better options).

We have experience that even when the linker option --push-state has been available in GNU ld for ~5 years, we don't use it in Clang Driver, since using the option in the default configuration will break old GNU ld users.

One slight tweak might be to avoid adding --no-relax-gp if linker relaxation is already disabled, though it's not going to matter once binutils gets support for --no-relax-gp.

Compile actions and link actions can be separate. Unfortunately Clang Driver for the link action does not have sufficient information whether linker relaxation has been disabled for all input relocatable object files.

asb added a comment.Apr 13 2023, 6:00 AM

Will --[no-]relax-gp make its way into a minor gcc point release or do we need to wait for the next major release?

In terms of this breaking GNU users - isn't it the case that without this option, they may get silently broken code when using the shadow call stack? Breaking loudly and early seems preferable, though of course it would be best if it's easily fixable by e.g. updating to a newer released binutils.

Yes, -fsanitize=shadow-call-stack using gp users will get silently broken code if linked with GNU ld, unless GNU ld is specified, or -Wl,--no-relax or -Wl,--no-relax-gp is specified.
This is an instance of the guideline proposed in https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/371/commits/bb0df41a4f2626fa039173c2a975039905dce99c

For such platforms, care must be taken to ensure all code (compiler generated or otherwise) avoids using gp in a way incompatible with the platform specific purpose, and that global pointer relaxation is disabled in the toolchain.

Personally I think most -fsanitize=shadow-call-stack users do not use GNU ld, so this incompatibility is actually minor.

-fsanitize=shadow-call-stack is already a quite specific configuration. For GNU ld users, I think placing the burden more on user education is fine (sorry, just that we don't have better options).

I just wanted to make sure I'm following your suggestion here. Have you come round to the idea of adding --no-relax-gp when shadow call stack is enabled and letting it error for users of old GNU tools on the basis that there aren't better options?

Will --[no-]relax-gp make its way into a minor gcc point release or do we need to wait for the next major release?

In terms of this breaking GNU users - isn't it the case that without this option, they may get silently broken code when using the shadow call stack? Breaking loudly and early seems preferable, though of course it would be best if it's easily fixable by e.g. updating to a newer released binutils.

Yes, -fsanitize=shadow-call-stack using gp users will get silently broken code if linked with GNU ld, unless GNU ld is specified, or -Wl,--no-relax or -Wl,--no-relax-gp is specified.
This is an instance of the guideline proposed in https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/371/commits/bb0df41a4f2626fa039173c2a975039905dce99c

For such platforms, care must be taken to ensure all code (compiler generated or otherwise) avoids using gp in a way incompatible with the platform specific purpose, and that global pointer relaxation is disabled in the toolchain.

Personally I think most -fsanitize=shadow-call-stack users do not use GNU ld, so this incompatibility is actually minor.

-fsanitize=shadow-call-stack is already a quite specific configuration. For GNU ld users, I think placing the burden more on user education is fine (sorry, just that we don't have better options).

I just wanted to make sure I'm following your suggestion here. Have you come round to the idea of adding --no-relax-gp when shadow call stack is enabled and letting it error for users of old GNU tools on the basis that there aren't better options?

I added --no-relax-gp because I imagined many use cases not wanting GNU ld's default global pointer relaxation behavior.
(I had had the patch long before it finally landed. I did not understand Andrew's stance and objection and what he would accept. If anyone is curious, please dig up the original binutils thread.)

I don't think enabling software shadow call stack should pass --no-relax-gp to the linker: the driver isn't in a good position to know whether the linker is a latest GNU ld that supports --no-relax-gp.
GNU ld users may specify -Wl,--no-relax and don't need --no-relax-gp. Clang Driver should not inspect -Wl, options to make a "smart" decision.

The compatibility burden lies on the user side. Users should be aware of the GNU ld's default behavior possibly causing problems.
Clang Driver being smart is more likely to get in the way. As mentioned, clang -fsanitize=shadow-call-stack users likely don't use GNU ld, so the pitfall situation is not great but I think is acceptable.