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