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

[Nico Weber]

On Fri, Mar 6, 2015 at 10:04 AM, David Blaikie <dblaikie at gmail.com> wrote:

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

Ah sorry, it was on the bug, not in the commit message:
http://llvm.org/bugs/show_bug.cgi?id=22793#c5


>
>
>> 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/d6607ad3/attachment.html>