[PATCH] PCH Support for C++ decls

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] PCH Support for C++ decls

Andrew Sutton
All,

Since I noticed Doug commit PCH support for NamespaceDecl's the other day (week?), I thought I'd send an update to my last PCH patch. I've been sitting on this work for quite some time while I've been busy with other things.

The attached implements serialization for
- namespaces, using directives, namespace aliases, using (and shadow) declarations
- classes
- methods, constructors, destructors, conversion functions
- and linkage specifications
There's some support for serializing friend declarations, but only if it's not a friend type. Fixing that should be relatively easy. There is also some work on serializing templates and dependent types, but it's asserted out for now. I integrated Doug's implementation of VisitNamespaceDecl (I wasn't serializing the AnonymousNamespace) into this patch.

The patch adds a large number of getters and setters to AST declarations since the PCH code has to construct them piecemeal. I've had to expose some new functionality for creating CXXRecord::DefinitionData objects (see createDefinition() and shareDefinition() in the attached patch). I think it's kind of nice. I also renamed one or two existing methods to make them have similar names as other functions--I forget which methods, though. These changes have been propagated to Sema.

I had to add new Create() functions for CXXMethod and its derivations since the normal Create() function asserts when you try to create these objects without names. As per a previous suggestion, these take an "EmptyShell" type, which is added as a public empty class in Decl (should it be private?)

WRT to the PCH reader/writer classes, I added some new functions for reading and writing base class specifiers and nested name specifiers. I also added a ReadSourceLocation and ReadSourceRange methods to the PCHReader to make their reading consistent with other operations (and easier in some cases).

I modified PCH/namespaces test to include some of the other namespace and using decls and linkage specs. There should be a new classes test in the patch with (let's just say... "not exhaustive") tests for serializing classes.

Please let me know if there is anything that I can do to help get this patch applied.

Andrew Sutton
[hidden email]

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

pch.patch (88K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Douglas Gregor

On Mar 2, 2010, at 8:26 AM, Andrew Sutton wrote:

> All,
>
> Since I noticed Doug commit PCH support for NamespaceDecl's the other day (week?), I thought I'd send an update to my last PCH patch. I've been sitting on this work for quite some time while I've been busy with other things.
>
> The attached implements serialization for
> - namespaces, using directives, namespace aliases, using (and shadow) declarations
> - classes
> - methods, constructors, destructors, conversion functions
> - and linkage specifications
> There's some support for serializing friend declarations, but only if it's not a friend type. Fixing that should be relatively easy. There is also some work on serializing templates and dependent types, but it's asserted out for now. I integrated Doug's implementation of VisitNamespaceDecl (I wasn't serializing the AnonymousNamespace) into this patch.

Nifty.

> The patch adds a large number of getters and setters to AST declarations since the PCH code has to construct them piecemeal. I've had to expose some new functionality for creating CXXRecord::DefinitionData objects (see createDefinition() and shareDefinition() in the attached patch). I think it's kind of nice.

This didn't seem necessary to me; see comments below.

> I had to add new Create() functions for CXXMethod and its derivations since the normal Create() function asserts when you try to create these objects without names. As per a previous suggestion, these take an "EmptyShell" type, which is added as a public empty class in Decl (should it be private?)

It's fine as a public empty class; that's what we do for expressions.

> WRT to the PCH reader/writer classes, I added some new functions for reading and writing base class specifiers and nested name specifiers. I also added a ReadSourceLocation and ReadSourceRange methods to the PCHReader to make their reading consistent with other operations (and easier in some cases).

Good, good. At some point (but not as part of this patch!) we should go replace all of the manually-deserialized source locations and ranges with uses of these functions.

> I modified PCH/namespaces test to include some of the other namespace and using decls and linkage specs. There should be a new classes test in the patch with (let's just say... "not exhaustive") tests for serializing classes.

I think we really need better tests. Due to PCH's laziness, it is extremely annoying to debug when it breaks, so small tests for each kind of declaration/statement/expression are important for catching problems quickly.

> Please let me know if there is anything that I can do to help get this patch applied.

Some comments:

Index: include/clang/Frontend/PCHBitCodes.h
===================================================================
--- include/clang/Frontend/PCHBitCodes.h (revision 97565)
+++ include/clang/Frontend/PCHBitCodes.h (working copy)
+      // FIXME: Serialize these or not?
+      DECL_UNRESOLVED_USING_VALUE,
+      DECL_UNRESOLVED_USING_TYPENAME,

Yes, we will have to serialize these. Every bit in the AST gets serialized.

@@ -524,9 +565,7 @@
       /// associates a declaration name with one or more declaration
       /// IDs. This data is used when performing qualified name lookup
       /// into a DeclContext via DeclContext::lookup.
-      DECL_CONTEXT_VISIBLE,
-      /// \brief A NamespaceDecl record.
-      DECL_NAMESPACE
+      DECL_CONTEXT_VISIBLE
     };

We should keep the existing DECL_* constants in order, since each reshuffle is (unnecessary) breakage in backward-compatibility for the PCH format.

Index: include/clang/AST/DeclCXX.h
===================================================================
--- include/clang/AST/DeclCXX.h (revision 97565)
+++ include/clang/AST/DeclCXX.h (working copy)
@@ -369,6 +390,21 @@
 
   bool hasDefinition() const { return DefinitionData != 0; }
 
+  // FIXME: This method is required for PCH reading.
+  // Initialize the definition for this class and propagate the definition
+  // to any previous declarartions.
+  void createDefinition();
+
+  // FIXME: This method ios requried for PCH reading.
+  // Force this class to use the definition of the other. This is an error
+  // if the class is already defined.
+  // FIXME: Are there other conditions we should check? I think it's required
+  // that D be a previous declaration of this. Not sure.
+  void shareDefinition(CXXRecordDecl *D) {
+    assert((D && !DefinitionData) && "class already has definition data");
+    DefinitionData = D->DefinitionData;
+  }

I don't see why we should need this. We already have startDefinition()/completeDefinition(), which takes care of allocating the DefinitionData and linking it in.

Index: lib/Frontend/PCHWriter.cpp
===================================================================
--- lib/Frontend/PCHWriter.cpp (revision 97565)
+++ lib/Frontend/PCHWriter.cpp (working copy)
@@ -2188,6 +2193,15 @@
   }
 }
 
+void PCHWriter::AddBaseClassRef(const CXXBaseSpecifier &Base,
+                                RecordData &Record) {
+  AddSourceRange(Base.getSourceRange(), Record);
+  Record.push_back(Base.isBaseOfClass());
+  Record.push_back(Base.isVirtual());
+  Record.push_back(Base.getAccessSpecifierAsWritten());
+  AddTypeRef(Base.getType(), Record);
+}

Please store/load fields in the PCH record in the same order as they occur in the corresponding AST; it makes it easier to verify that everything is there. In this case, BaseOfClass and Virtual should be swapped (the same goes for the reader).

+void PCHWriter::AddNestedNameSpecifier(const NestedNameSpecifier *Spec,
+                                       RecordData &Record) {

Why const-qualify the NestedNameSpecifier? You're just const_cast'ing it away anyway.

+  // We need to serialized the number of NNS components since it doesn't
+  // seem to be possible to read linked lists--we need to know where the list
+  // actually stops.
+  unsigned N = 0;
+  NestedNameSpecifier *P = const_cast<NestedNameSpecifier*>(Spec);
+  while (P) {
+    ++N;
+    P = P->getPrefix();
+  }
+  Record.push_back(N);
+
+  P = const_cast<NestedNameSpecifier*>(Spec);
+  while (P) {
+    NestedNameSpecifier::SpecifierKind Kind = P->getKind();
+    Record.push_back(Kind);
+    switch (Kind) {
+    case NestedNameSpecifier::Identifier:
+      AddIdentifierRef(P->getAsIdentifier(), Record);
+      break;
+
+    case NestedNameSpecifier::Namespace:
+      AddDeclRef(P->getAsNamespace(), Record);
+      break;
+
+    case NestedNameSpecifier::TypeSpec:
+    case NestedNameSpecifier::TypeSpecWithTemplate:
+      AddTypeRef(QualType(P->getAsType(), 0), Record);
+      Record.push_back(Kind == NestedNameSpecifier::TypeSpecWithTemplate);
+      break;
+
+    case NestedNameSpecifier::Global:
+      // Don't need to write an associated value.
+      break;
+    }
+    P = P->getPrefix();
+  }
+}

Why not push all of the NestedNameSpecifiers onto a stack and then emit them in reverse order? That would make it far easier for the reader to reconstruct them properly, since the prefix will come before the nested-name-specifier itself.

+NestedNameSpecifier *
+PCHReader::ReadNestedNameSpecifier(const RecordData &Record, unsigned &Idx) {
+  unsigned N = Record[Idx++];
+  NestedNameSpecifier *NNS = 0, *P = 0;
+  for (unsigned I = 0; I != N; ++I) {
+    NestedNameSpecifier::SpecifierKind Kind
+      = (NestedNameSpecifier::SpecifierKind)Record[Idx++];
+    switch (Kind) {
+    case NestedNameSpecifier::Identifier: {
+      IdentifierInfo *II = GetIdentifierInfo(Record, Idx);
+      P = NestedNameSpecifier::Create(*Context, P, II);
+      break;
+    }
+
+    case NestedNameSpecifier::Namespace: {
+      NamespaceDecl *NS = cast<NamespaceDecl>(GetDecl(Record[Idx++]));
+      P = NestedNameSpecifier::Create(*Context, P, NS);
+      break;
+    }
+
+    case NestedNameSpecifier::TypeSpec:
+    case NestedNameSpecifier::TypeSpecWithTemplate: {
+      Type *T = GetType(Record[Idx++]).getTypePtr();
+      bool Template = Record[Idx++];
+      P = NestedNameSpecifier::Create(*Context, P, Template, T);
+      break;
+    }
+
+    case NestedNameSpecifier::Global: {
+      P = NestedNameSpecifier::GlobalSpecifier(*Context);
+      // No associated value.
+      break;
+    }
+
+    // The "first" prefix will be the return value.
+    if (!NNS)
+      NNS = P;
+    }
+  }
+  return NNS;
+}

This won't work with nested-name-specifiers that have prefixes, because we see the nested-name-specifier before its prefix, and end up trying to build the nested-name-specifier backwards. Use the stack I mentioned previously to address this problem, and you can then get rid of the NNS/P dance, since the last nested-name-specifier you build will be the complete one.

Index: lib/Frontend/PCHReaderDecl.cpp
===================================================================
--- lib/Frontend/PCHReaderDecl.cpp (revision 97565)
+++ lib/Frontend/PCHReaderDecl.cpp (working copy)
@@ -39,7 +41,6 @@
     void VisitDecl(Decl *D);
     void VisitTranslationUnitDecl(TranslationUnitDecl *TU);
     void VisitNamedDecl(NamedDecl *ND);
-    void VisitNamespaceDecl(NamespaceDecl *D);
     void VisitTypeDecl(TypeDecl *TD);
     void VisitTypedefDecl(TypedefDecl *TD);
     void VisitTagDecl(TagDecl *TD);
@@ -56,6 +57,31 @@
     void VisitFileScopeAsmDecl(FileScopeAsmDecl *AD);
     void VisitBlockDecl(BlockDecl *BD);
     std::pair<uint64_t, uint64_t> VisitDeclContext(DeclContext *DC);
+
+    void VisitLinkageSpecDecl(LinkageSpecDecl *D);
+    void VisitNamespaceDecl(NamespaceDecl *D);
+    void VisitNamespaceAliasDecl(NamespaceAliasDecl *D);
+    void VisitUsing(UsingDecl *D);
+    void VisitUsingShadow(UsingShadowDecl *D);
+    void VisitUsingDirectiveDecl(UsingDirectiveDecl *D);
+    void VisitUnresolvedUsingValue(UnresolvedUsingValueDecl *D);
+    void VisitUnresolvedUsingTypename(UnresolvedUsingTypenameDecl *D);
+    void VisitCXXRecordDecl(CXXRecordDecl *D);
+    void VisitCXXMethodDecl(CXXMethodDecl *D);
+    void VisitCXXConstructorDecl(CXXConstructorDecl *D);
+    void VisitCXXDestructorDecl(CXXDestructorDecl *D);
+    void VisitCXXConversionDecl(CXXConversionDecl *D);
+    void VisitFriendDecl(FriendDecl *D);
+    void VisitTemplateDecl(TemplateDecl *D);
+    void VisitClassTemplateDecl(ClassTemplateDecl *D);
+    void VisitClassTemplateSpecializationDecl(ClassTemplateSpecializationDecl *D);
+    void VisitClassTemplatePartialSpecializationDecl(ClassTemplatePartialSpecializationDecl *D);
+    void visitFunctionTemplateDecl(FunctionTemplateDecl *D);
+    void VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D);
+    void VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D);
+    void VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D);
+    void VisitStaticAssertDecl(StaticAssertDecl *D);

