On Fri, Sep 9, 2016 at 11:33 AM, Richard Smith <rich...@metafoo.co.uk> wrote:
> On Fri, Sep 9, 2016 at 11:29 AM, Manman Ren via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> 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::getP >> rimaryContext(this=0x000000010f079a60) + 171 at DeclBase.cpp:1013 >> frame #3: 0x0000000105d08a75 clang`clang::DeclContext::getP >> rimaryContext(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::ActOnMethod >> Declaration(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. >> > > Hmm. So the value returned by `ObjCInterfaceDecl::getDefinition` changes > from one definition to another? > Yes, currently. > We go to lengths to avoid this for other kinds of DeclContext because > changes to which declaration is the (canonical) definition break AST > invariants. > I just looked at how C++ handles this. I think we can make ObjectiveC do the same thing (i.e. the logic in ReadCXXRecordDefinition and MergeDefinitionData), so the definition can be invariant once we select it. I will revert this commit for now. Thanks for all the help! Manman > Is this really OK for the Objective-C case? I'd be a little surprised if > this doesn't lead to more problems down the line. > > 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 >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits