r231451 - Don't crash on non-public referenced dtors in toplevel classes.

[David Blaikie]

On Fri, Mar 6, 2015 at 9:57 AM, Nico Weber <thakis at chromium.org> wrote:

> On Fri, Mar 6, 2015 at 9:32 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Thu, Mar 5, 2015 at 10:01 PM, Nico Weber <nicolasweber at gmx.de> wrote:
>>
>>> Author: nico
>>> Date: Fri Mar  6 00:01:06 2015
>>> New Revision: 231451
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=231451&view=rev
>>> Log:
>>> Don't crash on non-public referenced dtors in toplevel classes.
>>>
>>> Fixes PR22793, a bug that caused self-hosting to fail after the innocuous
>>> r231254. See the bug for details.
>>>
>>
>> Awesome - thanks for jumping on this!
>>
>>
>>>
>>> Modified:
>>>     cfe/trunk/lib/Sema/SemaExpr.cpp
>>>     cfe/trunk/test/CodeGenCXX/trivial-constructor-init.cpp
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=231451&r1=231450&r2=231451&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Fri Mar  6 00:01:06 2015
>>> @@ -117,7 +117,7 @@ static AvailabilityResult DiagnoseAvaila
>>>      case AR_Available:
>>>      case AR_NotYetIntroduced:
>>>        break;
>>> -
>>> +
>>>      case AR_Deprecated:
>>>        if (S.getCurContextAvailability() != AR_Deprecated)
>>>          S.EmitAvailabilityWarning(Sema::AD_Deprecation,
>>> @@ -11859,8 +11859,11 @@ void Sema::MarkFunctionReferenced(Source
>>>    } else if (CXXDestructorDecl *Destructor =
>>>                   dyn_cast<CXXDestructorDecl>(Func)) {
>>>      Destructor = cast<CXXDestructorDecl>(Destructor->getFirstDecl());
>>> -    if (Destructor->isDefaulted() && !Destructor->isDeleted())
>>> +    if (Destructor->isDefaulted() && !Destructor->isDeleted()) {
>>> +      if (Destructor->isTrivial() &&
>>> !Destructor->hasAttr<DLLExportAttr>())
>>> +        return;
>>>        DefineImplicitDestructor(Loc, Destructor);
>>> +    }
>>>      if (Destructor->isVirtual() && getLangOpts().AppleKext)
>>>        MarkVTableUsed(Loc, Destructor->getParent());
>>>    } else if (CXXMethodDecl *MethodDecl = dyn_cast<CXXMethodDecl>(Func))
>>> {
>>>
>>> Modified: cfe/trunk/test/CodeGenCXX/trivial-constructor-init.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/trivial-constructor-init.cpp?rev=231451&r1=231450&r2=231451&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/test/CodeGenCXX/trivial-constructor-init.cpp (original)
>>> +++ cfe/trunk/test/CodeGenCXX/trivial-constructor-init.cpp Fri Mar  6
>>> 00:01:06 2015
>>> @@ -32,3 +32,17 @@ static C c[4];
>>>
>>>  int main() {
>>>  }
>>> +
>>> +namespace PR22793 {
>>> +template <typename>
>>> +struct foo {
>>> +protected:
>>>
>>
>> Naively, I was hoping a/the fix might resolve around this being
>> protected. I was sort of hoping that the fix might involve /removing/ code
>> that was needlessly checking access rather than adding more conditions.
>> Seems strange that the access level could change the behavior of this code.
>> (but I haven't fully understood the notes you made on the bug - so I
>> imagine it's probably described in there somewhere) Any chance of that?
>>
>
> Public defaulted dtors are marked as "irrelevant destructors", and that
> triggers an early return somewhere. If a destructor is not public, it isn't
> irrelevant to sema though, as it now needs access checks.
>

Ah, I see - just an optimization, then. Thanks for the context.


> So the early return that "protects" irrelevant destructors doesn't fire
> for this case, and we need to explicitly check for triviality. As the
> commit message says,
>

Maybe a detail missed from the commit message? I don't see that detail
mentioned.


> constructors have the same check a few lines further up.
>

Ah, looking at the code I see - fair enough then.

Thanks for the details!

- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150306/9d62ce29/attachment.html>