[PATCH] D146463: [CodeGen][RISCV] Change Shadow Call Stack Register to X3

[Roland McGrath via Phabricator via cfe-commits]

mcgrathr accepted this revision.
mcgrathr added a comment.

lgtm with some nits and a few more test cases.



================
Comment at: clang/docs/ShadowCallStack.rst:61
+The instrumentation makes use of the platform register ``x18`` on AArch64 and
+``x3`` on RISC-V. For simplicity we will refer to this as the ``SCSReg`` On
+some platforms, ``SCSReg`` is reserved, and on others, it is designated as a
----------------
I think it would be worthwhile to say `x3 (gp)` here and perhaps everywhere that `x3` is mentioned.
Since `gp` is always a reserved register from the compiler point of view, I think some different verbiage here about the RISC-V case and the interactions with `-msmall-data-limit` and linker behavior make sense.




================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:307
 
+bool shadowCallStackDSLConflicts(const llvm::opt::ArgList &Args,
+                                 const llvm::Triple &TT) {
----------------
I'm guessing that "DSL" stands for "data small limit" or something, which is confusing since the thing is called small-data-limit, not data-small-limit. Anyway, this is the first use I've seen of DSL as an abbreviation related to any of this. I think it would be clear enough to just call it `shadowCallStackConflicts` since it's responsible for detecting conflicts between shadow-call-stack and whatever is relevant to that in the particular target.



================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:314
+  if (Arg *A = Args.getLastArg(options::OPT_G))
+    if (0 != strcmp("0", A->getValue()))
+      return true;
----------------
This seems like it ought to parse the integer and then compare it to zero as an integer, rather than relying on one exact string representation of zero.



================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:560
+  if (Kinds & SanitizerKind::ShadowCallStack) {
+    if (TC.getTriple().isAArch64() &&
+        !llvm::AArch64::isX18ReservedByDefault(TC.getTriple()) &&
----------------
I'd just move all this per-target logic inside `shadowCallStackConflicts` and maybe just make it a void function called `checkShadowCallStackConflcits` or something.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2063
+  // Default small data limitation on some platforms(Android and Fuchsia) is
+  // zero, otherwise its eight.
+  const char *SmallDataLimit =
----------------
typo: "it's"


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2065
+  const char *SmallDataLimit =
+      llvm::RISCV::isSmallDataLimitZeroByDefault(Triple) ? "0" : "8";
   // Get small data limitation.
----------------
I wonder if it wouldn't be more natural for the target hook to be `defaultSmallDataLimit` and return an integer where the base class returns 8 and the android/fuchsia subclasses return 0. It seems quite plausible that eventually different subtargets might have other tunings (for subtargets that do want to use gp relaxation).



================
Comment at: clang/test/Driver/sanitizer-ld.c:746
 
 // RUN: %clang -target riscv64-linux-android -fsanitize=shadow-call-stack %s -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-SHADOWCALLSTACK-ANDROID-RISCV64
----------------
I'd also add tests here for android and fuchsia targets with explicit `-msmall-data-limit=<nonzero>` being rejected by default but being allowed with `-fno-sanitize=shadow-call-stack`.

There should also be tests independent of shadow-call-stack that just verify at least for fuchsia targets that `-msmall-data-limit=0` is passed by default for the default (PIE) case. Actually, it should also test that this remains so when `-fno-sanitize=shadow-call-stack` is passed, because it's still the standard target ABI that `gp` might be used for shadow-call-stack by interoperating code and so `gp` should never be used for something else without explicit `-msmall-data-limit=...` on fuchsia.




================
Comment at: llvm/docs/ReleaseNotes.rst:147
 * Added support for the vendor-defined XTHeadFMemIdx (indexed memory operations for floating point) extension.
+* Changed the ShadowCallStack register from x18(s2) to x3(gp). Note this breaks
+  the existing non-standard ABI for ShadowCallStack, but conforms with the new
----------------
Spaces before the parenthetical clauses.



================
Comment at: llvm/include/llvm/TargetParser/RISCVTargetParser.h:43
 bool getCPUFeaturesExceptStdExt(CPUKind Kind, std::vector<StringRef> &Features);
-
-bool isX18ReservedByDefault(const Triple &TT);
+bool isSmallDataLimitZeroByDefault(const Triple &TT);
 
----------------
Why not return an integer value instead of "is zero"?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146463