[PATCH] D63158: StackProtector: Use PointerMayBeCaptured

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Generally fine, three smaller comments though.



================
Comment at: lib/CodeGen/StackProtector.cpp:179
-      // they are only visited once.
-      if (VisitedPHIs.insert(PN).second)
-        if (HasAddressTaken(PN))
----------------
VisitedPHIs is not needed anymore.


================
Comment at: lib/CodeGen/StackProtector.cpp:267
 
-        if (Strong && HasAddressTaken(AI)) {
+        if (Strong && PointerMayBeCaptured(AI, true, true)) {
           ++NumAddrTaken;
----------------
Are we sure we want return to capture?

And could you add the `/* StoreCaptures */` comments so ppl know what the booleans are.


================
Comment at: test/CodeGen/X86/stack-protector.ll:4091
+  store i32 1, i32* %1, align 4
+  %3 = load i32, i32* %1, align 4
   %4 = mul nsw i32 %3, 42
----------------
Was the problem we assume volatile stores cause the addrs to escape (=be captured)?
If so, can we add a comment, maybe even a negative test (with the volatile accesses)?


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

https://reviews.llvm.org/D63158