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(); } 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. > 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