[PATCH] Fix PR18307: Properly (de)serialize inherited constructors and their using declarations

Stephan Tolksdorf st at quanttec.com
Mon Mar 17 12:41:02 PDT 2014


Hi rsmith,

Before this patch there were two problems with the serialization and deserialization of inherited constructors in AstWriter and AstReader.

First, the inherited constructor of a CxxConstructorDecl wasn't (de)serialized.

Second, the constructors of a class and the using declarations for constructors of base classes had different DeclarationNames but identical keys in the OnDiskHashTable. This lead to WriteDeclContextVisibleBlock writing multiple entries with the same key into the OnDiskHashTable, violating an expected invariant. (Whether this issue alone actually triggers a compilation error depends on the (somewhat unstable) order in which WriteDeclContextVisibleBlock iterates over the declarations in the DeclContext.

I fixed the second issue by letting WriteDeclContextVisibleBlock treat CXXConstructorNames like CXXConversionFunctionNames. I'm not overly fond of this approach and would appreciate suggestions on how to do this better (without requiring major refactorings).

A comment that I rewrote in WriteDeclContextVisibleBlock originally stated that type information wasn't used in the key "since such type information is not stable across different modules". I'm not sure whether this statement is (still) accurate and should be preserved in the new comment.





http://llvm-reviews.chandlerc.com/D3102

Files:
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/PCH/pr18307.cpp

Index: lib/Serialization/ASTReaderDecl.cpp
===================================================================
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1348,7 +1348,9 @@
 
 void ASTDeclReader::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
   VisitCXXMethodDecl(D);
-  
+
+  if (auto *CD = ReadDeclAs<CXXConstructorDecl>(Record, Idx))
+    D->setInheritedConstructor(CD);
   D->IsExplicitSpecified = Record[Idx++];
   std::tie(D->CtorInitializers, D->NumCtorInitializers) =
       Reader.ReadCXXCtorInitializers(F, Record, Idx);
Index: lib/Serialization/ASTWriter.cpp
===================================================================
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -3429,30 +3429,46 @@
   ASTDeclContextNameLookupTrait Trait(*this);
 
   // Create the on-disk hash table representation.
+  DeclarationName ConstructorName;
   DeclarationName ConversionName;
+  SmallVector<NamedDecl *, 8> ConstructorDecls;
   SmallVector<NamedDecl *, 4> ConversionDecls;
   for (StoredDeclsMap::iterator D = Map->begin(), DEnd = Map->end();
        D != DEnd; ++D) {
     DeclarationName Name = D->first;
     DeclContext::lookup_result Result = D->second.getLookupResult();
     if (!Result.empty()) {
-      if (Name.getNameKind() == DeclarationName::CXXConversionFunctionName) {
-        // Hash all conversion function names to the same name. The actual
-        // type information in conversion function name is not used in the
-        // key (since such type information is not stable across different
-        // modules), so the intended effect is to coalesce all of the conversion
-        // functions under a single key.
+      // Different DeclarationName values of certain kinds are mapped to
+      // identical serialized keys, because we don't to use type identifiers
+      // in the keys.
+      switch (Name.getNameKind()) {
+      case DeclarationName::CXXConstructorName:
+        // There may be different CXXConstructorName DeclarationName values
+        // in a DeclContext because a UsingDecl that inherits constructors
+        // has the DeclarationName of the inherited constructors.
+        if (!ConstructorName)
+          ConstructorName = Name;
+        ConstructorDecls.append(Result.begin(), Result.end());
+        continue;
+      case DeclarationName::CXXConversionFunctionName:
         if (!ConversionName)
           ConversionName = Name;
         ConversionDecls.append(Result.begin(), Result.end());
         continue;
+      default:
+        break;
       }
-      
       Generator.insert(Name, Result, Trait);
     }
   }
-
-  // Add the conversion functions
+  // Add the constructors.
+  if (!ConstructorDecls.empty()) {
+    Generator.insert(ConstructorName,
+                     DeclContext::lookup_result(ConstructorDecls.begin(),
+                                                ConstructorDecls.end()),
+                     Trait);
+  }
+  // Add the conversion functions.
   if (!ConversionDecls.empty()) {
     Generator.insert(ConversionName, 
                      DeclContext::lookup_result(ConversionDecls.begin(),
Index: lib/Serialization/ASTWriterDecl.cpp
===================================================================
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -1005,6 +1005,7 @@
 void ASTDeclWriter::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
   VisitCXXMethodDecl(D);
 
+  Writer.AddDeclRef(D->getInheritedConstructor(), Record);
   Record.push_back(D->IsExplicitSpecified);
   Writer.AddCXXCtorInitializers(D->CtorInitializers, D->NumCtorInitializers,
                                 Record);
Index: test/PCH/pr18307.cpp
===================================================================
--- /dev/null
+++ test/PCH/pr18307.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -std=c++11 -emit-pch -o %t %s
+// RUN: %clang_cc1 -std=c++11 -include-pch %t -verify %s
+
+// expected-no-diagnostics
+
+#ifndef HEADER_INCLUDED
+#define HEADER_INCLUDED
+
+struct Base {
+  Base(int) {}
+};
+
+struct Test : Base {
+  using Base::Base;
+};
+
+#else
+
+Test test(42);
+
+#endif // HEADER_INCLUDED
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D3102.1.patch
Type: text/x-patch
Size: 4176 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140317/7afdeaee/attachment.bin>


More information about the cfe-commits mailing list