[objc] Don't require null-check and don't emit memset when result is ignored for struct-returning method calls

[kubamracek]

Summary

There's currently an issue with the emission of lifetime markers for struct-returning Obj-C msgSend calls. The following code triggers an ASan use-after-scope report:

MyClass *myNil = nil;  // intentionally nil
[myNil methodWhichReturnsARect];

The reason is that when the result of a struct-returning call is ignore, the temporary storage is only marked with lifetime markers in one of the two branches of the nil-receiver-check. The check is, however, not required when the result is unused. If we still need to emit the check (due to consumer arguments), let's not emit the memset to zero out the result if it's unused. This fixes the use-after-scope problem.

[rjmccall]

Please include an IRGen test directly for the lifetime marker emission you want, and please make sure you test the ns_consumed argument + ignored stret result case.

tools/clang/lib/CodeGen/CGObjCMac.cpp

2147

Please add a comment here, something like "We don't need to emit a null check to zero out an indirect result if the result is ignored."

[kubamracek]

Adding a test, updating the comment.

[rjmccall]

Thanks, looks great.

projects/compiler-rt/test/asan/TestCases/Darwin/nil-return-struct.mm

+// RUN: %clang_asan %s -o %t -framework Foundation
+// RUN: %run %t 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+struct MyStruct {
+  long a, b, c, d;
+};
+
+@interface MyClass: NSObject
+- (MyStruct)methodWhichReturnsARect;
+@end
+@implementation MyClass
+- (MyStruct)methodWhichReturnsARect {
+  MyStruct s;
+  s.a = 10;
+  s.b = 20;
+  s.c = 30;
+  s.d = 40;
+  return s;
+}
+@end
+
+int main() {
+  MyClass *myNil = nil;  // intentionally nil
+  [myNil methodWhichReturnsARect];
+  fprintf(stderr, "Hello world");
+}
+
+// CHECK-NOT: AddressSanitizer: stack-use-after-scope
+// CHECK: Hello world

tools/clang/lib/CodeGen/CGObjCMac.cpp

void init(CodeGenFunction &CGF, llvm::Value *receiver) {
     CGF.Builder.CreateCondBr(isNull, NullBB, callBB);
 
     // Otherwise, start performing the call.
     CGF.EmitBlock(callBB);
   }
 
   /// Complete the null-return operation.  It is valid to call this
   /// regardless of whether 'init' has been called.
-  RValue complete(CodeGenFunction &CGF, RValue result, QualType resultType,
+  RValue complete(CodeGenFunction &CGF,
+                  ReturnValueSlot returnSlot,
+                  RValue result,
+                  QualType resultType,
                   const CallArgList &CallArgs,
                   const ObjCMethodDecl *Method) {
     // If we never had to do a null-check, just use the raw result.
     if (!NullBB) return result;
 
     // The continuation block.  This will be left null if we don't have an
     // IP, which can happen if the method we're calling is marked noreturn.
     llvm::BasicBlock *contBB = nullptr;
RValue complete(CodeGenFunction &CGF,
     }
 
     // If we've got an aggregate return, null the buffer out.
     // FIXME: maybe we should be doing things differently for all the
     // cases where the ABI has us returning (1) non-agg values in
     // memory or (2) agg values in registers.
     if (result.isAggregate()) {
       assert(result.isAggregate() && "null init of non-aggregate result?");
+      if (!returnSlot.isUnused())
         CGF.EmitNullInitialization(result.getAggregateAddress(), resultType);
       if (contBB) CGF.EmitBlock(contBB);
       return result;
     }
 
     // Complex types.
     CGF.EmitBlock(contBB);
     CodeGenFunction::ComplexPairTy callResult = result.getComplexVal();
 
if (Self->getType().isConstQualified()) {
         llvm::Value *SelfAddr = CGF.GetAddrOfLocalVar(Self).getPointer();
         if (SelfAddr == LI->getPointerOperand()) {
           ReceiverCanBeNull = false;
         }
       }
     }
   }
 
-  NullReturnState nullReturn;
+  bool RequiresNullCheck = false;
 
   llvm::Constant *Fn = nullptr;
   if (CGM.ReturnSlotInterferesWithArgs(MSI.CallInfo)) {
-    if (ReceiverCanBeNull) nullReturn.init(CGF, Arg0);
+    if (ReceiverCanBeNull) RequiresNullCheck = true;
     Fn = (ObjCABI == 2) ?  ObjCTypes.getSendStretFn2(IsSuper)
       : ObjCTypes.getSendStretFn(IsSuper);
   } else if (CGM.ReturnTypeUsesFPRet(ResultType)) {
     Fn = (ObjCABI == 2) ? ObjCTypes.getSendFpretFn2(IsSuper)
       : ObjCTypes.getSendFpretFn(IsSuper);
   } else if (CGM.ReturnTypeUsesFP2Ret(ResultType)) {
     Fn = (ObjCABI == 2) ? ObjCTypes.getSendFp2RetFn2(IsSuper)
       : ObjCTypes.getSendFp2retFn(IsSuper);
   } else {
     // arm64 uses objc_msgSend for stret methods and yet null receiver check
     // must be made for it.
     if (ReceiverCanBeNull && CGM.ReturnTypeUsesSRet(MSI.CallInfo))
-      nullReturn.init(CGF, Arg0);
+      RequiresNullCheck = true;
     Fn = (ObjCABI == 2) ? ObjCTypes.getSendFn2(IsSuper)
       : ObjCTypes.getSendFn(IsSuper);
   }
 
