[PATCH] D126137: [X86] Add support for `-mharden-sls=all`

[Nick Desaulniers via Phabricator via llvm-commits]

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