On Fri, Mar 6, 2015 at 10:04 AM, David Blaikie <[email protected]> wrote:
> > > On Fri, Mar 6, 2015 at 9:57 AM, Nico Weber <[email protected]> wrote: > >> On Fri, Mar 6, 2015 at 9:32 AM, David Blaikie <[email protected]> wrote: >> >>> >>> >>> On Thu, Mar 5, 2015 at 10:01 PM, Nico Weber <[email protected]> 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 > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
