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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
