[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 marked an inline comment as done.
chandlerc added a comment.

All comments addressed now I think, thanks everyone! See some thoughts about a few of these below though.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27120
+  for (const auto &MO : MI.operands()) {
+    if (MO.isReg() && MO.isUse())
+      for (unsigned &Reg : AvailableRegs)
----------------
ab wrote:
> 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?
We should already have the assert below? If we get through this and have no available reg, we assert that we're in 32-bit.

That said, this really is a case that will come up. We shouldn't silently miscompile the code. I've changed both cases to `report_fatal_error` because these are fundamental incompatibilities with retpoline.


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

Actually, I don't think any of these are necessary because we hand build the MI and we are the last pass before the asm printer. But I've left the others for now. They seem at worst harmless.


https://reviews.llvm.org/D41723