r280728 - Modules: Fix an assertion in DeclContext::buildLookup.
Manman Ren via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 9 17:07:31 PDT 2016
Committed r281119. Let me know if you see any problem.
Cheers,
Manman
On Fri, Sep 9, 2016 at 12:07 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
> On Fri, Sep 9, 2016 at 11:33 AM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>
>> On Fri, Sep 9, 2016 at 11:29 AM, Manman Ren via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>>
>>> 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::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 @@
>>>>>>> + 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
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>> _______________________________________________
>>> 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/bf71632a/attachment-0001.html>
More information about the cfe-commits
mailing list