Committed r281119. Let me know if you see any problem. Cheers, Manman
On Fri, Sep 9, 2016 at 12:07 PM, Manman Ren <manman....@gmail.com> wrote: > > > 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