[PATCH] D16914: [CodeGen] Fix an assert in CodeGenFunction::EmitFunctionEpilog

[Akira Hatanaka via cfe-commits]

I agree with you, that was actually what John suggested too.

I'll update the patch shortly.

On Fri, Feb 5, 2016 at 3:38 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2016-Feb-04, at 18:54, Akira Hatanaka via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
> >
> > ahatanak created this revision.
> > ahatanak added a subscriber: cfe-commits.
> >
> > The assert near CGCall.cpp:2465 is triggered because
> QualType::isObjCRetainableType() is called on different types. In
> CodeGenFunction::StartFunction, it returns true because it is called on the
> typedef marked with __attribute__((NSObject)), whereas in
> CodeGenFunction::EmitFunctionEpilog it returns false because it's called on
> the canonical type.
> >
> > To fix the assert, this patch changes the code in
> CodeGenFunction::EmitFunctionEpilog to get the function's return type from
> CodeGenFunction::CurCodeDecl or BlockInfo instead of from CGFunctionInfo.
> >
> > http://reviews.llvm.org/D16914
>
> I have a nitpick below.  (I think you should find someone that knows
> this code better than I do to sign off on this though.)
>
> > Files:
> >  lib/CodeGen/CGCall.cpp
> >  test/CodeGenObjCXX/auto-release-result-assert.mm
> >
> > Index: test/CodeGenObjCXX/auto-release-result-assert.mm
> > ===================================================================
> > --- /dev/null
> > +++ test/CodeGenObjCXX/auto-release-result-assert.mm
> > @@ -0,0 +1,35 @@
> > +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -fblocks
> -fobjc-arc -o - %s | FileCheck %s
> > +
> > +// CHECK-LABEL: define %struct.S1* @_Z4foo1i(
> > +// CHECK: %[[CALL:[a-z0-9]+]] = call %struct.S1* @_Z4foo0i
> > +// CHECK: ret %struct.S1* %[[CALL]]
> > +
> > +// CHECK-LABEL: define %struct.S1* @_ZN2S22m1Ev(
> > +// CHECK: %[[CALL:[a-z0-9]+]] = call %struct.S1* @_Z4foo0i
> > +// CHECK: ret %struct.S1* %[[CALL]]
> > +
> > +// CHECK-LABEL: define internal %struct.S1* @Block1_block_invoke(
> > +// CHECK: %[[CALL:[a-z0-9]+]] = call %struct.S1* @_Z4foo0i
> > +// CHECK: ret %struct.S1* %[[CALL]]
> > +
> > +struct S1;
> > +
> > +typedef __attribute__((NSObject)) struct
> __attribute__((objc_bridge(id))) S1 * S1Ref;
> > +
> > +S1Ref foo0(int);
> > +
> > +struct S2 {
> > +  S1Ref m1();
> > +};
> > +
> > +S1Ref foo1(int a) {
> > +  return foo0(a);
> > +}
> > +
> > +S1Ref S2::m1() {
> > +  return foo0(0);
> > +}
> > +
> > +S1Ref (^Block1)(void) = ^{
> > +  return foo0(0);
> > +};
> > Index: lib/CodeGen/CGCall.cpp
> > ===================================================================
> > --- lib/CodeGen/CGCall.cpp
> > +++ lib/CodeGen/CGCall.cpp
> > @@ -14,6 +14,7 @@
> >
> > #include "CGCall.h"
> > #include "ABIInfo.h"
> > +#include "CGBlocks.h"
> > #include "CGCXXABI.h"
> > #include "CGCleanup.h"
> > #include "CodeGenFunction.h"
> > @@ -2462,9 +2463,21 @@
> >     // In ARC, end functions that return a retainable type with a call
> >     // to objc_autoreleaseReturnValue.
> >     if (AutoreleaseResult) {
> > +      QualType RT;
> > +
> > +      if (auto *FD = dyn_cast<FunctionDecl>(CurCodeDecl))
> > +        RT = FD->getReturnType();
> > +      else if (auto *MD = dyn_cast<ObjCMethodDecl>(CurCodeDecl))
> > +        RT = MD->getReturnType();
> > +      else if (isa<BlockDecl>(CurCodeDecl))
> > +        RT =
> BlockInfo->BlockExpression->getFunctionType()->getReturnType();
> > +      else
> > +        llvm_unreachable("Unexpected function/method type");
>
> There's no reason for this code to run when NDEBUG, is there?
>
> > +
> > +      (void)RT;
> >       assert(getLangOpts().ObjCAutoRefCount &&
> >              !FI.isReturnsRetained() &&
> > -             RetTy->isObjCRetainableType());
> > +             RT->isObjCRetainableType());
>
> I think you should change this to:
> ```
> #ifndef NDEBUG
> QualType RT;
> if (...)
>    RT = ...
> else if (...)
>    RT = ...
> else if (...)
>    RT = ...
> else
>    llvm_unreachable(...);
> assert(...);
> #endif
> ```
>
> >       RV = emitAutoreleaseOfResult(*this, RV);
> >     }
> >
> >
> >
> > <D16914.46990.patch>_______________________________________________
> > cfe-commits mailing list
> > cfe-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160205/2f397435/attachment.html>