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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Diff 431607 boots for me in QEMU (triple checked that CONFIG_CC_HAS_SLS=y AND CONFIG_SLS=y were set this time ;) ).



================
Comment at: clang/docs/ReleaseNotes.rst:371
 
+- Support ``-mharden-sls=all`` for X86.
+
----------------
pengfei wrote:
> nickdesaulniers wrote:
> > This should be updated if additional options are supported.
> > 
> > You should also update `clang/docs/ClangCommandLineReference.rst` I think.
> There's already introduction there.
We should expand the introduction there to state explicitly what options are supported for which targets.


================
Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:252-254
+    } else if (Scope == "none") {
+      Features.push_back("-harden-sls-ind");
+      Features.push_back("-harden-sls-ret");
----------------
I think if the `Scope` is `"none"`, we can simply do nothing.
@craig.topper is there a precedent for target features being "unset" like this? I guess it doesn't matter much either way...


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:345
+      const MCInstrDesc &Desc = I->getDesc();
+      auto IsIndirectTailCall = [I, &Desc]() {
+        return Desc.isCall() && Desc.isReturn() && Desc.isBarrier() &&
----------------
How come you capture `Desc` by reference, and `I` by value? Might it be simpler to just have static functions defined above `X86AsmPrinter::emitBasicBlockEnd` that accepts an MCInstr by reference?


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:346
+      auto IsIndirectTailCall = [I, &Desc]() {
+        return Desc.isCall() && Desc.isReturn() && Desc.isBarrier() &&
+               !I->getOperand(0).isGlobal();
----------------
Why `isBarrier`? Maybe add a comment that mentions a specific TableGen record from llvm/lib/Target/X86/X86InstrControl.td?  That would help me verify that `isBarrier()` is correct here.

I'm guessing this is:

363 let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1,                  
364     isCodeGenOnly = 1, Uses = [RSP, SSP] in {

block in llvm/lib/Target/X86/X86InstrControl.td.

Otherwise llvm/lib/Target/X86/X86FrameLowering.cpp has a static function that I think is precisely what we want here, called ` isTailCallOpcode`. Perhaps move that to a header shared between the two translation units then reuse it here?  A few other backends have an `IsTailCall` method though they're inconsistent where they define it. It still would be good though to try to match existing conventions which makes jumping between backends easier.


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:347
+        return Desc.isCall() && Desc.isReturn() && Desc.isBarrier() &&
+               !I->getOperand(0).isGlobal();
+      };
----------------
Rather than `!...isGlobal()`, would it be more precise to use `getOperand(0).isReg()`?  Looking at all of the values of `enum MachineOperandType`, I think it would be more precise if we check the operand is of one type, rather than "not one type."


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:349
+      };
+      if ((Subtarget->hardenSlsRet() && Desc.isReturn() && !Desc.isCall()) ||
+          (Subtarget->hardenSlsInd() &&
----------------
So this check is logically "isn't a tail call"?

I feel like we could have a helper or lamba for "is this a tail call" and "is this an indirect call" and compose our logic here out of those two.  Having descriptive names would make the code more readable, otherwise one has to think about which instruction is a return and not a call.


================
Comment at: llvm/test/CodeGen/X86/speculation-hardening-sls.ll:7
+; CHECK:         jle
+; CEHCK-NOT:     int3
+; CHECK:         retq
----------------
typo


================
Comment at: llvm/test/CodeGen/X86/speculation-hardening-sls.ll:59
+; CHECK:         jmp .L
+; CEHCK-NOT:     int3
+; CHECK:         retq
----------------
typo


================
Comment at: llvm/test/CodeGen/X86/speculation-hardening-sls.ll:83-94
+; CEHCK-NOT:     ret
+  tail call void %0()
+  ret void
+}
+
+declare dso_local void @foo()
+
----------------
3 typos, s/CEHCK/CHECK/g


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126137