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