Fix a marvelous chained AST writing bug, where we end up with the

following amusing sequence:
  - AST writing schedules writing a type X* that it had never seen
  before
  - AST writing starts writing another declaration, ends up
  deserializing X* from a prior AST file. Now we have two type IDs for
  the same type!
  - AST writer tries to write X*. It only has the lower-numbered ID
  from the the prior AST file, so references to the higher-numbered ID
  that was scheduled for writing go off into lalaland.

To fix this, keep the higher-numbered ID so we end up writing the type
twice. Since this issue occurs so rarely, and type records are
generally rather small, I deemed this better than the alternative: to
keep a separate mapping from the higher-numbered IDs to the
lower-numbered IDs, which we would end up having to check whenever we
want to deserialize any type.

Fixes <rdar://problem/8511624>, I think.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@115647 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp
index 8413faa65d62575f2595dd357d9db62c884df2fa..33c032dcaac02b076dd829e15fee09436ff37844 100644 (file)
--- a/lib/Serialization/ASTReader.cpp
+++ b/lib/Serialization/ASTReader.cpp
@@ -3115,6 +3115,9 @@ QualType ASTReader::GetType(TypeID ID) {
   assert(Index < TypesLoaded.size() && "Type index out-of-range");
   if (TypesLoaded[Index].isNull()) {
     TypesLoaded[Index] = ReadTypeRecord(Index);
+    if (TypesLoaded[Index].isNull())
+      return QualType();
+
     TypesLoaded[Index]->setFromAST();
     TypeIdxs[TypesLoaded[Index]] = TypeIdx::fromTypeID(ID);
     if (DeserializationListener)

diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp
index 7e0cdf41d7e9d6acef39d94d2b111e20367c188e..f26731caea1e32c67ba91c52fb74b4ca7b6697be 100644 (file)
--- a/lib/Serialization/ASTWriter.cpp
+++ b/lib/Serialization/ASTWriter.cpp
@@ -1419,10 +1419,7 @@ void ASTWriter::WriteType(QualType T) {
   if (Idx.getIndex() == 0) // we haven't seen this type before.
     Idx = TypeIdx(NextTypeID++);
 
-  // If this type comes from a previously-loaded PCH/AST file, don't try to
-  // write the type again.
-  if (Idx.getIndex() < FirstTypeID)
-    return;
+  assert(Idx.getIndex() >= FirstTypeID && "Re-writing a type from a prior AST");
 
   // Record the offset for this type.
   unsigned Index = Idx.getIndex() - FirstTypeID;
@@ -2872,7 +2869,7 @@ DeclID ASTWriter::GetDeclRef(const Decl *D) {
   if (D == 0) {
     return 0;
   }
-
+  assert(!(reinterpret_cast<uintptr_t>(D) & 0x01) && "Invalid decl pointer");
   DeclID &ID = DeclIDs[D];
   if (ID == 0) {
     // We haven't seen this declaration before. Give it a new ID and
@@ -3130,7 +3127,14 @@ void ASTWriter::IdentifierRead(IdentID ID, IdentifierInfo *II) {
 }
 
 void ASTWriter::TypeRead(TypeIdx Idx, QualType T) {
-  TypeIdxs[T] = Idx;
+  // Always take the highest-numbered type index. This copes with an interesting
+  // case for chained AST writing where we schedule writing the type and then,
+  // later, deserialize the type from another AST. In this case, we want to 
+  // keep the higher-numbered entry so that we can properly write it out to
+  // the AST file.
+  TypeIdx &StoredIdx = TypeIdxs[T];
+  if (Idx.getIndex() >= StoredIdx.getIndex())
+    StoredIdx = Idx;
 }
 
 void ASTWriter::DeclRead(DeclID ID, const Decl *D) {

diff --git a/test/PCH/Inputs/chain-remap-types1.h b/test/PCH/Inputs/chain-remap-types1.h
new file mode 100644 (file)
index 0000000..d105489
--- /dev/null
+++ b/test/PCH/Inputs/chain-remap-types1.h
@@ -0,0 +1,10 @@
+@class X;
+
+struct Y {
+  X *my_X;
+};
+
+@interface X {
+}
+@property X *prop;
+@end

diff --git a/test/PCH/Inputs/chain-remap-types2.h b/test/PCH/Inputs/chain-remap-types2.h
new file mode 100644 (file)
index 0000000..55ca8a9
--- /dev/null
+++ b/test/PCH/Inputs/chain-remap-types2.h
@@ -0,0 +1,8 @@
+void h(X*);
+
+@interface X (Blah) {
+}
+@end
+
+void g(X*);
+

diff --git a/test/PCH/chain-remap-types.m b/test/PCH/chain-remap-types.m
new file mode 100644 (file)
index 0000000..a45a79d
--- /dev/null
+++ b/test/PCH/chain-remap-types.m
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -emit-pch -x objective-c-header -o %t1 %S/Inputs/chain-remap-types1.h
+// RUN: %clang_cc1 -emit-pch -x objective-c-header -o %t2 %S/Inputs/chain-remap-types2.h -include-pch %t1 -chained-pch
+// RUN: %clang_cc1 -include-pch %t2 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -ast-print -include-pch %t2 %s | FileCheck %s
+
+// CHECK: @class X;
+// CHECK: struct Y 
+// CHECK: @property ( assign,readwrite ) X * prop
+// CHECK: void h(X *);
+// CHECK: @interface X(Blah)
+// CHECK: void g(X *);
+

diff --git a/tools/libclang/CIndexUSRs.cpp b/tools/libclang/CIndexUSRs.cpp
index d7135b94be964dc07e44d16fb08080c8c64e5b73..e22980df394521a5a48d55aeab82c0072f704085 100644 (file)
--- a/tools/libclang/CIndexUSRs.cpp
+++ b/tools/libclang/CIndexUSRs.cpp
@@ -699,7 +699,8 @@ void USRGenerator::VisitTemplateArgument(const TemplateArgument &Arg) {
     break;
 
   case TemplateArgument::Declaration:
-    Visit(Arg.getAsDecl());
+    if (Decl *D = Arg.getAsDecl())
+      Visit(D);
     break;
       
   case TemplateArgument::Template: