<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Yes, I wasn't sure how to test it at this level (there's a pending test at the swift level) but, as you pointed out in separate email, there was an adaptable test in the original commit this is a copy-and-modification of, so I copied-and-modified the test as well. Posted for review in <a href="https://reviews.llvm.org/D34788" class="">https://reviews.llvm.org/D34788</a><div class=""><br class=""></div><div class="">-Graydon<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Jun 28, 2017, at 2:18 PM, Bruno Cardoso Lopes <<a href="mailto:bruno.cardoso@gmail.com" class="">bruno.cardoso@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Hi Graydon,<div class=""><br class=""></div><div class="">Can you please add a testcase for this?</div><div class=""><br class=""></div><div class="">Thanks,</div></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Wed, Jun 28, 2017 at 11:36 AM, Graydon Hoare via cfe-commits <span dir="ltr" class=""><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="">cfe-commits@lists.llvm.org</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: graydon<br class="">
Date: Wed Jun 28 11:36:27 2017<br class="">
New Revision: 306583<br class="">
<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=306583&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-<wbr class="">project?rev=306583&view=rev</a><br class="">
Log:<br class="">
[ASTReader] Treat multiple defns of ObjC protocols the same as interfaces.<br class="">
<br class="">
Summary:<br class="">
In change 2ba19793512, the ASTReader logic for ObjC interfaces was modified to<br class="">
preserve the first definition-data read, "merging" later definitions into it<br class="">
rather than overwriting it (though this "merging" is, in practice, a no-op that<br class="">
discards the later definition-data).<br class="">
<br class="">
Unfortunately this change was only made to ObjC interfaces, not protocols; this<br class="">
means that when (for example) loading a protocol that references an interface,<br class="">
if both the protocol and interface are multiply defined (as can easily happen<br class="">
if the same header is read from multiple contexts), an _inconsistent_ pair of<br class="">
definitions is loaded: first-read for the interface and last-read for the<br class="">
protocol.<br class="">
<br class="">
This in turn causes very subtle downstream bugs in the Swift ClangImporter,<br class="">
which filters the results of name lookups based on the owning module of a<br class="">
definition; inconsistency between a pair of related definitions causes name<br class="">
lookup failures at various stages of compilation.<br class="">
<br class="">
To fix these downstream issues, this change replicates the logic applied to<br class="">
interfaces in change 2ba19793512, but for ObjC protocols.<br class="">
<br class="">
<a href="rdar://30851899" class="">rdar://30851899</a><br class="">
<br class="">
Reviewers: doug.gregor, rsmith<br class="">
<br class="">
Reviewed By: doug.gregor<br class="">
<br class="">
Subscribers: jordan_rose, cfe-commits<br class="">
<br class="">
Differential Revision: <a href="https://reviews.llvm.org/D34741" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/<wbr class="">D34741</a><br class="">
<br class="">
Modified:<br class="">
cfe/trunk/include/clang/AST/<wbr class="">Redeclarable.h<br class="">
cfe/trunk/lib/Serialization/<wbr class="">ASTReaderDecl.cpp<br class="">
<br class="">
Modified: cfe/trunk/include/clang/AST/<wbr class="">Redeclarable.h<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Redeclarable.h?rev=306583&r1=306582&r2=306583&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-<wbr class="">project/cfe/trunk/include/<wbr class="">clang/AST/Redeclarable.h?rev=<wbr class="">306583&r1=306582&r2=306583&<wbr class="">view=diff</a><br class="">
==============================<wbr class="">==============================<wbr class="">==================<br class="">
--- cfe/trunk/include/clang/AST/<wbr class="">Redeclarable.h (original)<br class="">
+++ cfe/trunk/include/clang/AST/<wbr class="">Redeclarable.h Wed Jun 28 11:36:27 2017<br class="">
@@ -21,6 +21,60 @@<br class="">
namespace clang {<br class="">
class ASTContext;<br class="">
<br class="">
+// Some notes on redeclarables:<br class="">
+//<br class="">
+// - Every redeclarable is on a circular linked list.<br class="">
+//<br class="">
+// - Every decl has a pointer to the first element of the chain _and_ a<br class="">
+// DeclLink that may point to one of 3 possible states:<br class="">
+// - the "previous" (temporal) element in the chain<br class="">
+// - the "latest" (temporal) element in the chain<br class="">
+// - the an "uninitialized-latest" value (when newly-constructed)<br class="">
+//<br class="">
+// - The first element is also often called the canonical element. Every<br class="">
+// element has a pointer to it so that "getCanonical" can be fast.<br class="">
+//<br class="">
+// - Most links in the chain point to previous, except the link out of<br class="">
+// the first; it points to latest.<br class="">
+//<br class="">
+// - Elements are called "first", "previous", "latest" or<br class="">
+// "most-recent" when referring to temporal order: order of addition<br class="">
+// to the chain.<br class="">
+//<br class="">
+// - To make matters confusing, the DeclLink type uses the term "next"<br class="">
+// for its pointer-storage internally (thus functions like<br class="">
+// NextIsPrevious). It's easiest to just ignore the implementation of<br class="">
+// DeclLink when making sense of the redeclaration chain.<br class="">
+//<br class="">
+// - There's also a "definition" link for several types of<br class="">
+// redeclarable, where only one definition should exist at any given<br class="">
+// time (and the defn pointer is stored in the decl's "data" which<br class="">
+// is copied to every element on the chain when it's changed).<br class="">
+//<br class="">
+// Here is some ASCII art:<br class="">
+//<br class="">
+// "first" "latest"<br class="">
+// "canonical" "most recent"<br class="">
+// +------------+ first +--------------+<br class="">
+// | | <--------------------------- | |<br class="">
+// | | | |<br class="">
+// | | | |<br class="">
+// | | +--------------+ | |<br class="">
+// | | first | | | |<br class="">
+// | | <---- | | | |<br class="">
+// | | | | | |<br class="">
+// | @class A | link | @interface A | link | @class A |<br class="">
+// | seen first | <---- | seen second | <---- | seen third |<br class="">
+// | | | | | |<br class="">
+// +------------+ +--------------+ +--------------+<br class="">
+// | data | defn | data | defn | data |<br class="">
+// | | ----> | | <---- | |<br class="">
+// +------------+ +--------------+ +--------------+<br class="">
+// | | ^ ^<br class="">
+// | |defn | |<br class="">
+// | link +-----+ |<br class="">
+// +-->--------------------------<wbr class="">-----------------+<br class="">
+<br class="">
/// \brief Provides common interface for the Decls that can be redeclared.<br class="">
template<typename decl_type><br class="">
class Redeclarable {<br class="">
<br class="">
Modified: cfe/trunk/lib/Serialization/<wbr class="">ASTReaderDecl.cpp<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=306583&r1=306582&r2=306583&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-<wbr class="">project/cfe/trunk/lib/<wbr class="">Serialization/ASTReaderDecl.<wbr class="">cpp?rev=306583&r1=306582&r2=<wbr class="">306583&view=diff</a><br class="">
==============================<wbr class="">==============================<wbr class="">==================<br class="">
--- cfe/trunk/lib/Serialization/<wbr class="">ASTReaderDecl.cpp (original)<br class="">
+++ cfe/trunk/lib/Serialization/<wbr class="">ASTReaderDecl.cpp Wed Jun 28 11:36:27 2017<br class="">
@@ -126,6 +126,9 @@ namespace clang {<br class="">
void ReadObjCDefinitionData(struct ObjCInterfaceDecl::<wbr class="">DefinitionData &Data);<br class="">
void MergeDefinitionData(<wbr class="">ObjCInterfaceDecl *D,<br class="">
struct ObjCInterfaceDecl::<wbr class="">DefinitionData &&NewDD);<br class="">
+ void ReadObjCDefinitionData(struct ObjCProtocolDecl::<wbr class="">DefinitionData &Data);<br class="">
+ void MergeDefinitionData(<wbr class="">ObjCProtocolDecl *D,<br class="">
+ struct ObjCProtocolDecl::<wbr class="">DefinitionData &&NewDD);<br class="">
<br class="">
static NamedDecl *getAnonymousDeclForMerging(<wbr class="">ASTReader &Reader,<br class="">
DeclContext *DC,<br class="">
@@ -1045,18 +1048,8 @@ void ASTDeclReader::<wbr class="">VisitObjCIvarDecl(Ob<br class="">
IVD->setSynthesize(synth);<br class="">
}<br class="">
<br class="">
-void ASTDeclReader::<wbr class="">VisitObjCProtocolDecl(<wbr class="">ObjCProtocolDecl *PD) {<br class="">
- RedeclarableResult Redecl = VisitRedeclarable(PD);<br class="">
- VisitObjCContainerDecl(PD);<br class="">
- mergeRedeclarable(PD, Redecl);<br class="">
-<br class="">
- if (Record.readInt()) {<br class="">
- // Read the definition.<br class="">
- PD->allocateDefinitionData();<br class="">
-<br class="">
- // Set the definition data of the canonical declaration, so other<br class="">
- // redeclarations will see it.<br class="">
- PD->getCanonicalDecl()->Data = PD->Data;<br class="">
+void ASTDeclReader::<wbr class="">ReadObjCDefinitionData(<br class="">
+ struct ObjCProtocolDecl::<wbr class="">DefinitionData &Data) {<br class="">
<br class="">
unsigned NumProtoRefs = Record.readInt();<br class="">
SmallVector<ObjCProtocolDecl *, 16> ProtoRefs;<br class="">
@@ -1067,9 +1060,37 @@ void ASTDeclReader::<wbr class="">VisitObjCProtocolDec<br class="">
ProtoLocs.reserve(<wbr class="">NumProtoRefs);<br class="">
for (unsigned I = 0; I != NumProtoRefs; ++I)<br class="">
ProtoLocs.push_back(<wbr class="">ReadSourceLocation());<br class="">
- PD->setProtocolList(ProtoRefs.<wbr class="">data(), NumProtoRefs, ProtoLocs.data(),<br class="">
- Reader.getContext());<br class="">
+ Data.ReferencedProtocols.set(<wbr class="">ProtoRefs.data(), NumProtoRefs,<br class="">
+ ProtoLocs.data(), Reader.getContext());<br class="">
+}<br class="">
+<br class="">
+void ASTDeclReader::<wbr class="">MergeDefinitionData(<wbr class="">ObjCProtocolDecl *D,<br class="">
+ struct ObjCProtocolDecl::<wbr class="">DefinitionData &&NewDD) {<br class="">
+ // FIXME: odr checking?<br class="">
+}<br class="">
<br class="">
+void ASTDeclReader::<wbr class="">VisitObjCProtocolDecl(<wbr class="">ObjCProtocolDecl *PD) {<br class="">
+ RedeclarableResult Redecl = VisitRedeclarable(PD);<br class="">
+ VisitObjCContainerDecl(PD);<br class="">
+ mergeRedeclarable(PD, Redecl);<br class="">
+<br class="">
+ if (Record.readInt()) {<br class="">
+ // Read the definition.<br class="">
+ PD->allocateDefinitionData();<br class="">
+<br class="">
+ ReadObjCDefinitionData(PD-><wbr class="">data());<br class="">
+<br class="">
+ ObjCProtocolDecl *Canon = PD->getCanonicalDecl();<br class="">
+ if (Canon->Data.getPointer()) {<br class="">
+ // If we already have a definition, keep the definition invariant and<br class="">
+ // merge the data.<br class="">
+ MergeDefinitionData(Canon, std::move(PD->data()));<br class="">
+ PD->Data = Canon->Data;<br class="">
+ } else {<br class="">
+ // Set the definition data of the canonical declaration, so other<br class="">
+ // redeclarations will see it.<br class="">
+ PD->getCanonicalDecl()->Data = PD->Data;<br class="">
+ }<br class="">
// Note that we have deserialized a definition.<br class="">
Reader.PendingDefinitions.<wbr class="">insert(PD);<br class="">
} else {<br class="">
<br class="">
<br class="">
______________________________<wbr class="">_________________<br class="">
cfe-commits mailing list<br class="">
<a href="mailto:cfe-commits@lists.llvm.org" class="">cfe-commits@lists.llvm.org</a><br class="">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/<wbr class="">mailman/listinfo/cfe-commits</a><br class="">
</blockquote></div><br class=""><br clear="all" class=""><div class=""><br class=""></div>-- <br class=""><div class="gmail_signature" data-smartmail="gmail_signature">Bruno Cardoso Lopes <br class=""><a href="http://www.brunocardoso.cc/" target="_blank" class="">http://www.brunocardoso.cc</a></div>
</div>
</div></blockquote></div><br class=""></div></body></html>