[PATCH] D126137: [X86] Add support for `-mharden-sls=[none|all|return|indirect-jmp]`

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:371
 
+- Support ``-mharden-sls=all`` for X86.
+
----------------
MaskRay wrote:
> nickdesaulniers wrote:
> > 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.
> Delete ` for X86`. The section is about X86.
I'd add ` for straight-line speculation hardening`


================
Comment at: clang/include/clang/Driver/Options.td:3525
+  HelpText<"Select straight-line speculation hardening scope (ARM/AArch64/X86 "
+           "only). <arg> must be 'all', 'none', 'retbr'(ARM/AArch64), "
+           "'blr'(ARM/AArch64), 'comdat'(ARM/AArch64), 'nocomdat'(ARM/AArch64),"
----------------
`must be one of: `.

Single quotes can be removed.


================
Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:259
+    } else if (Scope == "indirect-jmp") {
+      Features.push_back("+harden-sls-ind");
+    } else if (Scope != "none") {
----------------
The name harden-sls-ind seems a bit different from indirect-jmp. Use a more descriptive feature name.


================
Comment at: clang/test/Driver/x86-target-features.c:313
+// RUN: %clang --target=i386-unknown-linux-gnu -march=i386 -mharden-sls=return,indirect-jmp %s -### -o %t.o 2>&1 | FileCheck -check-prefix=BAD-SLS %s
+// NO-SLS-NOT: harden-sls
+// SLS-RET-DAG: "-target-feature" "+harden-sls-ret"
----------------
This can cause false positive if the path components of %s have `harden-sls`,

`...-NOT: "+harden-sls"` should be more robust.


================
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"
----------------
MaskRay wrote:
> nickdesaulniers wrote:
> > Please add a test for `-mharden-sls=all -mharden-sls=none` to verify that the last value "wins."
> ` -target ` is deprecated legacy spelling. Better to use `--target=`
If the feature applies to any ELF OSes, use --target=i386 (generic ELF) instead of --target=i386-unknown-linux-gnu.


================
Comment at: llvm/test/CodeGen/X86/speculation-hardening-sls.ll:1
+; RUN: llc -mattr=harden-sls-ret -verify-machineinstrs -mtriple=x86_64-unknown-unknown < %s | FileCheck %s -check-prefixes=CHECK,RET
+; RUN: llc -mattr=harden-sls-ind -verify-machineinstrs -mtriple=x86_64-unknown-unknown < %s | FileCheck %s -check-prefixes=CHECK,IND
----------------
Most `-verify-machineinstrs ` usage is redundant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126137