<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Sep 7, 2016 at 12:45 PM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="gmail-h5">On Tue, Sep 6, 2016 at 6:54 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>On Tue, Sep 6, 2016 at 11:16 AM, Manman Ren via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: mren<br>
Date: Tue Sep 6 13:16:54 2016<br>
New Revision: 280728<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=280728&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=280728&view=rev</a><br>
Log:<br>
Modules: Fix an assertion in DeclContext::buildLookup.<br>
<br>
When calling getMostRecentDecl, we can pull in more definitions from<br>
a module. We call getPrimaryContext afterwards to make sure that<br>
we buildLookup on a primary context.<br>
<br>
rdar://27926200<br>
<br>
Added:<br>
cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/<br>
cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/Base.h<br>
cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/Derive.h<br>
cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/H3.h<br>
cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/module.map<br>
cfe/trunk/test/Modules/lookup-<wbr>assert.m<br>
Modified:<br>
cfe/trunk/lib/AST/DeclBase.cpp<br>
<br>
Modified: cfe/trunk/lib/AST/DeclBase.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclBase.cpp?rev=280728&r1=280727&r2=280728&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/lib/AST/DeclBa<wbr>se.cpp?rev=280728&r1=280727&r2<wbr>=280728&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/AST/DeclBase.cpp (original)<br>
+++ cfe/trunk/lib/AST/DeclBase.cpp Tue Sep 6 13:16:54 2016<br>
@@ -1411,10 +1411,6 @@ DeclContext::lookup(Declaratio<wbr>nName Name<br>
assert(DeclKind != Decl::LinkageSpec &&<br>
"Should not perform lookups into linkage specs!");<br>
<br>
- const DeclContext *PrimaryContext = getPrimaryContext();<br>
- if (PrimaryContext != this)<br>
- return PrimaryContext->lookup(Name);<br>
-<br>
// If we have an external source, ensure that any later redeclarations of this<br>
// context have been loaded, since they may add names to the result of this<br>
// lookup (or add external visible storage).<br>
@@ -1422,6 +1418,12 @@ DeclContext::lookup(Declaratio<wbr>nName Name<br>
if (Source)<br>
(void)cast<Decl>(this)->getMo<wbr>stRecentDecl();<br>
<br>
+ // getMostRecentDecl can change the result of getPrimaryContext. Call<br>
+ // getPrimaryContext afterwards.<br>
+ const DeclContext *PrimaryContext = getPrimaryContext();<br>
+ if (PrimaryContext != this)<br>
+ return PrimaryContext->lookup(Name);<br></blockquote><div><br></div></div></div><div>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::hasDefiniti<wbr>on (indirectly called by getPrimaryContext) not loading the definition in this case?</div></div></div></div></blockquote><div><br></div></div></div><div>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.</div></div></div></div></blockquote><div><br></div><div>As far as I can see, it does. For ObjCInterface, getPrimaryContext calls ObjCInterface::getDefinition:</div><div><br></div><div><div> ObjCInterfaceDecl *getDefinition() {</div><div> return hasDefinition()? Data.getPointer()->Definition : nullptr;</div><div> }</div></div><div><br></div><div>hasDefinition() pulls from the external source to find a definition, if it believes the definition is out of date:</div><div><br></div><div><div> bool hasDefinition() const {</div><div> // If the name of this class is out-of-date, bring it up-to-date, which</div><div> // might bring in a definition.</div><div> // Note: a null value indicates that we don't have a definition and that</div><div> // modules are enabled.</div><div> if (!Data.getOpaqueValue())</div><div> getMostRecentDecl();</div><div><br></div><div> return Data.getPointer();</div><div> }</div></div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Since getPrimaryContext does not guarantee to pull from the external source, I thought that is why we call getMostRecentDecl in DeclContext::lookup.</div></div></div></div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Are you suggesting to pull from external source in getPrimaryContext?</div><div><br></div><div>Cheers,</div><div>Manman</div><div><div class="gmail-h5"><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
if (hasExternalVisibleStorage()) {<br>
assert(Source && "external visible storage but no external source?");<br>
<br>
<br>
Added: cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/Base.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/lookup-assert/Base.h?rev=280728&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/Modules/I<wbr>nputs/lookup-assert/Base.h?rev<wbr>=280728&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/Base.h (added)<br>
+++ cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/Base.h Tue Sep 6 13:16:54 2016<br>
@@ -0,0 +1,4 @@<br>
+@interface BaseInterface<br>
+- (void) test;<br>
+@end<br>
+<br>
<br>
Added: cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/Derive.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/lookup-assert/Derive.h?rev=280728&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/Modules/I<wbr>nputs/lookup-assert/Derive.h?r<wbr>ev=280728&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/Derive.h (added)<br>
+++ cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/Derive.h Tue Sep 6 13:16:54 2016<br>
@@ -0,0 +1,3 @@<br>
+#include "Base.h"<br>
+@interface DerivedInterface : BaseInterface<br>
+@end<br>
<br>
Added: cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/H3.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/lookup-assert/H3.h?rev=280728&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/Modules/I<wbr>nputs/lookup-assert/H3.h?rev=2<wbr>80728&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/H3.h (added)<br>
+++ cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/H3.h Tue Sep 6 13:16:54 2016<br>
@@ -0,0 +1 @@<br>
+#include "Base.h"<br>
<br>
Added: cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/module.map<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/lookup-assert/module.map?rev=280728&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/Modules/I<wbr>nputs/lookup-assert/module.map<wbr>?rev=280728&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/module.map (added)<br>
+++ cfe/trunk/test/Modules/Inputs/<wbr>lookup-assert/module.map Tue Sep 6 13:16:54 2016<br>
@@ -0,0 +1,4 @@<br>
+module X {<br>
+ header "H3.h"<br>
+ export *<br>
+}<br>
<br>
Added: cfe/trunk/test/Modules/lookup-<wbr>assert.m<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/lookup-assert.m?rev=280728&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/Modules/l<wbr>ookup-assert.m?rev=280728&view<wbr>=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/Modules/lookup-<wbr>assert.m (added)<br>
+++ cfe/trunk/test/Modules/lookup-<wbr>assert.m Tue Sep 6 13:16:54 2016<br>
@@ -0,0 +1,10 @@<br>
+// RUN: rm -rf %t<br>
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/lookup-assert %s -verify<br>
+// expected-no-diagnostics<br>
+<br>
+#include "Derive.h"<br>
+#import <H3.h><br>
+@implementation DerivedInterface<br>
+- (void)test {<br>
+}<br>
+@end<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>