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

[Matthew Riley via Phabricator via cfe-commits]

mattdr added a comment.

Calling special attention to the comment at line 341, since I think it affects the correctness of the pass.



================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:287
+  // The `Transmitters` map memoizes transmitters found for each def. If a def
+  // has not yet been analyzed, then its list of transmitters will be empty.
+  DenseMap<NodeId, std::vector<NodeId>> Transmitters;
----------------
This comment doesn't seem to match how the map is used -- it looks like the loop assumes a def has been analyzed iff it is present in the map. This matches my expectation that, if a def is present and maps to an empty list, it would meant the def **had** been analyzed and found not to transmit.


================
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())
----------------
fwiw, this code would be easier to understand if we didn't shadow `Def` with another variable named `Def`.


================
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
+
----------------
"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.


================
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())
----------------
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



================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:341
+              if (UseMI.mayLoad())
+                continue; // Found a transmitting load, stop traversing defs
+            }
----------------
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.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:364-365
+
+          // Remove duplicate transmitters
+          auto &DefTransmitters = Transmitters[Def.Id];
+          llvm::sort(DefTransmitters);
----------------
This is also the place we populate `Transmitters` (with a default-constructed vector) for the current def if we haven't otherwise found any transmits. That's good, and necessary for `Transmitters` to remember we've analyzed the current def. But we should leave a comment about this subtle load-bearing side-effect.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:366-369
+          llvm::sort(DefTransmitters);
+          DefTransmitters.erase(
+              std::unique(DefTransmitters.begin(), DefTransmitters.end()),
+              DefTransmitters.end());
----------------
Should `Transmitters` map to an `llvm::SmallSet`?


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

https://reviews.llvm.org/D75936