[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 cfe-commits]

sconstab added inline comments.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:295
+    std::function<void(NodeAddr<DefNode *>)> AnalyzeDefUseChain =
+        [&](NodeAddr<DefNode *> Def) {
+          if (Transmitters.find(Def.Id) != Transmitters.end())
----------------
mattdr wrote:
> fwiw, this code would be easier to understand if we didn't shadow `Def` with another variable named `Def`.
Changed the outer def to `SourceDef`, which also seems to make the code after the lambda a lot clearer.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:321-322
+          for (auto UseID : Uses) {
+            if (!UsesVisited.insert(UseID).second)
+              continue; // Already visited this use of the current def
+
----------------
mattdr wrote:
> "current def" is a bit ambiguous here. I _believe_ it means `AnalyzeDef`'s `Def` argument? At least, that's the interpretation that makes the comment make sense since `UsesVisited` is in `AnalyzeDef`'s scope.
I am now trying to be clearer by using capital-d "Def" to refer specifically to the def that is being analyzed, and lower-case-d "def" to refer to any other defs. Do you think this is better? Good enough?


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:331
+            // We naively assume that an instruction propagates any loaded
+            // Uses to all Defs, unless the instruction is a call.
+            if (UseMI.isCall())
----------------
mattdr wrote:
> Copying a comment from a previous iteration:
> 
> > Why is it okay to assume that a call doesn't propagate its uses to defs? Is it because we can assume the CFI transform is already inserting an LFENCE? Whatever the reason, let's state it explicitly here
> 
Added clarification to the comment.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:341
+              if (UseMI.mayLoad())
+                continue; // Found a transmitting load, stop traversing defs
+            }
----------------
mattdr wrote:
> The comment doesn't match the loop, which is traversing over `Uses`. More importantly, though: why are we allowed to stop traversing through `Uses` here? This `Def` won't be analyzed again, so this is our only chance to enumerate all transmitters to make sure we have all the necessary source -> sink edges in the gadget graph.
@mattdr I think that the code is correct, and I added more to the comment in an attempt to clarify. Let me know if you still think that this is an issue.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:366-369
+          llvm::sort(DefTransmitters);
+          DefTransmitters.erase(
+              std::unique(DefTransmitters.begin(), DefTransmitters.end()),
+              DefTransmitters.end());
----------------
mattdr wrote:
> Should `Transmitters` map to an `llvm::SmallSet`?
In my testing, `std::vector` seems a bit faster than `llvm::SmallSet`. I also suspect that `llvm::SmallSet` may waste more space because many defs will have no transmitters.


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

https://reviews.llvm.org/D75936