[cfe-commits] r105408 - in /cfe/trunk: lib/Sema/SemaExprCXX.cpp test/CXX/class.access/p4.cpp test/CodeGenCXX/throw-expression-dtor.cpp

[Eli Friedman]

On Thu, Jun 3, 2010 at 4:13 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Jun 3, 2010, at 1:39 PM, Eli Friedman wrote:
>
>> Author: efriedma
>> Date: Thu Jun  3 15:39:03 2010
>> New Revision: 105408
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=105408&view=rev
>> Log:
>> Make sure to check the accessibility of and mark the destructor for the
>> operand of a throw expression.  Fixes PR7281.
>
> Cool. One comment below.
>
>> Added:
>>    cfe/trunk/test/CodeGenCXX/throw-expression-dtor.cpp
>> Modified:
>>    cfe/trunk/lib/Sema/SemaExprCXX.cpp
>>    cfe/trunk/test/CXX/class.access/p4.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=105408&r1=105407&r2=105408&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Thu Jun  3 15:39:03 2010
>> @@ -458,11 +458,28 @@
>>     return true;
>>   E = Res.takeAs<Expr>();
>>
>> +  // If the exception has class type, we need additional handling.
>> +  const RecordType *RecordTy = Ty->getAs<RecordType>();
>> +  if (!RecordTy)
>> +    return false;
>> +  CXXRecordDecl *RD = cast<CXXRecordDecl>(RecordTy->getDecl());
>> +
>>   // If we are throwing a polymorphic class type or pointer thereof,
>>   // exception handling will make use of the vtable.
>> -  if (const RecordType *RecordTy = Ty->getAs<RecordType>())
>> -    MarkVTableUsed(ThrowLoc, cast<CXXRecordDecl>(RecordTy->getDecl()));
>> -
>> +  MarkVTableUsed(ThrowLoc, RD);
>> +
>> +  // If the class has a non-trivial destructor, we must be able to call it.
>> +  if (RD->hasTrivialDestructor())
>> +    return false;
>> +
>> +  CXXDestructorDecl *Destructor =
>> +    const_cast<CXXDestructorDecl*>(RD->getDestructor(Context));
>> +  if (!Destructor)
>> +    return false;
>> +
>> +  MarkDeclarationReferenced(E->getExprLoc(), Destructor);
>> +  CheckDestructorAccess(E->getExprLoc(), Destructor,
>> +                        PDiag(diag::err_access_dtor_temp) << Ty);
>>   return false;
>> }
>
> Could we get a new diagnostic that talks about the destructor of the exception object, rather than reusing err_access_dtor_temp?

Would something like "exception object of type 'test16::A' has private
destructor" be okay?

>>
>> Modified: cfe/trunk/test/CXX/class.access/p4.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/class.access/p4.cpp?rev=105408&r1=105407&r2=105408&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/CXX/class.access/p4.cpp (original)
>> +++ cfe/trunk/test/CXX/class.access/p4.cpp Thu Jun  3 15:39:03 2010
>> @@ -420,3 +420,9 @@
>>   template class B<int>;  // expected-note {{in instantiation}}
>>   template class B<long>; // expected-note 4 {{in instantiation}}
>> }
>> +
>> +// PR7281
>> +namespace test16 {
>> +  class A { ~A(); }; // expected-note {{declared private here}}
>> +  void b() { throw A(); } // expected-error{{temporary of type 'test16::A' has private destructor}}
>> +}
>>
>> Added: cfe/trunk/test/CodeGenCXX/throw-expression-dtor.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/throw-expression-dtor.cpp?rev=105408&view=auto
>> ==============================================================================
>> --- cfe/trunk/test/CodeGenCXX/throw-expression-dtor.cpp (added)
>> +++ cfe/trunk/test/CodeGenCXX/throw-expression-dtor.cpp Thu Jun  3 15:39:03 2010
>> @@ -0,0 +1,14 @@
>> +// RUN: %clang_cc1 %s -emit-llvm-only -verify
>> +// PR7281
>> +
>> +class A {
>> +public:
>> +    ~A();
>> +};
>> +class B : public A {
>> +    void ice_throw();
>> +};
>> +void B::ice_throw() {
>> +    throw *this;
>> +}
>> +
>
> I guess this is just testing that CodeGen doesn't crash, right? Could we check the generated IR to ensure that a pointer to the destructor is being put into the right place as part of the throw statement?

I think that's sufficiently covered by test/CodeGenCXX/eh.cpp, and I
don't really know what we should be generating well enough to verify
that it's working.

-Eli