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