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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab accepted this revision.
ab added a comment.

Clang/LLVM pieces LGTM.  Thanks for doing this, people.



================
Comment at: llvm/lib/CodeGen/IndirectBrExpandPass.cpp:56
+
+private:
+};
----------------
Nit: Unnecessary 'private'?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27120
+  for (const auto &MO : MI.operands()) {
+    if (MO.isReg() && MO.isUse())
+      for (unsigned &Reg : AvailableRegs)
----------------
I wonder: should this also check the CSR regmask?  There are a couple CCs that do preserve R11, but it probably doesn't matter for -mretpoline users. E.g.,


```
define void @t(void ()* %f) {
  call cc 17 void %f()
  ret void
}
```

Worth an assert?


================
Comment at: llvm/lib/Target/X86/X86RetpolineThunks.cpp:119
+    // .Lr11_call_target:
+    //   movq %r11, (%rsp)
+
----------------
'retq' at the end


================
Comment at: llvm/lib/Target/X86/X86RetpolineThunks.cpp:171
+  return true;
+}
+Function *X86RetpolineThunks::createThunkFunction(Module &M, StringRef Name) {
----------------
Nit: newlines between functions


================
Comment at: llvm/lib/Target/X86/X86RetpolineThunks.cpp:183
+  B.addAttribute(llvm::Attribute::NoUnwind);
+  B.addAttribute(llvm::Attribute::NoInline);
+  B.addAttribute(llvm::Attribute::Naked);
----------------
'noinline' makes sense, but I'm curious: is it necessary?


================
Comment at: llvm/lib/Target/X86/X86Subtarget.h:344
 
+  /// Processor needs retpolin to avoid branch prediction errors.
+  bool UseRetpoline;
----------------
Nit: "Use retpoline to avoid ..."


https://reviews.llvm.org/D41723