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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
