nickdesaulniers added a comment.
Thanks for the patch!
> A tangential question is do we need these two options expect all?
For the Linux kernel's use, no. But I think it would extremely simple to
implement. Instead of having one `SubtargetFeature` (`FeatureHardenSlsAll`),
you'd have two (say `FeatureHardenSlsRet` and `FeatureHardenSlsInd`). Then the
front-end decomposes `-mharden-sls=all` into BOTH `SubtargetFeatures`. You've
already implemented support for either in `X86AsmPrinter::emitBasicBlockEnd`.
You condition could instead become something like `if (... &&
((Subtarget->hardenSlsRet() && I->getDesc().isReturn()) ||
(Subtarget->hardenSlsInd() && I->getDesc().isIndirectBranch())))`. You'd also
need the frontend to recognize the two additional values. Do you think that's
doable @pengfei ?
---
Consider the following test case:
void foo(void);
void bar(void) {
void (*x)(void) = foo;
x();
}
With this patch, `clang -mharden-sls=all x.c -c -O2` produces:
0000000000000000 <bar>:
0: e9 00 00 00 00 jmp 0x5 <bar+0x5>
0000000000000001: R_X86_64_PLT32 foo-0x4
5: cc int3
while `gcc -mharden-sls=all x.c -c -O2` produces
0000000000000000 <bar>:
0: e9 00 00 00 00 jmp 0x5 <bar+0x5>
0000000000000001: R_X86_64_PLT32 foo-0x4
So we pessimize tail calls. Please fix and add a test case for that. This might
be an unintended side effect of using `isUnconditionalBranch`.
---
Relevant GCC commits:
c2e5c4feed32c808591b5278f680bbabe63eb225 ("x86: Generate INT3 for
__builtin_eh_return")
53a643f8568067d7700a9f2facc8ba39974973d3 ("x86: Add
-mharden-sls=[none|all|return|indirect-branch]")
The first seems to have some interaction between `-fcf-protection` and
`__builtin_eh_return`. Is that something we need to handle?
================
Comment at: clang/docs/ReleaseNotes.rst:371
+- Support ``-mharden-sls=all`` for X86.
+
----------------
This should be updated if additional options are supported.
You should also update `clang/docs/ClangCommandLineReference.rst` I think.
================
Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:249-250
+ StringRef Scope = A->getValue();
+ if (Scope == "all")
+ Features.push_back("+harden-sls-all");
+ }
----------------
```
else
D.Diag(diag::err_invalid_sls_hardening)
<< Scope << A->getAsString(Args);
```
and add a test for a nonsensical value.
================
Comment at: clang/test/Driver/x86-target-features.c:308-309
+
+// RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mharden-sls=all %s
-### -o %t.o 2>&1 | FileCheck -check-prefix=SLS %s
+// RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mharden-sls=none %s
-### -o %t.o 2>&1 | FileCheck -check-prefix=NO-SLS %s
+// SLS: "-target-feature" "+harden-sls-all"
----------------
Please add a test for `-mharden-sls=all -mharden-sls=none` to verify that the
last value "wins."
================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:342-343
+ auto I = MBB.getLastNonDebugInstr();
+ if (I != MBB.end() && Subtarget->hardenSlsAll() &&
+ (I->getDesc().isUnconditionalBranch() || I->getDesc().isReturn())) {
+ MCInst TmpInst;
----------------
Consider checking `Subtarget->hardenSlsAll()` first, since that's most unlikely
to be unset.
```
if (Subtarget->hardenSlsAll()) {
auto I = ...
}
```
I assume that's cheaper than finding the last non-debug instruction.
================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:343
+ if (I != MBB.end() && Subtarget->hardenSlsAll() &&
+ (I->getDesc().isUnconditionalBranch() || I->getDesc().isReturn())) {
+ MCInst TmpInst;
----------------
Should we be using `MCInstrDesc::isIndirectBranch()` rather than
`MCInstrDesc::isUnconditionalBranch()`? The naming of the command line flag
options seem to imply the former, not the latter.
================
Comment at: llvm/test/CodeGen/X86/speculation-hardening-sls.ll:91
+ ret i32 1
+}
----------------
Please add a test case for the extremely simple:
```
void bar(void (*x)(void)) {
x();
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126137/new/
https://reviews.llvm.org/D126137
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
