Re: r324062 - [Sema] Add implicit members even for invalid CXXRecordDecls
I'll try to come up with a fix, thanks for spotting that. On Sat, Feb 3, 2018 at 2:24 AM Eric Fiselierwrote: > This causes some stray diagnostics to be emitted in certian cases where a > base class is already invalid: > > Reproducer: > https://gist.github.com/EricWF/588a361030edeaebbbc1155b8347cab0 > > On Fri, Feb 2, 2018 at 1:40 AM, Ilya Biryukov via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: ibiryukov >> Date: Fri Feb 2 00:40:08 2018 >> New Revision: 324062 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=324062=rev >> Log: >> [Sema] Add implicit members even for invalid CXXRecordDecls >> >> Summary: >> It should be safe, since other code paths are already generating >> implicit members even in invalid CXXRecordDecls (e.g. lookup). >> >> If we don't generate implicit members on CXXRecordDecl's completion, >> they will be generated by next lookup of constructors. This causes a >> crash when the following conditions are met: >> - a CXXRecordDecl is invalid, >> - it is provided via ExternalASTSource (e.g. from PCH), >> - it has inherited constructors (they create ShadowDecls), >> - lookup of its constructors was not run before ASTWriter serialized >> it. >> >> This may require the ShadowDecls created for inherited constructors to >> be removed from the class, but that's no longer possible since class is >> provided by ExternalASTSource. >> >> See provided lit test for an example. >> >> Reviewers: bkramer >> >> Reviewed By: bkramer >> >> Subscribers: cfe-commits >> >> Differential Revision: https://reviews.llvm.org/D42810 >> >> Added: >> cfe/trunk/test/Index/Inputs/crash-preamble-classes.h >> cfe/trunk/test/Index/crash-preamble-classes.cpp >> Modified: >> cfe/trunk/lib/Sema/SemaDecl.cpp >> >> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=324062=324061=324062=diff >> >> == >> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Feb 2 00:40:08 2018 >> @@ -15440,10 +15440,10 @@ void Sema::ActOnFields(Scope *S, SourceL >>CXXRecord->getDestructor()); >> } >> >> -if (!CXXRecord->isInvalidDecl()) { >> - // Add any implicitly-declared members to this class. >> - AddImplicitlyDeclaredMembersToClass(CXXRecord); >> +// Add any implicitly-declared members to this class. >> +AddImplicitlyDeclaredMembersToClass(CXXRecord); >> >> +if (!CXXRecord->isInvalidDecl()) { >>// If we have virtual base classes, we may end up finding >> multiple >>// final overriders for a given virtual function. Check for >> this >>// problem now. >> >> Added: cfe/trunk/test/Index/Inputs/crash-preamble-classes.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/Inputs/crash-preamble-classes.h?rev=324062=auto >> >> == >> --- cfe/trunk/test/Index/Inputs/crash-preamble-classes.h (added) >> +++ cfe/trunk/test/Index/Inputs/crash-preamble-classes.h Fri Feb 2 >> 00:40:08 2018 >> @@ -0,0 +1,9 @@ >> +struct Incomplete; >> + >> +struct X : Incomplete { >> + X(); >> +}; >> + >> +struct Y : X { >> + using X::X; >> +}; >> >> Added: cfe/trunk/test/Index/crash-preamble-classes.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/crash-preamble-classes.cpp?rev=324062=auto >> >> == >> --- cfe/trunk/test/Index/crash-preamble-classes.cpp (added) >> +++ cfe/trunk/test/Index/crash-preamble-classes.cpp Fri Feb 2 00:40:08 >> 2018 >> @@ -0,0 +1,8 @@ >> +#include "crash-preamble-classes.h" >> + >> +struct Z : Y { >> + Z() {} >> +}; >> + >> +// RUN: env CINDEXTEST_EDITING=1 \ >> +// RUN: c-index-test -test-load-source-reparse 5 local -I %S/Inputs %s >> >> >> ___ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > > -- Regards, Ilya Biryukov ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r324062 - [Sema] Add implicit members even for invalid CXXRecordDecls
This causes some stray diagnostics to be emitted in certian cases where a base class is already invalid: Reproducer: https://gist.github.com/EricWF/588a361030edeaebbbc1155b8347cab0 On Fri, Feb 2, 2018 at 1:40 AM, Ilya Biryukov via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: ibiryukov > Date: Fri Feb 2 00:40:08 2018 > New Revision: 324062 > > URL: http://llvm.org/viewvc/llvm-project?rev=324062=rev > Log: > [Sema] Add implicit members even for invalid CXXRecordDecls > > Summary: > It should be safe, since other code paths are already generating > implicit members even in invalid CXXRecordDecls (e.g. lookup). > > If we don't generate implicit members on CXXRecordDecl's completion, > they will be generated by next lookup of constructors. This causes a > crash when the following conditions are met: > - a CXXRecordDecl is invalid, > - it is provided via ExternalASTSource (e.g. from PCH), > - it has inherited constructors (they create ShadowDecls), > - lookup of its constructors was not run before ASTWriter serialized > it. > > This may require the ShadowDecls created for inherited constructors to > be removed from the class, but that's no longer possible since class is > provided by ExternalASTSource. > > See provided lit test for an example. > > Reviewers: bkramer > > Reviewed By: bkramer > > Subscribers: cfe-commits > > Differential Revision: https://reviews.llvm.org/D42810 > > Added: > cfe/trunk/test/Index/Inputs/crash-preamble-classes.h > cfe/trunk/test/Index/crash-preamble-classes.cpp > Modified: > cfe/trunk/lib/Sema/SemaDecl.cpp > > Modified: cfe/trunk/lib/Sema/SemaDecl.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/ > SemaDecl.cpp?rev=324062=324061=324062=diff > > == > --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) > +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Feb 2 00:40:08 2018 > @@ -15440,10 +15440,10 @@ void Sema::ActOnFields(Scope *S, SourceL >CXXRecord->getDestructor()); > } > > -if (!CXXRecord->isInvalidDecl()) { > - // Add any implicitly-declared members to this class. > - AddImplicitlyDeclaredMembersToClass(CXXRecord); > +// Add any implicitly-declared members to this class. > +AddImplicitlyDeclaredMembersToClass(CXXRecord); > > +if (!CXXRecord->isInvalidDecl()) { >// If we have virtual base classes, we may end up finding > multiple >// final overriders for a given virtual function. Check for this >// problem now. > > Added: cfe/trunk/test/Index/Inputs/crash-preamble-classes.h > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/ > Inputs/crash-preamble-classes.h?rev=324062=auto > > == > --- cfe/trunk/test/Index/Inputs/crash-preamble-classes.h (added) > +++ cfe/trunk/test/Index/Inputs/crash-preamble-classes.h Fri Feb 2 > 00:40:08 2018 > @@ -0,0 +1,9 @@ > +struct Incomplete; > + > +struct X : Incomplete { > + X(); > +}; > + > +struct Y : X { > + using X::X; > +}; > > Added: cfe/trunk/test/Index/crash-preamble-classes.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/ > crash-preamble-classes.cpp?rev=324062=auto > > == > --- cfe/trunk/test/Index/crash-preamble-classes.cpp (added) > +++ cfe/trunk/test/Index/crash-preamble-classes.cpp Fri Feb 2 00:40:08 > 2018 > @@ -0,0 +1,8 @@ > +#include "crash-preamble-classes.h" > + > +struct Z : Y { > + Z() {} > +}; > + > +// RUN: env CINDEXTEST_EDITING=1 \ > +// RUN: c-index-test -test-load-source-reparse 5 local -I %S/Inputs %s > > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r324062 - [Sema] Add implicit members even for invalid CXXRecordDecls
Author: ibiryukov Date: Fri Feb 2 00:40:08 2018 New Revision: 324062 URL: http://llvm.org/viewvc/llvm-project?rev=324062=rev Log: [Sema] Add implicit members even for invalid CXXRecordDecls Summary: It should be safe, since other code paths are already generating implicit members even in invalid CXXRecordDecls (e.g. lookup). If we don't generate implicit members on CXXRecordDecl's completion, they will be generated by next lookup of constructors. This causes a crash when the following conditions are met: - a CXXRecordDecl is invalid, - it is provided via ExternalASTSource (e.g. from PCH), - it has inherited constructors (they create ShadowDecls), - lookup of its constructors was not run before ASTWriter serialized it. This may require the ShadowDecls created for inherited constructors to be removed from the class, but that's no longer possible since class is provided by ExternalASTSource. See provided lit test for an example. Reviewers: bkramer Reviewed By: bkramer Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D42810 Added: cfe/trunk/test/Index/Inputs/crash-preamble-classes.h cfe/trunk/test/Index/crash-preamble-classes.cpp Modified: cfe/trunk/lib/Sema/SemaDecl.cpp Modified: cfe/trunk/lib/Sema/SemaDecl.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=324062=324061=324062=diff == --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Feb 2 00:40:08 2018 @@ -15440,10 +15440,10 @@ void Sema::ActOnFields(Scope *S, SourceL CXXRecord->getDestructor()); } -if (!CXXRecord->isInvalidDecl()) { - // Add any implicitly-declared members to this class. - AddImplicitlyDeclaredMembersToClass(CXXRecord); +// Add any implicitly-declared members to this class. +AddImplicitlyDeclaredMembersToClass(CXXRecord); +if (!CXXRecord->isInvalidDecl()) { // If we have virtual base classes, we may end up finding multiple // final overriders for a given virtual function. Check for this // problem now. Added: cfe/trunk/test/Index/Inputs/crash-preamble-classes.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/Inputs/crash-preamble-classes.h?rev=324062=auto == --- cfe/trunk/test/Index/Inputs/crash-preamble-classes.h (added) +++ cfe/trunk/test/Index/Inputs/crash-preamble-classes.h Fri Feb 2 00:40:08 2018 @@ -0,0 +1,9 @@ +struct Incomplete; + +struct X : Incomplete { + X(); +}; + +struct Y : X { + using X::X; +}; Added: cfe/trunk/test/Index/crash-preamble-classes.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/crash-preamble-classes.cpp?rev=324062=auto == --- cfe/trunk/test/Index/crash-preamble-classes.cpp (added) +++ cfe/trunk/test/Index/crash-preamble-classes.cpp Fri Feb 2 00:40:08 2018 @@ -0,0 +1,8 @@ +#include "crash-preamble-classes.h" + +struct Z : Y { + Z() {} +}; + +// RUN: env CINDEXTEST_EDITING=1 \ +// RUN: c-index-test -test-load-source-reparse 5 local -I %S/Inputs %s ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits