[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls accepted this revision.
kristof.beyls added a comment.

I'm not an expert on many of the areas touched by this patch, but it looks fine from me from a high-level point-of-view, modulo a few nits I have on a few comments.



================
Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:151
+  // flags). This is a bit hacky but keeps existing usages working. We should
+  // consider deprecated this and instead warning if the user requests external
+  // retpoline thunks and *doesn't* request some form of retpolines.
----------------
s/deprecated/deprecating/
s/warning/warn/


================
Comment at: llvm/include/llvm/IR/Attributes.td:181-185
+/// Note that this uses the default compatibility (always compatible during
+/// inlining) and the default merge strategy of retaining the caller's
+/// attribute. This specifically matches the intent for this attribute which is
+/// that the context dominates, and inlined code will become hardened or lose
+/// its hardening based on the caller's attribute.
----------------
After updating the LangRef.rst text, I think this comment also needs to be updated. As is, it still documents the old inlining behaviour?
I'm also not sure in how far the comment makes most sense here. This is already documented in LangRef.rst, and AFAIK, the inlining compatibility mode is not something that is defined here?


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:78-82
+static cl::opt<bool> EnableSpeculativeLoadHardening(
+    "x86-speculative-load-hardening",
+    cl::desc("Force enable speculative load hardening"), cl::init(false),
+    cl::Hidden);
+
----------------
I'm not sure, but do you really still need an option to force enable SLH when you have function attributes now to enable it?
When you generate LLVM-IR using clang, you now have a clang option to enable that function attribute on all functions, so do you still have scenarios where you need an LLVM backend option to override the function attribute?


================
Comment at: llvm/lib/Target/X86/X86TargetMachine.cpp:474
 
-  if (EnableSpeculativeLoadHardening)
-    addPass(createX86SpeculativeLoadHardeningPass());
+  // Will only run if force enabled or detects the relevant attribute.
+  addPass(createX86SpeculativeLoadHardeningPass());
----------------
I guess this is true for some other passes too, and they don't add such a comment here. Maybe best to remove this comment if my guess is right?


Repository:
  rL LLVM

https://reviews.llvm.org/D51157