[PATCH] D63158: StackProtector: Use PointerMayBeCaptured

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: lib/CodeGen/StackProtector.cpp:267
 
-        if (Strong && HasAddressTaken(AI)) {
+        if (Strong && PointerMayBeCaptured(AI, true, true)) {
           ++NumAddrTaken;
----------------
arsenm wrote:
> jdoerfert wrote:
> > Are we sure we want return to capture?
> > 
> > And could you add the `/* StoreCaptures */` comments so ppl know what the booleans are.
> I don't know, I just figured it should be more conservative
I see. My reasoning was: The original code new about the existence of return and it did not consider it as "address taken" so, ... 


================
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
----------------
arsenm wrote:
> jdoerfert wrote:
> > 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)?
> I think the interpretation of volatile capturing the address is absurd, but I don't care enough to fight that battle. The volatiles were irrelevant for the test here
Fair point.


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

https://reviews.llvm.org/D63158