On Wed, Sep 7, 2016 at 4:44 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> On Wed, Sep 7, 2016 at 12:45 PM, Manman Ren <manman....@gmail.com> wrote: > >> On Tue, Sep 6, 2016 at 6:54 PM, Richard Smith <rich...@metafoo.co.uk> >> wrote: >> >>> On Tue, Sep 6, 2016 at 11:16 AM, Manman Ren via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> >>>> Author: mren >>>> Date: Tue Sep 6 13:16:54 2016 >>>> New Revision: 280728 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=280728&view=rev >>>> Log: >>>> Modules: Fix an assertion in DeclContext::buildLookup. >>>> >>>> When calling getMostRecentDecl, we can pull in more definitions from >>>> a module. We call getPrimaryContext afterwards to make sure that >>>> we buildLookup on a primary context. >>>> >>>> rdar://27926200 >>>> >>>> Added: >>>> cfe/trunk/test/Modules/Inputs/lookup-assert/ >>>> cfe/trunk/test/Modules/Inputs/lookup-assert/Base.h >>>> cfe/trunk/test/Modules/Inputs/lookup-assert/Derive.h >>>> cfe/trunk/test/Modules/Inputs/lookup-assert/H3.h >>>> cfe/trunk/test/Modules/Inputs/lookup-assert/module.map >>>> cfe/trunk/test/Modules/lookup-assert.m >>>> Modified: >>>> cfe/trunk/lib/AST/DeclBase.cpp >>>> >>>> Modified: cfe/trunk/lib/AST/DeclBase.cpp >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclBa >>>> se.cpp?rev=280728&r1=280727&r2=280728&view=diff >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/lib/AST/DeclBase.cpp (original) >>>> +++ cfe/trunk/lib/AST/DeclBase.cpp Tue Sep 6 13:16:54 2016 >>>> @@ -1411,10 +1411,6 @@ DeclContext::lookup(DeclarationName Name >>>> assert(DeclKind != Decl::LinkageSpec && >>>> "Should not perform lookups into linkage specs!"); >>>> >>>> - const DeclContext *PrimaryContext = getPrimaryContext(); >>>> - if (PrimaryContext != this) >>>> - return PrimaryContext->lookup(Name); >>>> - >>>> // If we have an external source, ensure that any later >>>> redeclarations of this >>>> // context have been loaded, since they may add names to the result >>>> of this >>>> // lookup (or add external visible storage). >>>> @@ -1422,6 +1418,12 @@ DeclContext::lookup(DeclarationName Name >>>> if (Source) >>>> (void)cast<Decl>(this)->getMostRecentDecl(); >>>> >>>> + // getMostRecentDecl can change the result of getPrimaryContext. Call >>>> + // getPrimaryContext afterwards. >>>> + const DeclContext *PrimaryContext = getPrimaryContext(); >>>> + if (PrimaryContext != this) >>>> + return PrimaryContext->lookup(Name); >>>> >>> >>> This seems like a bug in getPrimaryContext() -- if there is a >>> not-yet-loaded definition of the DeclContext, getPrimaryContext() should >>> return that instead of returning a non-defining declaration. Why is >>> ObjCInterfaceDecl::hasDefinition (indirectly called by >>> getPrimaryContext) not loading the definition in this case? >>> >> >> In the testing case, we have a definition of the ObjC interface from >> textually including a header file, but the definition is also in a module. >> getPrimaryContext for ObjCInterface currently does not pull from the >> external source. >> > > As far as I can see, it does. For ObjCInterface, getPrimaryContext calls > ObjCInterface::getDefinition: > > ObjCInterfaceDecl *getDefinition() { > return hasDefinition()? Data.getPointer()->Definition : nullptr; > } > > hasDefinition() pulls from the external source to find a definition, if it > believes the definition is out of date: > > bool hasDefinition() const { > // If the name of this class is out-of-date, bring it up-to-date, which > // might bring in a definition. > // Note: a null value indicates that we don't have a definition and > that > // modules are enabled. > if (!Data.getOpaqueValue()) > getMostRecentDecl(); > > return Data.getPointer(); > } > --> You are right, this is the backtrace when calling getPrimaryContext. * frame #0: 0x0000000102e6c1b0 clang`clang::ObjCInterfaceDecl::hasDefinition(this=0x000000010f079a38) const + 16 at DeclObjC.h:1445 frame #1: 0x0000000105d09009 clang`clang::ObjCInterfaceDecl::getDefinition(this=0x000000010f079a38) + 25 at DeclObjC.h:1455 frame #2: 0x0000000105d08b2b clang`clang::DeclContext::getPrimaryContext(this=0x000000010f079a60) + 171 at DeclBase.cpp:1013 frame #3: 0x0000000105d08a75 clang`clang::DeclContext::getPrimaryContext(this=0x000000010f079a60) const + 21 at DeclBase.h:1360 frame #4: 0x0000000105d0cda4 clang`clang::DeclContext::lookup(this=0x000000010f079a60, Name=(Ptr = 4547240953)) const + 164 at DeclBase.cpp:1416 frame #5: 0x0000000105d30804 clang`clang::ObjCContainerDecl::getMethod(this=0x000000010f079a38, Sel=(InfoPtr = 4547240953), isInstance=true, AllowHidden=false) const + 212 at DeclObjC.cpp:86 frame #6: 0x0000000105d347bd clang`clang::ObjCInterfaceDecl::lookupMethod(this=0x000000010f079c88, Sel=(InfoPtr = 4547240953), isInstance=true, shallowCategoryLookup=false, followSuper=true, C=0x0000000000000000) const + 221 at DeclObjC.cpp:671 frame #7: 0x0000000104d1ffae clang`clang::Sema::ActOnMethodDeclaration(this=0x000000010f073a00, S=0x000000010e716780, MethodLoc=(ID = 83), EndLoc=(ID = 96), MethodType=minus, ReturnQT=0x00007fff5fbf98c0, ReturnType=(Ptr = 0x000000010f09c020), SelectorLocs=ArrayRef<clang::SourceLocation> @ 0x00007fff5fbf9368, Sel=(InfoPtr = 4547240953), ArgInfo=0x0000000000000000, CParamInfo=0x00007fff5fbfa318, CNumArgs=0, AttrList=0x0000000000000000, MethodDeclKind=objc_not_keyword, isVariadic=false, MethodDefinition=true) + 3342 at SemaDeclObjC.cpp:4414 In the testing case +#include "Derive.h" +#import <H3.h> we textually include Base.h from Derive.h. When calling getPrimaryContext() -> hasDefinition() above, we already have a definition, so we will not call getMostRecentDecl. We then call getMostRecentDecl from DeclContext::lookup, and will deserialize a definition from module X that includes H3.h (which includes Base.h). To correctly fix this issue, it sounds like we should do sufficient deserialization in getPrimaryContext: to pull from the external source even though we have a definition already. > So, getPrimaryContext() should always pull from the external source when > building with modules, unless we already have a primary context. In either > case, the context returned by getPrimaryContext() should never change -- we > should do sufficient deserialization for it to return the right answer. > > >> Since getPrimaryContext does not guarantee to pull from the external >> source, I thought that is why we call getMostRecentDecl in >> DeclContext::lookup. >> > > That's present to solve a different problem, where we can merge together > multiple definitions of the same entity, and where later definitions can > provide additional lookup results. This happens in C++ due to lazy > declarations of implicit special member functions, and should never result > in the primary context changing. > Thanks for the info! I apparently misunderstood why getMostRecentDecl is there. Manman > > >> Are you suggesting to pull from external source in getPrimaryContext? >> >> Cheers, >> Manman >> >> >> >>> >>>> + >>>> if (hasExternalVisibleStorage()) { >>>> assert(Source && "external visible storage but no external >>>> source?"); >>>> >>>> >>>> Added: cfe/trunk/test/Modules/Inputs/lookup-assert/Base.h >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/I >>>> nputs/lookup-assert/Base.h?rev=280728&view=auto >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/test/Modules/Inputs/lookup-assert/Base.h (added) >>>> +++ cfe/trunk/test/Modules/Inputs/lookup-assert/Base.h Tue Sep 6 >>>> 13:16:54 2016 >>>> @@ -0,0 +1,4 @@ >>>> +@interface BaseInterface >>>> +- (void) test; >>>> +@end >>>> + >>>> >>>> Added: cfe/trunk/test/Modules/Inputs/lookup-assert/Derive.h >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/I >>>> nputs/lookup-assert/Derive.h?rev=280728&view=auto >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/test/Modules/Inputs/lookup-assert/Derive.h (added) >>>> +++ cfe/trunk/test/Modules/Inputs/lookup-assert/Derive.h Tue Sep 6 >>>> 13:16:54 2016 >>>> @@ -0,0 +1,3 @@ >>>> +#include "Base.h" >>>> +@interface DerivedInterface : BaseInterface >>>> +@end >>>> >>>> Added: cfe/trunk/test/Modules/Inputs/lookup-assert/H3.h >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/I >>>> nputs/lookup-assert/H3.h?rev=280728&view=auto >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/test/Modules/Inputs/lookup-assert/H3.h (added) >>>> +++ cfe/trunk/test/Modules/Inputs/lookup-assert/H3.h Tue Sep 6 >>>> 13:16:54 2016 >>>> @@ -0,0 +1 @@ >>>> +#include "Base.h" >>>> >>>> Added: cfe/trunk/test/Modules/Inputs/lookup-assert/module.map >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/I >>>> nputs/lookup-assert/module.map?rev=280728&view=auto >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/test/Modules/Inputs/lookup-assert/module.map (added) >>>> +++ cfe/trunk/test/Modules/Inputs/lookup-assert/module.map Tue Sep 6 >>>> 13:16:54 2016 >>>> @@ -0,0 +1,4 @@ >>>> +module X { >>>> + header "H3.h" >>>> + export * >>>> +} >>>> >>>> Added: cfe/trunk/test/Modules/lookup-assert.m >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/l >>>> ookup-assert.m?rev=280728&view=auto >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/test/Modules/lookup-assert.m (added) >>>> +++ cfe/trunk/test/Modules/lookup-assert.m Tue Sep 6 13:16:54 2016 >>>> @@ -0,0 +1,10 @@ >>>> +// RUN: rm -rf %t >>>> +// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules >>>> -fimplicit-module-maps -I %S/Inputs/lookup-assert %s -verify >>>> +// expected-no-diagnostics >>>> + >>>> +#include "Derive.h" >>>> +#import <H3.h> >>>> +@implementation DerivedInterface >>>> +- (void)test { >>>> +} >>>> +@end >>>> >>>> >>>> _______________________________________________ >>>> 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
