r280728 - Modules: Fix an assertion in DeclContext::buildLookup.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 9 11:33:05 PDT 2016
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::
> 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.
>
Hmm. So the value returned by `ObjCInterfaceDecl::getDefinition` changes
from one definition to another? We go to lengths to avoid this for other
kinds of DeclContext because changes to which declaration is the
(canonical) definition break AST invariants.
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/128857cc/attachment-0001.html>
More information about the cfe-commits
mailing list