msan: Handle musttail calls

[rnk]

Summary

First, avoid calling setTailCall(false) on musttail calls. The funciton
prototypes should be "congruent", so the shadow layout should be exactly
the same.

Second, avoid inserting instrumentation after a musttail call to
propagate the return value shadow. We don't need to propagate the
result of a tail call, it should already be in the right place.

[eugenis]

LGTM w/ nits

include/llvm/IR/Instructions.h

2418 ↗

(On Diff #10949)

Are you sure it should be part of ReturnInst interface?
Makes more sense as a utility function to me.
Anyway, this is an unrelated refactoring, please commit separately.

lib/Transforms/Instrumentation/MemorySanitizer.cpp

2280 ↗

(On Diff #10949)

I no longer understand what is this code trying to achieve. The comment suggests that there is a problem when caller returns void, and that's exactly the case you are enabling for musttail calls.

But this whole thing does not make sense, and I can't figure out what is it that tail call optimization can break. Perhaps it had something to do with the hybrid tool and had been made obsolete by us resetting retval shadow to 0 _before_ every call.

Anyway, the tests still pass if this chunk is removed. I suggest you just do that. If I'm missing something, we will know it soon enough.

[rnk]

include/llvm/IR/Instructions.h

2418 ↗

(On Diff #10949)

I agree, the IR classes are generally very lightweight and don't have these kinds of analysis helpers.

+chandlerc, WDYT? Is there a better place to put this?

lib/Transforms/Instrumentation/MemorySanitizer.cpp

2280 ↗

(On Diff #10949)

OK. :) I'll commit that as a separate change without any of the actual musttail handling stuff.

[rnk]

rebase

[rnk]

Closed by commit rL215415 (authored by @rnk).

llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp

void visitCallSite(CallSite CS) {
     DEBUG(dbgs() << "  done with call args\n");
 
     FunctionType *FT =
       cast<FunctionType>(CS.getCalledValue()->getType()->getContainedType(0));
     if (FT->isVarArg()) {
       VAHelper->visitCallSite(CS, IRB);
     }
 
+    // If this is a musttail call site, we can't insert propagation code here.
+    // The return type of the caller must match the callee, so the shadow should
+    // already be set up for an immediate return.
+    if (CS.isMustTailCall())
+      return;
+
     // Now, get the shadow for the RetVal.
     if (!I.getType()->isSized()) return;
     IRBuilder<> IRBBefore(&I);
     // Until we have full dynamic coverage, make sure the retval shadow is 0.
     Value *Base = getShadowPtrForRetval(&I, IRBBefore);
     IRBBefore.CreateAlignedStore(getCleanShadow(&I), Base, kShadowTLSAlignment);
     Instruction *NextInsn = nullptr;
     if (CS.isCall()) {
Value *RetvalShadow =
       IRBAfter.CreateAlignedLoad(getShadowPtrForRetval(&I, IRBAfter),
                                  kShadowTLSAlignment, "_msret");
     setShadow(&I, RetvalShadow);
     if (MS.TrackOrigins)
       setOrigin(&I, IRBAfter.CreateLoad(getOriginPtrForRetval(IRBAfter)));
   }
 
   void visitReturnInst(ReturnInst &I) {
+    // Don't propagate shadow between musttail calls and the return.
+    if (I.getParent()->getTerminatingMustTailCall())
+      return;
+
     IRBuilder<> IRB(&I);
     Value *RetVal = I.getReturnValue();
     if (!RetVal) return;
     Value *ShadowPtr = getShadowPtrForRetval(RetVal, IRB);
     if (CheckReturnValue) {
       insertShadowCheck(RetVal, &I);
       Value *Shadow = getCleanShadow(RetVal);
       IRB.CreateAlignedStore(Shadow, ShadowPtr, kShadowTLSAlignment);

llvm/trunk/test/Instrumentation/MemorySanitizer/msan_basic.ll

 }
 
 ; We used to strip off the 'tail' modifier, but now that we unpoison return slot
 ; shadow before the call, we don't need to anymore.
 
 ; CHECK-LABEL: define void @MismatchedReturnTypeTailCall
 ; CHECK: tail call i32 @InnerTailCall
 ; CHECK: ret void
+
+declare i32 @InnerMustTailCall(i32 %a)
+
+define i32 @MustTailCall(i32 %a) {
+  %b = musttail call i32 @InnerMustTailCall(i32 %a)
+  ret i32 %b
+}
+
+; Test that 'musttail' is preserved.  The ABI should make this work.
+
+; CHECK-LABEL: define i32 @MustTailCall
+; CHECK: musttail call i32 @InnerMustTailCall
+; CHECK-NEXT: ret i32