FWIW, I've typically been ordering the Visit functions the same way as the Decl nodes are ordered in DeclNodes.def.

+void PCHDeclReader::VisitNamespaceAliasDecl(NamespaceAliasDecl *D) {
+  VisitNamedDecl(D);
+  D->setAliasLoc(Reader.ReadSourceLocation(Record, Idx));
+  D->setTargetNameLoc(Reader.ReadSourceLocation(Record, Idx));

Please serialize in the same order as the declarations in the Decl node (here, TargetNameLoc should follow the qualifier).

+void PCHDeclReader::VisitUsing(UsingDecl *D) {
+  VisitNamedDecl(D);
+  D->setUsingLocation(Reader.ReadSourceLocation(Record, Idx));
+  D->setNestedNameRange(Reader.ReadSourceRange(Record, Idx));

Same request here :)

+void PCHDeclReader::VisitUsingDirectiveDecl(UsingDirectiveDecl *D) {
+  VisitNamedDecl(D);
+  D->setNamespaceKeyLocation(Reader.ReadSourceLocation(Record, Idx));
+  D->setIdentLocation(Reader.ReadSourceLocation(Record, Idx));
+  D->setQualifierRange(Reader.ReadSourceRange(Record, Idx));
+  Reader.ReadNestedNameSpecifier(Record, Idx);
+  D->setNominatedNamespace(cast<NamedDecl>(Reader.GetDecl(Record[Idx++])));
+  D->setCommonAncestor(dyn_cast<DeclContext>(Reader.GetDecl(Record[Idx++])));
+}

... and here.

Also, why use dyn_cast? If the common ancestor can be NULL, use cast_or_null. (Same comment for PCHDeclWriter::VisitUsingDirectiveDecl).

Aside: anonymous namespaces are somewhat interesting for PCH, because we need to make sure that an anonymous namespace defined in the source file gets linked to an anonymous namespace defined in the PCH file, since they are effectively the same anonymous namespace. One fun way to test this is:

  // In PCH:
  namespace {
    class X;
  }

  // In source file (that includes PCH):
  namespace {
    class X;
  }

  X *x; // okay, but will likely fail with an ambiguity if the two anonymous namespaces don't get linked.

+void PCHDeclReader::VisitCXXRecordDecl(CXXRecordDecl *D) {
+  VisitRecordDecl(D);
+  // Injected class names are, I think, actually empty.

Yes, they are. Aside from making sure to get them into the right declaration context and link up their previous declaration chains, there shouldn't be much to serialize for them.

+  if (!D->isInjectedClassName()) {
+    // We may have to explicitly initialize the classe's internal definition
+    // object if it hasn't already been declared.
+    CXXRecordDecl *Prev =
+      dyn_cast_or_null<CXXRecordDecl>(D->getPreviousDeclaration());
+    if (!Prev) {
+      D->createDefinition();
+    } else {
+      D->shareDefinition(Prev);
+    }

isInjectedClassName() is the wrong check for this. Instead, PCHDeclWriter::VisitCXXRecordDecl should serialize all of the bits in the definition when D->getDefinition() == D, i.e., the declaration you're serializing is the definition.

+    // Read in the numerous class properties.
+    D->setAggregate(Record[Idx++]);
+    D->setPOD(Record[Idx++]);
+    D->setEmpty(Record[Idx++]);
+    D->setPolymorphic(Record[Idx++]);
+    D->setAbstract(Record[Idx++]);
+    D->setHasTrivialConstructor(Record[Idx++]);
+    D->setHasTrivialCopyConstructor(Record[Idx++]);
+    D->setHasTrivialCopyAssignment(Record[Idx++]);
+    D->setHasTrivialDestructor(Record[Idx++]);

'twould be good to follow class-declaration order here.

+void PCHDeclReader::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
+  VisitCXXMethodDecl(D);
+  D->setExplicitSpecified(Record[Idx++]);
+  if (D->isThisDeclarationADefinition())
+    D->setImplicitlyDefined(Record[Idx++]);
+  // FIXME: Serialize member initializers?
+}

I suggest that the PCH reader/writer avoid cleverness, and serialize exactly what is in the AST (rather than doing the isThisDeclarationADefinition() check).

As for the FIXME, the answer is definitely "yes". We need to serialize everything.

+void PCHDeclReader::VisitCXXDestructorDecl(CXXDestructorDecl *D) {
+  VisitCXXMethodDecl(D);
+  if (D->isThisDeclarationADefinition())
+    D->setImplicitlyDefined(Record[Idx++]);
+  D->setOperatorDelete(cast_or_null<FunctionDecl>(Reader.GetDecl(Record[Idx++])));
+}

Same comment here: PCH shouldn't be clever w.r.t. isThisDeclarationADefinition().

+void PCHDeclReader::VisitFriendDecl(FriendDecl *D) {
+  VisitDecl(D);
+  D->setFriendLoc(Reader.ReadSourceLocation(Record, Idx));
+  bool IsType = Record[Idx++];
+  if (IsType) {
+    // FIXME: Not sure how to translate a QualType into a Type*.
+    assert(false && "Cannot serialize friend type declarations yet");
+  } else {
+    D->setFriendDecl(cast<NamedDecl>(Reader.GetDecl(Record[Idx++])));
+  }
+}

If you have a QualType, use getTypePtr() to get the Type*. If you have a Type*, use QualType(the-Type*, 0) to build the QualType.

The WasSpecialization bit is a hack, but it still needs to be stored in the PCH file.

Index: lib/AST/Decl.cpp
===================================================================
--- lib/AST/Decl.cpp (revision 97565)
+++ lib/AST/Decl.cpp (working copy)
@@ -1363,7 +1363,9 @@
     TagT->decl.setInt(1);
   }
 
-  if (isa<CXXRecordDecl>(this)) {
+  if (CXXRecordDecl *D = dyn_cast<CXXRecordDecl>(this)) {
+    D->createDefinition();
+    /*
     CXXRecordDecl *D = cast<CXXRecordDecl>(this);
     struct CXXRecordDecl::DefinitionData *Data =
       new (getASTContext()) struct CXXRecordDecl::DefinitionData(D);
@@ -1371,6 +1373,7 @@
       D->DefinitionData = Data;
       D = cast_or_null<CXXRecordDecl>(D->getPreviousDeclaration());
     } while (D);
+  */
   }
 }

???

@@ -94,11 +103,12 @@
     // Keep track of inherited vbases for this base class.
     const CXXBaseSpecifier *Base = Bases[i];
     QualType BaseType = Base->getType();
+
     // Skip dependent types; we can't do any checking on them now.
     if (BaseType->isDependentType())
       continue;
     CXXRecordDecl *BaseClassDecl
-      = cast<CXXRecordDecl>(BaseType->getAs<RecordType>()->getDecl());
+      = cast<CXXRecordDecl>(BaseType->getAs<RecordType>()->getDecl());\

Stray '\'

Thanks for working on this!

        - Doug
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Andrew Sutton

> Since I noticed Doug commit PCH support for NamespaceDecl's the other day (week?), I thought I'd send an update to my last PCH patch. I've been sitting on this work for quite some time while I've been busy with other things.
>

Nifty.

I've spent the morning addressing issues in the patch and had some questions, comments. I'm not sending up a new patch just yet, though.
 
I think we really need better tests. Due to PCH's laziness, it is extremely annoying to debug when it breaks, so small tests for each kind of declaration/statement/expression are important for catching problems quickly.

Yes... it is very annoying to debug :)
 
