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

[Douglas Gregor]

On Jun 3, 2010, at 4:25 PM, Eli Friedman wrote:

> 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.

Yes, I see it tested in eh.cpp. Thanks!

	- Doug