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. Since getPrimaryContext does not guarantee to pull from the external source, I thought that is why we call getMostRecentDecl in DeclContext::lookup. 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