We should keep the existing DECL_* constants in order, since each reshuffle is (unnecessary) breakage in backward-compatibility for the PCH format.

I moved all of the constants down to the bottom (and reorganized a couple).
 
I don't see why we should need this. We already have startDefinition()/completeDefinition(), which takes care of allocating the DefinitionData and linking it in.

Removed. I was concerned about side effects, but after looking I don't think there are any. I'm still having problems understanding which classes should be serialized. More below.
 
Please store/load fields in the PCH record in the same order as they occur in the corresponding AST; it makes it easier to verify that everything is there. In this case, BaseOfClass and Virtual should be swapped (the same goes for the reader).

I think I've addressed all instances that you've pointed out, but haven't checked every decl yet. I wish there was an easier way to do this. Maybe I could use clang to dump the order of members...
 
Why const-qualify the NestedNameSpecifier? You're just const_cast'ing it away anyway.

It's a non-modifying operation. I was only const-casting it away because getPrefix() returns a non-const ptr. It's probably easier just to remove the const in the function that try to make NNS more const-friendly.
 
Why not push all of the NestedNameSpecifiers onto a stack and then emit them in reverse order? That would make it far easier for the reader to reconstruct them properly, since the prefix will come before the nested-name-specifier itself.

This won't work with nested-name-specifiers that have prefixes, because we see the nested-name-specifier before its prefix, and end up trying to build the nested-name-specifier backwards. Use the stack I mentioned previously to address this problem, and you can then get rid of the NNS/P dance, since the last nested-name-specifier you build will be the complete one.

