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