r280728 - Modules: Fix an assertion in DeclContext::buildLookup.
Manman Ren via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 9 11:29:23 PDT 2016
On Wed, Sep 7, 2016 at 4:44 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Wed, Sep 7, 2016 at 12:45 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>> On Tue, Sep 6, 2016 at 6:54 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>
>>> On Tue, Sep 6, 2016 at 11:16 AM, Manman Ren via cfe-commits <
>>> cfe-commits at 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 @@
>>>> + at interface BaseInterface
>>>> +- (void) test;
>>>> + at 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"
>>>> + at interface DerivedInterface : BaseInterface
>>>> + at 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>
>>>> + at implementation DerivedInterface
>>>> +- (void)test {
>>>> +}
>>>> + at end
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160909/aa6c0acd/attachment-0001.html>
More information about the cfe-commits
mailing list