+  if (Return.isUnused())
+    RequiresNullCheck = false;
+
   // Emit a null-check if there's a consumed argument other than the receiver.
-  bool RequiresNullCheck = false;
-  if (ReceiverCanBeNull && CGM.getLangOpts().ObjCAutoRefCount && Method) {
+  if (!RequiresNullCheck && CGM.getLangOpts().ObjCAutoRefCount && Method) {
     for (const auto *ParamDecl : Method->parameters()) {
       if (ParamDecl->hasAttr<NSConsumedAttr>()) {
-        if (!nullReturn.NullBB)
-          nullReturn.init(CGF, Arg0);
         RequiresNullCheck = true;
         break;
       }
     }
   }
-  
+  NullReturnState nullReturn;
+  if (RequiresNullCheck) {
+    nullReturn.init(CGF, Arg0);
+  }
+  
   llvm::Instruction *CallSite;
   Fn = llvm::ConstantExpr::getBitCast(Fn, MSI.MessengerType);
   CGCallee Callee = CGCallee::forDirect(Fn);
   RValue rvalue = CGF.EmitCall(MSI.CallInfo, Callee, Return, ActualArgs,
                                &CallSite);
 
   // Mark the call as noreturn if the method is marked noreturn and the
   // receiver cannot be null.
   if (Method && Method->hasAttr<NoReturnAttr>() && !ReceiverCanBeNull) {
     llvm::CallSite(CallSite).setDoesNotReturn();
   }
 
-  return nullReturn.complete(CGF, rvalue, ResultType, CallArgs,
+  return nullReturn.complete(CGF, Return, rvalue, ResultType, CallArgs,
                              RequiresNullCheck ? Method : nullptr);
 }
 
 static Qualifiers::GC GetGCAttrTypeForType(ASTContext &Ctx, QualType FQT,
                                            bool pointee = false) {
   // Note that GC qualification applies recursively to C pointer types
   // that aren't otherwise decorated.  This is weird, but it's probably
   // an intentional workaround to the unreliable placement of GC qualifiers.
CGObjCNonFragileABIMac::EmitVTableMessageSend(CodeGenFunction &CGF,
   Address calleeAddr =
       CGF.Builder.CreateStructGEP(mref, 0, CharUnits::Zero());
   llvm::Value *calleePtr = CGF.Builder.CreateLoad(calleeAddr, "msgSend_fn");
 
   calleePtr = CGF.Builder.CreateBitCast(calleePtr, MSI.MessengerType);
   CGCallee callee(CGCalleeInfo(), calleePtr);
 
   RValue result = CGF.EmitCall(MSI.CallInfo, callee, returnSlot, args);
-  return nullReturn.complete(CGF, result, resultType, formalArgs,
+  return nullReturn.complete(CGF, returnSlot, result, resultType, formalArgs,
                              requiresnullCheck ? method : nullptr);
 }
 
 /// Generate code for a message send expression in the nonfragile abi.
 CodeGen::RValue
 CGObjCNonFragileABIMac::GenerateMessageSend(CodeGen::CodeGenFunction &CGF,
                                             ReturnValueSlot Return,
                                             QualType ResultType,

tools/clang/test/CodeGenObjC/stret-1.m

 // RUN: %clang_cc1 -fblocks -triple arm64-apple-darwin %s -emit-llvm -o - | FileCheck %s
 // rdar://12416433
 
 struct stret { int x[100]; };
 struct stret one = {{1}};
 
 @interface Test  @end
 
 @implementation Test
 +(struct stret) method { return one; }
 @end
 
 int main(int argc, const char **argv)
 {
-    [(id)(argc&~255) method];
+    struct stret s;
+    s = [(id)(argc&~255) method];
     // CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (%struct.stret*, i8*, i8*)*)(%struct.stret* sret [[T0:%[^,]+]]
     // CHECK: [[T0P:%.*]] = bitcast %struct.stret* [[T0]] to i8*
     // CHECK: call void @llvm.memset.p0i8.i64(i8* [[T0P]], i8 0, i64 400, i32 4, i1 false)
 
+    s = [Test method];
+    // CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (%struct.stret*, i8*, i8*)*)(%struct.stret* sret [[T1:%[^,]+]]
+    // CHECK-NOT: call void @llvm.memset.p0i8.i64(
+
+    [(id)(argc&~255) method];
+    // CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (%struct.stret*, i8*, i8*)*)(%struct.stret* sret [[T1:%[^,]+]]
+    // CHECK-NOT: call void @llvm.memset.p0i8.i64(
+
     [Test method];
     // CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (%struct.stret*, i8*, i8*)*)(%struct.stret* sret [[T1:%[^,]+]]
     // CHECK-NOT: call void @llvm.memset.p0i8.i64(
 }