[PATCH] D41723: Introduce the "retpoline" x86 mitigation technique for variant #2 of the speculative execution vulnerabilities disclosed today, specifically identified by CVE-2017-5715, "Branch Target Injection", and is one of the two halves to Spectre..

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Thanks for the review!



================
Comment at: clang/lib/Basic/Targets/X86.cpp:1306
       .Case("rdseed", HasRDSEED)
+      .Case("retpoline", HasRetpoline)
       .Case("rtm", HasRTM)
----------------
majnemer wrote:
> Why is this phrased as a target feature? It just seems weird as it is not a hardware capability (all X86 hardware should support the reptoline instruction sequence unless I am mistaken).
> 
> Also, it seems that other hardware platforms beyond Intel have problems with speculation of this nature. IMO, this is more similar to things like the relocation-model and the code-model...
All x86-64 hardware supports SSE and SSE2, but we can still turn them on and off?

I think it makes some sense as it *only* impacts the target's code generation, not anything produced by Clang etc, and this seems to be a way to pass that down.


And while other architectures may want to introduce similar mitigations for Spectre, the retpoline construct itself is pretty x86-specific. My suspicion is that other architectures will potentially re-use some of the LLVM infrastructure like removing indirectbr, but want to handle the hard cases with a target-specific technique. So something like relocation-model and code-model seem weird to me as this is (at least so far) a fundamentally x86 thing.


================
Comment at: llvm/lib/CodeGen/IndirectBrExpandPass.cpp:97-99
+  SmallDenseMap<BasicBlock *, intptr_t, 4> BBToIndex;
+  for (BasicBlock &BB : F) {
+    int BBIndex = -1;
----------------
majnemer wrote:
> BBToIndex is to `intptr_t` but BBIndex is an `int`, shouldn't they agree?
I just did this out of habit to avoid padding in the map's pair object. The size of this isn't really significant unless we have over 2 billion basic blocks with their address taken....

Still, if this bothers folks I can change it either direction.


================
Comment at: llvm/lib/CodeGen/IndirectBrExpandPass.cpp:161
+
+      int BBIndex = InsertResult.first->second;
+      if (BBIndex == 0)
----------------
majnemer wrote:
> I would think this should also agree with BBToIndex.
Or should BBToIndex just use an 'int'?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27082-27083
+  switch (Reg) {
+  default:
+    break;
+  case X86::EAX:
----------------
majnemer wrote:
> I'd remove this empty default to make `getRetpolineSymForReg` more similar to `getOpcodeForRetpoline`.
Good call, done.


https://reviews.llvm.org/D41723