[PATCH] D148034: [clang][driver] Disable GP relaxation with RISC-V ShadowCallStack

2023-04-13 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

In D148034#4264996 , @asb wrote:

> In D148034#4262991 , @MaskRay wrote:
>
>> In D148034#4260376 , @asb wrote:
>>
>>> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148034/new/

https://reviews.llvm.org/D148034

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148034: [clang][driver] Disable GP relaxation with RISC-V ShadowCallStack

2023-04-13 Thread Alex Bradbury via Phabricator via cfe-commits 
asb added a comment.

In D148034#4262991 , @MaskRay wrote:

> In D148034#4260376 , @asb wrote:
>
>> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148034/new/

https://reviews.llvm.org/D148034

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148034: [clang][driver] Disable GP relaxation with RISC-V ShadowCallStack

2023-04-12 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

In D148034#4260376 , @asb wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148034/new/

https://reviews.llvm.org/D148034

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148034: [clang][driver] Disable GP relaxation with RISC-V ShadowCallStack

2023-04-12 Thread Craig Topper via Phabricator via cfe-commits 
craig.topper added a comment.

In D148034#4262938 , @jrtc27 wrote:

> In D148034#4262934 , @craig.topper 
> wrote:
>
>> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148034/new/

https://reviews.llvm.org/D148034

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148034: [clang][driver] Disable GP relaxation with RISC-V ShadowCallStack

2023-04-12 Thread Jessica Clarke via Phabricator via cfe-commits 
jrtc27 added a comment.

In D148034#4262934 , @craig.topper 
wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148034/new/

https://reviews.llvm.org/D148034

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148034: [clang][driver] Disable GP relaxation with RISC-V ShadowCallStack

2023-04-12 Thread Craig Topper via Phabricator via cfe-commits 
craig.topper added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148034/new/

https://reviews.llvm.org/D148034

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148034: [clang][driver] Disable GP relaxation with RISC-V ShadowCallStack

2023-04-12 Thread Alex Bradbury via Phabricator via cfe-commits 
asb added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148034/new/

https://reviews.llvm.org/D148034

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148034: [clang][driver] Disable GP relaxation with RISC-V ShadowCallStack

2023-04-11 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148034/new/

https://reviews.llvm.org/D148034

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148034: [clang][driver] Disable GP relaxation with RISC-V ShadowCallStack

2023-04-11 Thread Paul Kirth via Phabricator via cfe-commits 
paulkirth created this revision.
paulkirth added reviewers: phosek, mcgrathr, jrtc27, craig.topper, 
aaron.ballman.
Herald added subscribers: VincentWu, vkmr, luismarques, sameer.abuasal, 
s.egerton, Jim, PkmX, rogfer01, shiva0217, kito-cheng, simoncook, asb, 
arichardson.
Herald added a project: All.
paulkirth requested review of this revision.
Herald added subscribers: cfe-commits, pcwang-thead, MaskRay.
Herald added a project: clang.

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 



Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148034

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/sanitizer-ld.c


Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -738,18 +738,32 @@
 // RUN:   | FileCheck --check-prefix=CHECK-SHADOWCALLSTACK-ELF-RISCV32 %s
 // CHECK-SHADOWCALLSTACK-ELF-RISCV32: '-fsanitize=shadow-call-stack' only 
allowed with '-ffixed-x18'
 
+// RUN: %clang -fsanitize=shadow-call-stack -### %s 2>&1 \
+// RUN: --target=riscv32-unknown-linux -fuse-ld=ld -ffixed-x18 \
+// RUN:   | FileCheck 
--check-prefix=CHECK-SHADOWCALLSTACK-ELF-RISCV32-FIXED-X18 %s
+// CHECK-SHADOWCALLSTACK-ELF-RISCV32-FIXED-X18: --no-relax-gp
+// CHECK-SHADOWCALLSTACK-ELF-RISCV32-FIXED-X18-NOT: error:
+
 // RUN: %clang -fsanitize=shadow-call-stack -### %s 2>&1 \
 // RUN: --target=riscv64-unknown-linux -fuse-ld=ld \
 // RUN:   | FileCheck --check-prefix=CHECK-SHADOWCALLSTACK-LINUX-RISCV64 %s
 // CHECK-SHADOWCALLSTACK-LINUX-RISCV64: '-fsanitize=shadow-call-stack' only 
