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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Thanks, should all be addressed now.



================
Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:169-170
                           options::OPT_mno_retpoline_external_thunk, false)) {
     // FIXME: Add a warning about failing to specify `-mretpoline` and
     // eventually switch to an error here.
     Features.push_back("+retpoline-indirect-calls");
----------------
rsmith wrote:
> `-mspeculative-load-hardening -mretpoline-external-thunk` does not enable `+retpoline-indirect-branches` (but `-mretpoline-external-thunk` by itself does). Is that intentional?
Yes, as that's is specifically useful (and that is one thing that motivated my above comment that we should remove the ability to use `-mretpoline-external-thunk` by itself eventually).

You might only need SLH's minimal retpolines, but you might need to provide your own thunks for them in the Kernel for example.


================
Comment at: llvm/include/llvm/IR/Attributes.td:249
 def : MergeRule<"adjustNullPointerValidAttr">;
+
----------------
rnk wrote:
> I can't tell if this is just vim fixing the lack of a proper trailing newline, but revert any whitespace change if possible.
Soryr, my annoying editor. Will fix before submit.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1169
+  case Attribute::SpeculativeLoadHardening:
+    return 1ULL << 60;
   case Attribute::Dereferenceable:
----------------
rsmith wrote:
> rnk wrote:
> > These appear to repeat LLVMBitcodes.h, unless I am mistaken. Weird. Anyway, fixing that is out of scope.
> The values "above the fold" in Phabricator aren't all the same as the enum value plus one. *shrug*
Yeah, this isn't factorable easily. Anyways....


Repository:
  rL LLVM

https://reviews.llvm.org/D51157