For some reason, I thought that trying to write this without a stack would be a better solution. Other than that no real reason. I'll rewrite this in the next day or two.
 
FWIW, I've typically been ordering the Visit functions the same way as the Decl nodes are ordered in DeclNodes.def.

Fair enough. Would it be worthwhile reordering all of the ObjC visitors too?
 
Aside: anonymous namespaces are somewhat interesting for PCH, because we need to make sure that an anonymous namespace defined in the source file gets linked to an anonymous namespace defined in the PCH file, since they are effectively the same anonymous namespace. One fun way to test this is:

 // In PCH:
 namespace {
   class X;
 }

 // In source file (that includes PCH):
 namespace {
   class X;
 }

I'll add this to the test tomorrow.
 
+void PCHDeclReader::VisitCXXRecordDecl(CXXRecordDecl *D) {
+  VisitRecordDecl(D);
+  // Injected class names are, I think, actually empty.

Yes, they are. Aside from making sure to get them into the right declaration context and link up their previous declaration chains, there shouldn't be much to serialize for them.

isInjectedClassName() is the wrong check for this. Instead, PCHDeclWriter::VisitCXXRecordDecl should serialize all of the bits in the definition when D->getDefinition() == D, i.e., the declaration you're serializing is the definition.

This is where I'm a little lost w.r.t. class decls, defs, and injected names... Here's my current understanding.

class C;    // call this CXXRecordDecl* decl;
class C { };  // call this CXXRecordDecl* def;
// call C::C (the injected name) CXXRecordDecl *inj;

decl->getDefinition() == def
def->getDefinition() == def
decl in def->redeclarations() // pseduo python-ese
inj->getDefinition() == def // ???
decl->DefinitionData == def->DefinitionData == inj->DefinitionData // ???
decl->DefiniotinData->Definition == def // ???

And then for a slightly different case, if C isn't defined (so def == inj == 0), then:
decl->getDefinition() == 0?

Is this about right or is there something I might be missing?

Also, w.r.t. class serialization. Should virtual bases be serialized the same way that regular bases are serialized? What b

+void PCHDeclReader::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
+  VisitCXXMethodDecl(D);
+  D->setExplicitSpecified(Record[Idx++]);
+  if (D->isThisDeclarationADefinition())
+    D->setImplicitlyDefined(Record[Idx++]);
+  // FIXME: Serialize member initializers?
+}

