[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..

[David Majnemer via Phabricator via llvm-commits]

majnemer added inline comments.


================
Comment at: clang/lib/Basic/Targets/X86.cpp:1306
       .Case("rdseed", HasRDSEED)
+      .Case("retpoline", HasRetpoline)
       .Case("rtm", HasRTM)
----------------
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...


================
Comment at: llvm/lib/CodeGen/IndirectBrExpandPass.cpp:97-99
+  SmallDenseMap<BasicBlock *, intptr_t, 4> BBToIndex;
+  for (BasicBlock &BB : F) {
+    int BBIndex = -1;
----------------
BBToIndex is to `intptr_t` but BBIndex is an `int`, shouldn't they agree?


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


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


https://reviews.llvm.org/D41723