[PATCH] D75936: Add a Pass to X86 that builds a Condensed CFG for Load Value Injection (LVI) Gadgets [4/6]

[Scott Constable via Phabricator via llvm-commits]

sconstab added inline comments.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:322
+  DenseSet<std::pair<GraphIter, GraphIter>> GadgetEdgeSet;
+  auto AnalyzeUse = [&](NodeAddr<UseNode *> Use, MachineInstr *MI) {
+    assert(!(Use.Addr->getFlags() & NodeAttrs::PhiRef));
----------------
craig.topper wrote:
> mattdr wrote:
> > Please add a comment explaining the semantics of the boolean return here. I //think// it's: `true` if we need to consider defs of this instruction tainted by this use (and therefore add them for analysis); `false` if this instruction consumes its use
> Was this comment addressed?
It had not been addressed, so thank you for pointing this out. That lambda was doing too many things at once, which made it more confusing than it needed to be. So I just inlined it in the
```
for (auto N : Uses) {
…
}
```
loop, and I added some additional clarifying comments.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:341
+                        GadgetBegin.first, GadgetEnd.first);
+      if (UseMI.mayLoad()) // FIXME: This should be more precise
+        return false;      // stop traversing further uses of `Reg`
----------------
craig.topper wrote:
> mattdr wrote:
> > Some more detail would be useful here: precise about what? What are the likely errors?
> > 
> Was this answered somewhere?
This was referring to the use of `mayLoad()`. At the time I wrote that comment, I wasn't sure that `mayLoad()` was exactly what was needed there, but I now think that it does suffice (SLH also uses `MachineInstr::mayLoad()`).


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:409
+        NodeList Defs = SA.Addr->members_if(DataFlowGraph::IsDef, DFG);
+        llvm::for_each(Defs, AnalyzeDef);
+      }
----------------
craig.topper wrote:
> mattdr wrote:
> > We analyze every def from every instruction in the function, but then //also// in `AnalyzeDefUseChain` analyze every def of every instruction with an interesting use. Are we doing a lot of extra work?
> Was this answered somewhere?
Wow, big oversight on my part. @mattdr was correct that this was doing a LOT of extra work. I added a memoization scheme that remembers the instructions that may transmit for each def. The getGadgetGraph() routine now runs about 75% faster.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75936/new/

https://reviews.llvm.org/D75936