I suggest that the PCH reader/writer avoid cleverness, and serialize exactly what is in the AST (rather than doing the isThisDeclarationADefinition() check).

This is a problem since isImplicitlyDefined and setImplicitlyDefined will assert if isThisDeclarationADefintion returns false. Would it be an acceptable workaround to add non-asserting accessors? 

Back to work...

Andrew Sutton
[hidden email]

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Douglas Gregor


Sent from my iPhone

On Mar 3, 2010, at 7:08 AM, Andrew Sutton <[hidden email]> wrote:


> Since I noticed Doug commit PCH support for NamespaceDecl's the other day (week?), I thought I'd send an update to my last PCH patch. I've been sitting on this work for quite some time while I've been busy with other things.
>

Nifty.

I've spent the morning addressing issues in the patch and had some questions, comments. I'm not sending up a new patch just yet, though.
 
I think we really need better tests. Due to PCH's laziness, it is extremely annoying to debug when it breaks, so small tests for each kind of declaration/statement/expression are important for catching problems quickly.

Yes... it is very annoying to debug :)
 
We should keep the existing DECL_* constants in order, since each reshuffle is (unnecessary) breakage in backward-compatibility for the PCH format.

I moved all of the constants down to the bottom (and reorganized a couple).

Thanks. 

 
I don't see why we should need this. We already have startDefinition()/completeDefinition(), which takes care of allocating the DefinitionData and linking it in.

Removed. I was concerned about side effects, but after looking I don't think there are any. I'm still having problems understanding which classes should be serialized. More below.
 
Please store/load fields in the PCH record in the same order as they occur in the corresponding AST; it makes it easier to verify that everything is there. In this case, BaseOfClass and Virtual should be swapped (the same goes for the reader).

I think I've addressed all instances that you've pointed out, but haven't checked every decl yet. I wish there was an easier way to do this. Maybe I could use clang to dump the order of members...

Well, Clang can clearly parse itself, which means that there is a fun (but time-consuming) answer to this. 

 
Why const-qualify the NestedNameSpecifier? You're just const_cast'ing it away anyway.

