subject:"r280728 \- Modules\: Fix an assertion in DeclContext\:\:buildLookup."

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

2016-09-09 Thread Manman Ren via cfe-commits

Committed r281119. Let me know if you see any problem.

Cheers,
Manman

On Fri, Sep 9, 2016 at 12:07 PM, Manman Ren  wrote:

>
>
> On Fri, Sep 9, 2016 at 11:33 AM, Richard Smith 
> wrote:
>
>> On Fri, Sep 9, 2016 at 11:29 AM, Manman Ren via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> On Wed, Sep 7, 2016 at 4:44 PM, Richard Smith 
>>> wrote:
>>>
 On Wed, Sep 7, 2016 at 12:45 PM, Manman Ren 
 wrote:

> On Tue, Sep 6, 2016 at 6:54 PM, Richard Smith 
> wrote:
>
>> On Tue, Sep 6, 2016 at 11:16 AM, Manman Ren via cfe-commits <
>> cfe-commits@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=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=280727=280728=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(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: 0x000102e6c1b0 clang`clang::ObjCInterfaceDecl
>>> ::hasDefinition(this=0x00010f079a38) const + 16 at DeclObjC.h:1445
>>> frame #1: 0x000105d09009 clang`clang::ObjCInterfaceDecl
>>> ::getDefinition(this=0x00010f079a38) + 25 at DeclObjC.h:1455
>>> frame #2: 0x000105d08b2b clang`clang::DeclContext::getP
>>>

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

2016-09-09 Thread Manman Ren via cfe-commits

On Fri, Sep 9, 2016 at 11:33 AM, Richard Smith 
wrote:

> On Fri, Sep 9, 2016 at 11:29 AM, Manman Ren via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> On Wed, Sep 7, 2016 at 4:44 PM, Richard Smith 
>> wrote:
>>
>>> On Wed, Sep 7, 2016 at 12:45 PM, Manman Ren 
>>> wrote:
>>>
 On Tue, Sep 6, 2016 at 6:54 PM, Richard Smith 
 wrote:

> On Tue, Sep 6, 2016 at 11:16 AM, Manman Ren via cfe-commits <
> cfe-commits@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=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=280727=280728=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(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: 0x000102e6c1b0 clang`clang::ObjCInterfaceDecl
>> ::hasDefinition(this=0x00010f079a38) const + 16 at DeclObjC.h:1445
>> frame #1: 0x000105d09009 clang`clang::ObjCInterfaceDecl
>> ::getDefinition(this=0x00010f079a38) + 25 at DeclObjC.h:1455
>> frame #2: 0x000105d08b2b clang`clang::DeclContext::getP
>> rimaryContext(this=0x00010f079a60) + 171 at DeclBase.cpp:1013
>> frame #3: 0x000105d08a75 clang`clang::DeclContext::getP
>> rimaryContext(this=0x00010f079a60) const + 21 at DeclBase.h:1360
>> frame #4: 0x000105d0cda4 
>>

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

2016-09-09 Thread Richard Smith via cfe-commits

On Fri, Sep 9, 2016 at 11:29 AM, Manman Ren via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> On Wed, Sep 7, 2016 at 4:44 PM, Richard Smith 
> wrote:
>
>> On Wed, Sep 7, 2016 at 12:45 PM, Manman Ren  wrote:
>>
>>> On Tue, Sep 6, 2016 at 6:54 PM, Richard Smith 
>>> wrote:
>>>
 On Tue, Sep 6, 2016 at 11:16 AM, Manman Ren via cfe-commits <
 cfe-commits@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=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=280727=280728=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(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: 0x000102e6c1b0 clang`clang::ObjCInterfaceDecl::
> hasDefinition(this=0x00010f079a38) const + 16 at DeclObjC.h:1445
> frame #1: 0x000105d09009 clang`clang::ObjCInterfaceDecl::
> getDefinition(this=0x00010f079a38) + 25 at DeclObjC.h:1455
> frame #2: 0x000105d08b2b clang`clang::DeclContext::
> getPrimaryContext(this=0x00010f079a60) + 171 at DeclBase.cpp:1013
> frame #3: 0x000105d08a75 clang`clang::DeclContext::
> getPrimaryContext(this=0x00010f079a60) const + 21 at DeclBase.h:1360
> frame #4: 0x000105d0cda4 
> clang`clang::DeclContext::lookup(this=0x00010f079a60,
> Name=(Ptr = 4547240953)) const + 164 at DeclBase.cpp:1416
> frame #5: 0x000105d30804 clang`clang::
> ObjCContainerDecl::getMethod(this=0x00010f079a38, Sel=(InfoPtr =
> 4547240953),

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

2016-09-09 Thread Manman Ren via cfe-commits

On Wed, Sep 7, 2016 at 4:44 PM, Richard Smith  wrote:

> On Wed, Sep 7, 2016 at 12:45 PM, Manman Ren  wrote:
>
>> On Tue, Sep 6, 2016 at 6:54 PM, Richard Smith 
>> wrote:
>>
>>> On Tue, Sep 6, 2016 at 11:16 AM, Manman Ren via cfe-commits <
>>> cfe-commits@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=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=280727=280728=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(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: 0x000102e6c1b0
clang`clang::ObjCInterfaceDecl::hasDefinition(this=0x00010f079a38)
const + 16 at DeclObjC.h:1445
frame #1: 0x000105d09009
clang`clang::ObjCInterfaceDecl::getDefinition(this=0x00010f079a38) + 25
at DeclObjC.h:1455
frame #2: 0x000105d08b2b
clang`clang::DeclContext::getPrimaryContext(this=0x00010f079a60) + 171
at DeclBase.cpp:1013
frame #3: 0x000105d08a75
clang`clang::DeclContext::getPrimaryContext(this=0x00010f079a60) const
+ 21 at DeclBase.h:1360
frame #4: 0x000105d0cda4
clang`clang::DeclContext::lookup(this=0x00010f079a60, Name=(Ptr =
4547240953)) const + 164 at DeclBase.cpp:1416
frame #5: 0x000105d30804
clang`clang::ObjCContainerDecl::getMethod(this=0x00010f079a38,
Sel=(InfoPtr = 4547240953), isInstance=true, AllowHidden=false) const + 212
at DeclObjC.cpp:86
frame #6: 0x000105d347bd
clang`clang::ObjCInterfaceDecl::lookupMethod(this=0x00010f079c88,
Sel=(InfoPtr = 4547240953), isInstance=true, shallowCategoryLookup=false,

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

2016-09-07 Thread Richard Smith via cfe-commits

On Wed, Sep 7, 2016 at 12:45 PM, Manman Ren  wrote:

> On Tue, Sep 6, 2016 at 6:54 PM, Richard Smith 
> wrote:
>
>> On Tue, Sep 6, 2016 at 11:16 AM, Manman Ren via cfe-commits <
>> cfe-commits@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=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=280727=280728=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(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=auto
>>> 
>>> ==
>>> ---

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

2016-09-07 Thread Manman Ren via cfe-commits

On Tue, Sep 6, 2016 at 6:54 PM, Richard Smith  wrote:

> On Tue, Sep 6, 2016 at 11:16 AM, Manman Ren via cfe-commits <
> cfe-commits@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=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=280727=280728=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(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=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 @@
>> +@interface BaseInterface
>> +- (void) test;
>> +@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=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"
>> +@interface DerivedInterface : BaseInterface
>> +@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=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=auto
>> 
>> ==
>> ---

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

2016-09-06 Thread Richard Smith via cfe-commits

On Tue, Sep 6, 2016 at 11:16 AM, Manman Ren via cfe-commits <
cfe-commits@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=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/
> DeclBase.cpp?rev=280728=280727=280728=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(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?


> +
>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/Inputs/lookup-assert/Base.h?rev=280728=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 @@
> +@interface BaseInterface
> +- (void) test;
> +@end
> +
>
> Added: cfe/trunk/test/Modules/Inputs/lookup-assert/Derive.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
> Modules/Inputs/lookup-assert/Derive.h?rev=280728=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"
> +@interface DerivedInterface : BaseInterface
> +@end
>
> Added: cfe/trunk/test/Modules/Inputs/lookup-assert/H3.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
> Modules/Inputs/lookup-assert/H3.h?rev=280728=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/Inputs/lookup-assert/module.map?rev=280728=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/lookup-assert.m?rev=280728=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

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

2016-09-06 Thread Manman Ren via cfe-commits

Author: mren
Date: Tue Sep  6 13:16:54 2016
New Revision: 280728

URL: http://llvm.org/viewvc/llvm-project?rev=280728=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/DeclBase.cpp?rev=280728=280727=280728=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(this)->getMostRecentDecl();
 
+  // getMostRecentDecl can change the result of getPrimaryContext. Call
+  // getPrimaryContext afterwards.
+  const DeclContext *PrimaryContext = getPrimaryContext();
+  if (PrimaryContext != this)
+return PrimaryContext->lookup(Name);
+
   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/Inputs/lookup-assert/Base.h?rev=280728=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 @@
+@interface BaseInterface
+- (void) test;
+@end
+

Added: cfe/trunk/test/Modules/Inputs/lookup-assert/Derive.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/lookup-assert/Derive.h?rev=280728=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"
+@interface DerivedInterface : BaseInterface
+@end

Added: cfe/trunk/test/Modules/Inputs/lookup-assert/H3.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/lookup-assert/H3.h?rev=280728=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/Inputs/lookup-assert/module.map?rev=280728=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/lookup-assert.m?rev=280728=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 
+@implementation DerivedInterface
+- (void)test {
+}
+@end


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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

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

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

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

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

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

8 matches

Site Navigation

Mail list logo

Footer information