[PATCH] D44824: [Spectre] Introduce a new pass to do speculative load hardening to mitigate Spectre variant #1 for x86.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/docs/SpeculativeLoadHardening.md:567
+gigabytes of address space. While these offsets are not attacker controlled,
+the attacker could chose to attack a load which happens to have the desired
+offset and then successfully read memory in that region. This significantly
----------------
chose->choose


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:778
+
+  case X86::ADDPDrm:
+  case X86::ADDPSrm:
----------------
Can we drop all the vector instructions here? Their AVX and AVX512 equivalents are all missing. It probably makes more sense to add them more methodically. Or add a bit to TSFlags.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:929
+  case X86::UNPCKLPSrm:
+  case X86::XORPDrm:
+    // We don't handle loads into vector registers yet.
----------------
Further proof this list needs a methodical scrub. XORPSrm is missing, while ANDPS, ORPS, ANDNPS are all present.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:965
+  case X86::SUBSSrm_Int:
+  case X86::XORPSrm:
+  case X86::CVTSD2SSrm:
----------------
Oh there's the XORPSrm...


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:1123
+  case X86::CVTTSS2SIrm:
+  case X86::CVTTSS2SIrm_Int:
+
----------------
AVX versions of these are missing, but we can address as a follow up if you want. Just don't want to lose it since none of the instructions here will be used on Haswell with AVX.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:1126
+    // Loads to register don't set flags.
+  case X86::MOV8rm:
+  case X86::MOV16rm:
----------------
May want MOV8rm_NOREX, MOVSX32rm8_NOREX, MOVZX32rm8_NOREX, 


Repository:
  rL LLVM

https://reviews.llvm.org/D44824