It's a non-modifying operation. I was only const-casting it away because getPrefix() returns a non-const ptr. It's probably easier just to remove the const in the function that try to make NNS more const-friendly.

A const getPrefix (which does a const_cast internally) would be fine. NestedNameSpecifiers are logically immutable anyway. 

 
Why not push all of the NestedNameSpecifiers onto a stack and then emit them in reverse order? That would make it far easier for the reader to reconstruct them properly, since the prefix will come before the nested-name-specifier itself.

This won't work with nested-name-specifiers that have prefixes, because we see the nested-name-specifier before its prefix, and end up trying to build the nested-name-specifier backwards. Use the stack I mentioned previously to address this problem, and you can then get rid of the NNS/P dance, since the last nested-name-specifier you build will be the complete one.

For some reason, I thought that trying to write this without a stack would be a better solution. Other than that no real reason. I'll rewrite this in the next day or two.

SmallVector makes a good stack. 
 
FWIW, I've typically been ordering the Visit functions the same way as the Decl nodes are ordered in DeclNodes.def.

Fair enough. Would it be worthwhile reordering all of the ObjC visitors too?

Not in this patch. We can tweak that later. 

 
Aside: anonymous namespaces are somewhat interesting for PCH, because we need to make sure that an anonymous namespace defined in the source file gets linked to an anonymous namespace defined in the PCH file, since they are effectively the same anonymous namespace. One fun way to test this is:

 // In PCH:
 namespace {
   class X;
 }

 // In source file (that includes PCH):
 namespace {
   class X;
 }

I'll add this to the test tomorrow.
 
+void PCHDeclReader::VisitCXXRecordDecl(CXXRecordDecl *D) {
+  VisitRecordDecl(D);
+  // Injected class names are, I think, actually empty.

Yes, they are. Aside from making sure to get them into the right declaration context and link up their previous declaration chains, there shouldn't be much to serialize for them.

isInjectedClassName() is the wrong check for this. Instead, PCHDeclWriter::VisitCXXRecordDecl should serialize all of the bits in the definition when D->getDefinition() == D, i.e., the declaration you're serializing is the definition.

This is where I'm a little lost w.r.t. class decls, defs, and injected names... Here's my current understanding.

class C;    // call this CXXRecordDecl* decl;
class C { };  // call this CXXRecordDecl* def;
// call C::C (the injected name) CXXRecordDecl *inj;

decl->getDefinition() == def
def->getDefinition() == def
decl in def->redeclarations() // pseduo python-ese
inj->getDefinition() == def // ???

Yes

decl->DefinitionData == def->DefinitionData == inj->DefinitionData // ???
decl->DefiniotinData->Definition == def // ???

Yes. An injected-class-name is just a redeclaration with a funny scope. And getDefinition always points at the definition. 

And then for a slightly different case, if C isn't defined (so def == inj == 0), then:
decl->getDefinition() == 0?

Yes. 

Is this about right or is there something I might be missing?

I think you have it correct. 

Also, w.r.t. class serialization. Should virtual bases be serialized the same way that regular bases are serialized? What b

They're all the same. Decls are serialized as a reference to somewhere else in the PCH, so it doesn't matter how many times we reference that decl.  

+void PCHDeclReader::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
+  VisitCXXMethodDecl(D);
+  D->setExplicitSpecified(Record[Idx++]);
+  if (D->isThisDeclarationADefinition())
+    D->setImplicitlyDefined(Record[Idx++]);
+  // FIXME: Serialize member initializers?
+}

I suggest that the PCH reader/writer avoid cleverness, and serialize exactly what is in the AST (rather than doing the isThisDeclarationADefinition() check).

This is a problem since isImplicitlyDefined and setImplicitlyDefined will assert if isThisDeclarationADefintion returns false. Would it be an acceptable workaround to add non-asserting accessors? 

Oh, I missed the assert. In that case, please ignore my comment. 

Back to work...

Andrew Sutton
[hidden email]

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Andrew Sutton
In reply to this post by Douglas Gregor

Aside: anonymous namespaces are somewhat interesting for PCH, because we need to make sure that an anonymous namespace defined in the source file gets linked to an anonymous namespace defined in the PCH file, since they are effectively the same anonymous namespace. One fun way to test this is:

 // In PCH:
 namespace {
   class X;
 }

 // In source file (that includes PCH):
 namespace {
   class X;
 }

 X *x; // okay, but will likely fail with an ambiguity if the two anonymous namespaces don't get linked.
 
Just a quick update/observation on this point. It looks like this was easily solved by making sure that the TU serializes its anonymous namespace, which it wasn't. I've fixed this in the patch that I'll probably push tomorrow. I need to write some more tests for classes.

Andrew Sutton
[hidden email]

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Andrew Sutton
In reply to this post by Douglas Gregor
I've spent the morning addressing issues in the patch and had some questions, comments. I'm not sending up a new patch just yet, though.
I had planned to send this up on Saturday, but couldn't connect to the server in the morning so I worked on other stuff instead. Attached is a new version of C++/PCH Decl support. I think I've addressed all of your comments. I've also added support for

- unresolved value/type decls
- friend type references
- ctor member initializers
- stubbed a function for friend template decls

There are 3 issues with this patch that I either don't like or are cause problems.

