r280728 - Modules: Fix an assertion in DeclContext::buildLookup.

Wed Sep 7 12:45:42 PDT 2016

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. Since getPrimaryContext  does not guarantee to pull from
the external source, I thought that is why we call getMostRecentDecl in
DeclContext::lookup.

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/f87c7c32/attachment-0001.html>