Fix crash in ObjC codegen introduced with 5ab6ee75994d645725264e757d67bbb1c96fb2b6

[theraven]

Summary

5ab6ee75994d645725264e757d67bbb1c96fb2b6 assumed that if RValue::isScalar() returns true then RValue::getScalarVal will return a valid value. This is not the case when the return value is void and so void message returns would crash if they hit this path. This is triggered only for cases where the nil-handling path needs to do something non-trivial (destroy arguments that should be consumed by the callee).

[rjmccall]

LGTM. Could you add a test case?

[theraven]

I'd like to. The test case from the original bug report was very large but I'm not sure how to trigger this with anything comprehensibly small. You need something where the callee is responsible for destroying the argument and I'm not sure what combination of properties / ABIs results in that. The original test was -(void)foo:(std::string)aString on the MSVC ABI, but I don't know how much of the Visual Studio std::string implementation is necessary to trigger this behaviour. Is it just a non-trivial destructor?

[rjmccall]

In D123898#3455933, @theraven wrote:

I'd like to. The test case from the original bug report was very large but I'm not sure how to trigger this with anything comprehensibly small. You need something where the callee is responsible for destroying the argument and I'm not sure what combination of properties / ABIs results in that. The original test was -(void)foo:(std::string)aString on the MSVC ABI, but I don't know how much of the Visual Studio std::string implementation is necessary to trigger this behaviour. Is it just a non-trivial destructor?

Yeah, a non-trivial destructor should be good enough. Or a consumed ObjC pointer argument in ARC might also work, although I don't know if that takes the same path.

[triplef]

Thanks, LGTM!

I tested these changes with our app’s code base, which was triggering the bug in a couple of places, and it all builds fine with the patch.

clang/lib/CodeGen/CGObjCGNU.cpp

     // Enter the continuation block and emit a phi if required.
     CGF.EmitBlock(continueBB);
     if (msgRet.isScalar()) {
-      llvm::Value *v = msgRet.getScalarVal();
-      llvm::PHINode *phi = Builder.CreatePHI(v->getType(), 2);
-      phi->addIncoming(v, nonNilPathBB);
-      phi->addIncoming(CGM.EmitNullConstant(ResultType), nilPathBB);
-      msgRet = RValue::get(phi);
+      // If the return type is void, do nothing
+      if (llvm::Value *v = msgRet.getScalarVal()) {
+        llvm::PHINode *phi = Builder.CreatePHI(v->getType(), 2);
+        phi->addIncoming(v, nonNilPathBB);
+        phi->addIncoming(CGM.EmitNullConstant(ResultType), nilPathBB);
+        msgRet = RValue::get(phi);
+      }
     } else if (msgRet.isAggregate()) {
       // Aggregate zeroing is handled in nilCleanupBB when it's required.
     } else /* isComplex() */ {