1. I needed a way to attach the DefinitionData of a CXXRecordDecl that is not a definition to a previously defined record declaration via the PCH reader (hence the attachDefinition()) function in CXXRecordDecl. It basically addresses this problem:

class C { };
class C; // DefinitionData not attached during reading

The DefinitionData isn't attached but the previous declaration is set correctly. IIRC, the same situation happens with injected class names.

I'm not thrilled with this solution. I think the best solution would be to make setPreviousDeclaration automatically share (forward?) DefinitionData. I just didn't want to open that can of worms in this patch.

2. Anonymous namespaces within other namespaces appear to be incorrectly linked in semantic analysis but not PCH serialization. Of course, my interpretation of C++ semantics may also be wrong :) Let's say we have:

// Yes, there are two of  them
namesapce N { namespace { class C; } } // 1
namesapce N { namespace { class C; } } // 2
N::C* p; // ERROR ambiguous lookup for C.

My understanding is that the anonymous namespace should be linked so that 1 and 2 refer to the same class. Is that right? I added a test to SemaCXX/namespaces.cpp to check.

Strangely, the PCH serialization seems to deal with this correctly. Emphasis on "seems to". It doesn't generate an error, and it doesn't crash... I can only guess that this is somehow an issue with lookup or a bizarre artifact of lazy deserialization.

3. There seems to be a bug in the PCH reader when you explicitly initialize member variables of a base class. Let's say you have:

struct B : A {
  B() : A(), x() { } // <- ASSERT
  int x;
};

The assertion occurs in the deserialization (ReadStmt) of the ImplicitValueInit expr for x(). Somehow, after visiting that stmt/expr, the Idx == Record.size() - 1. Curiously, if B has no base classes, this seems to work fine.

I'm not sure what the status of this patch is going to be since a) clang/llvm seems to be in a code freeze and b) this patch introduces an assertion. I have a lot of other things to take care of so I may not be able to address those issues in a timely manner.

Andrew Sutton
[hidden email]

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

pch.patch (108K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Douglas Gregor

On Mar 8, 2010, at 7:44 AM, Andrew Sutton wrote:

I've spent the morning addressing issues in the patch and had some questions, comments. I'm not sending up a new patch just yet, though.
I had planned to send this up on Saturday, but couldn't connect to the server in the morning so I worked on other stuff instead. Attached is a new version of C++/PCH Decl support. I think I've addressed all of your comments. I've also added support for

- unresolved value/type decls
- friend type references
- ctor member initializers
- stubbed a function for friend template decls

It would be much easier to handle such additions as separate patches once the main patch has gone in. Big patches take much longer to review. We haven't quite dealt with all of the issues there yet.

Also, friends are in a state of flux, so it's not worth trying to (de-)serialize them now. Once the AST settles down and access control is turned on by default, it will make sense to implement PCH support with friends.

There are 3 issues with this patch that I either don't like or are cause problems.

1. I needed a way to attach the DefinitionData of a CXXRecordDecl that is not a definition to a previously defined record declaration via the PCH reader (hence the attachDefinition()) function in CXXRecordDecl. It basically addresses this problem:

class C { };
class C; // DefinitionData not attached during reading

The DefinitionData isn't attached but the previous declaration is set correctly. IIRC, the same situation happens with injected class names.

I'm not thrilled with this solution. I think the best solution would be to make setPreviousDeclaration automatically share (forward?) DefinitionData. I just didn't want to open that can of worms in this patch.

Since you posted the original patch, a better solution has come up. When we start a definition of a CXXRecordDecl, we go back and fix up all of the DefinitionData pointers.

2. Anonymous namespaces within other namespaces appear to be incorrectly linked in semantic analysis but not PCH serialization. Of course, my interpretation of C++ semantics may also be wrong :) Let's say we have:

// Yes, there are two of  them
namesapce N { namespace { class C; } } // 1
namesapce N { namespace { class C; } } // 2
N::C* p; // ERROR ambiguous lookup for C.

My understanding is that the anonymous namespace should be linked so that 1 and 2 refer to the same class. Is that right? I added a test to SemaCXX/namespaces.cpp to check.

Yes, good catch. I ended up fixing this as part of PR6341.

3. There seems to be a bug in the PCH reader when you explicitly initialize member variables of a base class. Let's say you have:

struct B : A {
  B() : A(), x() { } // <- ASSERT
  int x;
};

The assertion occurs in the deserialization (ReadStmt) of the ImplicitValueInit expr for x(). Somehow, after visiting that stmt/expr, the Idx == Record.size() - 1. Curiously, if B has no base classes, this seems to work fine.

I'm not sure what the status of this patch is going to be since a) clang/llvm seems to be in a code freeze and b) this patch introduces an assertion. I have a lot of other things to take care of so I may not be able to address those issues in a timely manner.

Only the 2.7 branch of Clang/LLVM is in code freeze. The trunk is in its usual frenzy of activity.

