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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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));
----------------
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?


================
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`
----------------
mattdr wrote:
> Some more detail would be useful here: precise about what? What are the likely errors?
> 
Was this answered somewhere?


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:409
+        NodeList Defs = SA.Addr->members_if(DataFlowGraph::IsDef, DFG);
+        llvm::for_each(Defs, AnalyzeDef);
+      }
----------------
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?


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

https://reviews.llvm.org/D75936