allowed with '-ffixed-x18'
 
+// RUN: %clang -fsanitize=shadow-call-stack -### %s 2>&1 \
+// RUN: --target=riscv64-unknown-linux -fuse-ld=ld -ffixed-x18 \
+// RUN:   | FileCheck 
--check-prefix=CHECK-SHADOWCALLSTACK-LINUX-RISCV64-FIXED-X18 %s
+// CHECK-SHADOWCALLSTACK-LINUX-RISCV64-FIXED-X18: --no-relax-gp
+// CHECK-SHADOWCALLSTACK-LINUX-RISCV64-FIXED-X18-NOT: error:
+
 // RUN: %clang -target riscv64-linux-android -fsanitize=shadow-call-stack %s 
-### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-SHADOWCALLSTACK-ANDROID-RISCV64
+// CHECK-SHADOWCALLSTACK-ANDROID-RISCV64: --no-relax-gp
 // CHECK-SHADOWCALLSTACK-ANDROID-RISCV64-NOT: error:
 
 // RUN: %clang -fsanitize=shadow-call-stack -### %s 2>&1 \
 // RUN: --target=riscv64-unknown-fuchsia -fuse-ld=ld \
 // RUN:   | FileCheck --check-prefix=CHECK-SHADOWCALLSTACK-FUCHSIA-RISCV64 %s
+// CHECK-SHADOWCALLSTACK-FUCHSIA-RISCV64: --no-relax-gp
 // CHECK-SHADOWCALLSTACK-FUCHSIA-RISCV64-NOT: error:
 
 // RUN: %clang -fsanitize=shadow-call-stack -### %s 2>&1 \
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1183,6 +1183,9 @@
   CmdArgs.push_back("--android-memtag-stack");
   }
 
+  if (SanArgs.hasShadowCallStack() && TC.getTriple().isRISCV())
+CmdArgs.push_back("--no-relax-gp");
+
   return !StaticRuntimes.empty() || !NonWholeStaticRuntimes.empty();
 }
 


Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -738,18 +738,32 @@
 // RUN:   | FileCheck --check-prefix=CHECK-SHADOWCALLSTACK-ELF-RISCV32 %s
 // CHECK-SHADOWCALLSTACK-ELF-RISCV32: '-fsanitize=shadow-call-stack' only allowed with '-ffixed-x18'
 
+// RUN: %clang -fsanitize=shadow-call-stack -### %s 2>&1 \
+// RUN: --target=riscv32-unknown-linux -fuse-ld=ld -ffixed-x18 \
+// RUN:   | FileCheck --check-prefix=CHECK-SHADOWCALLSTACK-ELF-RISCV32-FIXED-X18 %s
+// CHECK-SHADOWCALLSTACK-ELF-RISCV32-FIXED-X18: --no-relax-gp
+// CHECK-SHADOWCALLSTACK-ELF-RISCV32-FIXED-X18-NOT: error:
+
 // RUN: %clang -fsanitize=shadow-call-stack -### %s 2>&1 \
 // RUN: --target=riscv64-unknown-linux -fuse-ld=ld \
 // RUN:   | FileCheck --check-prefix=CHECK-SHADOWCALLSTACK-LINUX-RISCV64 %s
 // CHECK-SHADOWCALLSTACK-LINUX-RISCV64: '-fsanitize=shadow-call-stack' only allowed with '-ffixed-x18'
 
+// RUN: %clang -fsanitize=shadow-call-stack -### %s 2>&1 \
+// RUN: --target=riscv64-unknown-linux -fuse-ld=ld -ffixed-x18 \
+// RUN:   | FileCheck --check-prefix=CHECK-SHADOWCALLSTACK-LINUX-RISCV64-FIXED-X18 %s
+// CHECK-SHADOWCALLSTACK-LINUX-RISCV64-FIXED-X18: --no-relax-gp
+// CHECK-SHADOWCALLSTACK-LINUX-RISCV64-FIXED-X18-NOT: error:
+
 // RUN: %clang -target riscv64-linux-android -fsanitize=shadow-call-stack %s -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-SHADOWCALLSTACK-ANDROID-RISCV64
+// CHECK-SHADOWCALLSTACK-ANDROID-RISCV64: --no-relax-gp
 //