r306583 - [ASTReader] Treat multiple defns of ObjC protocols the same as interfaces.
Graydon Hoare via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 28 16:42:31 PDT 2017
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.cardoso at 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 at lists.llvm.org <mailto:cfe-commits at 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 at lists.llvm.org <mailto:cfe-commits at 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/>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170628/2aa5221d/attachment-0001.html>
More information about the cfe-commits
mailing list