[MemCpyOpt] Be conservative in the face of returns_twice calls

[majnemer]

Summary

It is not safe to forward loads and stores around if calls to setjmp are
present: there might be a call which longjmps causing a memory location
to be initialized with an unexpected value.

This fixes PR27848.

[eli.friedman]

lib/Transforms/Scalar/MemCpyOptimizer.cpp

1381 ↗

(On Diff #58191)

I don't think all the optimizations in this pass are affected; memset merging seems pretty safe. That said, probably not worth the trouble, given how rare functions which call setjmp are.

[rnk]

I think the right fix is declare that volatile loads and stores "capture" their pointer operands, or to tease out some other function attribute like that. Your reduced test case from IRC shows how we incorrectly redirect volatile stores from y to x: https://ghostbin.com/paste/v5n3f

[majnemer]

Move this change over to functionattrs.

[majnemer]

• Update the LangRef.

[rnk]

lgtm, but let's bug Sanjoy, the person who seems to think the hardest about IPO function attrs right now.

[sanjoy]

I'm fine being bugged, but I think this review is missing some context -- how is "volatile stores escape the location being stored to" related to how we handle setjmp / longjmp?

A explicit test case on why we need this new rule will also be helpful (if not as a test case, then an example in the commit message), since @test_volatile only checks that we implement the rule correctly, but does not explain what the motivation is.

[majnemer]

In D20555#440527, @sanjoy wrote:

I'm fine being bugged, but I think this review is missing some context -- how is "volatile stores escape the location being stored to" related to how we handle setjmp / longjmp?

We substituted the underlying storage for a pointer when calling some function, that function would later perform a volatile store to that pointer. This is important because setjmp and longjmp require any modifications to objects which are loaded after the longjmp to be performed with volatile accesses. This all falls apart if the wrong underlying object is stored to.

A explicit test case on why we need this new rule will also be helpful (if not as a test case, then an example in the commit message), since @test_volatile only checks that we implement the rule correctly, but does not explain what the motivation is.

llvm/trunk/docs/LangRef.rst

``noalias``
     when used on function arguments. On function return values, the ``noalias``
     attribute indicates that the function acts like a system memory allocation
     function, returning a pointer to allocated storage disjoint from the
     storage for any other object accessible to the caller.
 
 ``nocapture``
     This indicates that the callee does not make any copies of the
     pointer that outlive the callee itself. This is not a valid
-    attribute for return values.
+    attribute for return values.  Addresses used in volatile operations
+    are considered to be captured.
 
 .. _nest:
 
 ``nest``
     This indicates that the pointer parameter can be excised using the
     :ref:`trampoline intrinsics <int_trampoline>`. This is not a valid
     attribute for return values and can only be applied to one parameter.
 

llvm/trunk/lib/Analysis/CaptureTracking.cpp

 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/CFG.h"
 #include "llvm/Analysis/CaptureTracking.h"
 #include "llvm/Analysis/OrderedBasicBlock.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
 
 using namespace llvm;
 
 CaptureTracker::~CaptureTracker() {}
 
 bool CaptureTracker::shouldExplore(const Use *U) { return true; }
 
 namespace {
while (!Worklist.empty()) {
     case Instruction::Invoke: {
       CallSite CS(I);
       // Not captured if the callee is readonly, doesn't return a copy through
       // its return value and doesn't unwind (a readonly function can leak bits
       // by throwing an exception or not depending on the input value).
       if (CS.onlyReadsMemory() && CS.doesNotThrow() && I->getType()->isVoidTy())
         break;
 
+      // Volatile operations effectively capture the memory location that they
+      // load and store to.
+      if (auto *MI = dyn_cast<MemIntrinsic>(I))
+        if (MI->isVolatile())
+          if (Tracker->captured(U))
+            return;
+
       // Not captured if only passed via 'nocapture' arguments.  Note that
       // calling a function pointer does not in itself cause the pointer to
       // be captured.  This is a subtle point considering that (for example)
       // the callee might return its own address.  It is analogous to saying
       // that loading a value from a pointer does not cause the pointer to be
       // captured, even though the loaded value might be the pointer itself
       // (think of self-referential objects).
       CallSite::data_operand_iterator B =
         CS.data_operands_begin(), E = CS.data_operands_end();
       for (CallSite::data_operand_iterator A = B; A != E; ++A)
         if (A->get() == V && !CS.doesNotCapture(A - B))
           // The parameter is not marked 'nocapture' - captured.
           if (Tracker->captured(U))
             return;
       break;
     }
     case Instruction::Load:
-      // Loading from a pointer does not cause it to be captured.
+      // Volatile loads make the address observable.
+      if (cast<LoadInst>(I)->isVolatile())
+        if (Tracker->captured(U))
+          return;
       break;
     case Instruction::VAArg:
       // "va-arg" from a pointer does not cause it to be captured.
       break;
     case Instruction::Store:
-      if (V == I->getOperand(0))
         // Stored the pointer - conservatively assume it may be captured.
+        // Volatile stores make the address observable.
+      if (V == I->getOperand(0) || cast<StoreInst>(I)->isVolatile())
         if (Tracker->captured(U))
           return;
-      // Storing to the pointee does not cause the pointer to be captured.
       break;
-    case Instruction::AtomicRMW:
-    case Instruction::AtomicCmpXchg:
-      // atomicrmw and cmpxchg conceptually include both a load and store from
-      // the same location.  As with a store, the location being accessed is
-      // not captured, but the value being stored is.  (For cmpxchg, we
-      // probably don't need to capture the original comparison value, but for
-      // the moment, let's be conservative.)
-      if (V != I->getOperand(0))
+    case Instruction::AtomicRMW: {
+      // atomicrmw conceptually includes both a load and store from
+      // the same location.
+      // As with a store, the location being accessed is not captured,
+      // but the value being stored is.
+      // Volatile stores make the address observable.
+      auto *ARMWI = cast<AtomicRMWInst>(I);
+      if (ARMWI->getValOperand() == V || ARMWI->isVolatile())
         if (Tracker->captured(U))
           return;
       break;
+    }
+    case Instruction::AtomicCmpXchg: {
+      // cmpxchg conceptually includes both a load and store from
+      // the same location.
+      // As with a store, the location being accessed is not captured,
+      // but the value being stored is.
+      // Volatile stores make the address observable.
+      auto *ACXI = cast<AtomicCmpXchgInst>(I);
+      if (ACXI->getCompareOperand() == V || ACXI->getNewValOperand() == V ||
+          ACXI->isVolatile())
+        if (Tracker->captured(U))
+          return;
+      break;
+    }
     case Instruction::BitCast:
     case Instruction::GetElementPtr:
     case Instruction::PHI:
     case Instruction::Select:
     case Instruction::AddrSpaceCast:
       // The original value is not captured via this if the new value isn't.
       Count = 0;
       for (Use &UU : I->uses()) {

llvm/trunk/test/Transforms/FunctionAttrs/nocapture.ll

define void @test_cmpxchg_ptr(i32** %p, i32* %q) {
   ret void
 }
 
 ; CHECK: define void @test_atomicrmw(i32* nocapture %p)
 define void @test_atomicrmw(i32* %p) {
   atomicrmw add i32* %p, i32 1 seq_cst
   ret void
 }
+
+; CHECK: define void @test_volatile(i32* %x)
+define void @test_volatile(i32* %x) {
+entry:
+  %gep = getelementptr i32, i32* %x, i64 1
+  store volatile i32 0, i32* %gep, align 4
+  ret void
+}