r280728 - Modules: Fix an assertion in DeclContext::buildLookup.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 7 16:44:32 PDT 2016
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();
}
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 @@
>>> + 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/20160907/c9bbd273/attachment-0001.html>
More information about the cfe-commits
mailing list