mattdr accepted this revision.
mattdr marked 2 inline comments as done.
mattdr added a comment.
The extra comments and the new variable name are all helpful. Thanks again.
================
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
+
----------------
sconstab wrote:
> 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?
Much better. Thank you for the change!
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:341
+ if (UseMI.mayLoad())
+ continue; // Found a transmitting load, stop traversing defs
+ }
----------------
sconstab wrote:
> 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.
I definitely misread `continue` as `break` here. Thanks for the extra clarity
and sorry for the noise.
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