Overall, this patch isn't quite ready to go in. I've attached an updated version of the patch, which brings your patch into line and fixes some of the issues. It cleans up the handling of DefinitionData (#1), eliminates the code for friend type references (which have changed, and may change yet again), and takes a step further toward getting conversion functions right. However, to get conversion functions 100% correct, the list of conversion functions needs to be loaded lazily: see the FIXME I added in PCHDeclReader::VisitCXXRecordDecl.

I think fixing conversion functions and the ctor-initializers are crucial for this patch to go in.

- Doug



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

pch-updated.patch (78K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Andrew Sutton
 
It would be much easier to handle such additions as separate patches once the main patch has gone in. Big patches take much longer to review. We haven't quite dealt with all of the issues there yet.

I realized that after I re-posted it. I can try to break it down into more digestible pieces.
 
Also, friends are in a state of flux, so it's not worth trying to (de-)serialize them now. Once the AST settles down and access control is turned on by default, it will make sense to implement PCH support with friends.

Sounds good. I'll try to take out the friend stuff. 
The DefinitionData isn't attached but the previous declaration is set correctly. IIRC, the same situation happens with injected class names.
Since you posted the original patch, a better solution has come up. When we start a definition of a CXXRecordDecl, we go back and fix up all of the DefinitionData pointers.

That is a better solution :)
 
Only the 2.7 branch of Clang/LLVM is in code freeze. The trunk is in its usual frenzy of activity.

Overall, this patch isn't quite ready to go in.

I think fixing conversion functions and the ctor-initializers are crucial for this patch to go in.

Sounds good. I'll take another turn on it. Unfortunately, I'm pretty busy for the next week or so. Maybe extending PCH support for clang (expressions, too) would make a good GSoC project :)

Andrew Sutton
[hidden email]

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Andrew Sutton
In reply to this post by Douglas Gregor

It would be much easier to handle such additions as separate patches once the main patch has gone in. Big patches take much longer to review. We haven't quite dealt with all of the issues there yet.

Also, friends are in a state of flux, so it's not worth trying to (de-)serialize them now. Once the AST settles down and access control is turned on by default, it will make sense to implement PCH support with friends.


After a couple months of writing papers, a dissertation, making presentations, and interviewing, I've finally been able to sit down and work on some code... So I'm resubmitting this patch. Sort of.

I took the previous (big) patch, re-applied it to a fresh checkout, and then stripped out most of the updates that didn't have anything to do with namespaces, namespace aliases, linkage specs, using directives, etc. I also updated the NamespaceDecl to use the PtrUnion for anon/original namespace pointers.

This might still look like a big patch, but it's not too large. I'm actually stubbing out most of the other Decls, but will just assert if you try to serialize them--except (C++) classes, constructors, destructors, and conversions. These are stubbed out enough to prevent any new regressions from the test suite, but they don't actually do anything.
 
I think fixing conversion functions and the ctor-initializers are crucial for this patch to go in.

Hopefully, since you've already reviewed most of this, it can be applied in relatively short order. After its accepted, I can re-patch and update w.r.t. class (and member) serializations. That should be a much smaller patch, and its easier for me to focus on.

Andrew Sutton
[hidden email]

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

pch-namespaces.patch (71K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Chris Lattner

On May 7, 2010, at 8:01 AM, Andrew Sutton wrote:

I think fixing conversion functions and the ctor-initializers are crucial for this patch to go in.

Hopefully, since you've already reviewed most of this, it can be applied in relatively short order. After its accepted, I can re-patch and update w.r.t. class (and member) serializations. That should be a much smaller patch, and its easier for me to focus on.

This patch looks great to me.  I committed it in r103301.

I'm also interested in working on C++ PCH support in the next couple days.  What can I work on that won't conflict with your work?  If possible, I'd like to shoot for getting C++ PCH support working in the next week or two.

-Chris

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Andrew Sutton
This patch looks great to me.  I committed it in r103301.

Great!
 
I'm also interested in working on C++ PCH support in the next couple days.  What can I work on that won't conflict with your work?  If possible, I'd like to shoot for getting C++ PCH support working in the next week or two.

The remainder of the patch that Doug reviewed dealt with classes, constructors, destructors, and conversion operators needed a little work (to be loaded lazily I think). The next patch will definitely include read/write support for CXXBaseSpecifier and CXXBaseOrMemberInitializer. I cropped those out of this patch, but I don't think that there wasn't anything wrong with them.

I'll try to get the next patch out Monday.

I don't plan on touching the serialization of templates and template parameters The last time I tried to do something with it, I kept getting assertions that I shouldn't serialize dependent types or expressions. That was probably in December, though. Things may have changed since then :)

I haven't looked at any of the CXX expressions either.

Andrew Sutton
[hidden email]

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PCH Support for C++ decls

Chris Lattner

On May 7, 2010, at 3:01 PM, Andrew Sutton wrote:

This patch looks great to me.  I committed it in r103301.

Great!
 
I'm also interested in working on C++ PCH support in the next couple days.  What can I work on that won't conflict with your work?  If possible, I'd like to shoot for getting C++ PCH support working in the next week or two.

The remainder of the patch that Doug reviewed dealt with classes, constructors, destructors, and conversion operators needed a little work (to be loaded lazily I think). The next patch will definitely include read/write support for CXXBaseSpecifier and CXXBaseOrMemberInitializer. I cropped those out of this patch, but I don't think that there wasn't anything wrong with them.

I'll try to get the next patch out Monday.

I don't plan on touching the serialization of templates and template parameters The last time I tried to do something with it, I kept getting assertions that I shouldn't serialize dependent types or expressions. That was probably in December, though. Things may have changed since then :)

I haven't looked at any of the CXX expressions either.

Ok, sounds good.  I'll start simple with the CXX expressions!

-Chris

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev