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

Fri May 8 00:29:39 PDT 2020

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