[clang][ASTImporter] Fix import of typedef with unnamed structures

[balazske]

Summary

Fix crash in ASTImporter related to import of unnamed structures and typedefs
to these maybe with pointer.
There was a series of problems exposed by https://reviews.llvm.org/D133468
(commit 69a6417406a1b0316a1fa6aeb63339d0e1d2abbd) in the ASTImporter breaking
cross-translation unit analysis. This change fixes one of the problems exposed
by that change for importing unnamed structures. The problem was
discovered when running clang static analysis on open source projects
using cross-translation unit analysis.

Simple test command. Produces crash without change, passes all tests
with change.

ninja ASTTests && ./tools/clang/unittests/AST/ASTTests
  --gtest_filter="*/*ImportAnonymousStruct/0"

Formatted crash stack:

ASTTests: <root>/clang/lib/AST/ASTContext.cpp:4787:
  clang::QualType clang::ASTContext::getTypedefType(const clang::TypedefNameDecl*,
  clang::QualType) const: Assertion `hasSameType(Decl->getUnderlyingType(), Underlying)' failed.
...
 #9 <addr> clang::ASTContext::getTypedefType(clang::TypedefNameDecl const*, clang::QualType) const
             <root>/clang/lib/AST/ASTContext.cpp:4789:26
             <root>/clang/lib/AST/ASTImporter.cpp:1374:71
             <root>/tools/clang/include/clang/AST/TypeNodes.inc:75:1
             <root>/clang/lib/AST/ASTImporter.cpp:8663:8

[vabridgers]

@balazske and I discussed, he will be commandeering this patch to improve.

[balazske]

My opinion is that we can not omit importing the "underlying type". The TypedefType has the type of the declaration (type of getDecl()) and the "underlying type" that may be different (this is the thing that comes from commit D133468). This is exactly different if TypedefType::typeMatchesDecl() (returns a stored value in TypedefType) returns true. In this case the type object is stored in the TypedefType node and is not the same as the type of declaration getDecl(). If function getTypeDeclType is used it creates the typedef type always with the type of getDecl() and the typeMatchesDecl will always return true for this type even if at the to-be-imported type it was false.

[vabridgers]

Hi @balazske , all LIT and unittests pass with this change. By your logic, then we are missing some LIT or unittest cases that support your statement. Can you think of a case that demonstrates this? Thanks!

[balazske]

The problem is somewhat bigger and not fast to fix. This test shows what is problematic:

TEST_P(ASTImporterOptionSpecificTestBase,
       ImportExistingTypedefToUnnamedRecordPtr) {
  const char *Code =
      R"(
      typedef const struct { int fff; } * const T;
      extern T x;
      )";
  Decl *ToTU = getToTuDecl(Code, Lang_C99);
  Decl *FromTU = getTuDecl(Code, Lang_C99);

  auto *FromX =
      FirstDeclMatcher<VarDecl>().match(FromTU, varDecl(hasName("x")));
  auto *ToX = Import(FromX, Lang_C99);
  EXPECT_TRUE(ToX);

  auto *Typedef1 =
      FirstDeclMatcher<TypedefDecl>().match(ToTU, typedefDecl(hasName("T")));
  auto *Typedef2 =
      LastDeclMatcher<TypedefDecl>().match(ToTU, typedefDecl(hasName("T")));
  EXPECT_EQ(Typedef1, Typedef2);
  
  auto *FromR =
      FirstDeclMatcher<RecordDecl>().match(FromTU, recordDecl(hasDescendant(fieldDecl(hasName("fff")))));
  auto *ToRExisting =
      FirstDeclMatcher<RecordDecl>().match(ToTU, recordDecl(hasDescendant(fieldDecl(hasName("fff")))));
  ASSERT_TRUE(FromR);
  auto *ToRImported = Import(FromR, Lang_C99);
  EXPECT_EQ(ToRExisting, ToRImported);
}

The test fails because the last EXPECT_EQ fails (without the fix in this patch it crashes with the hasSameType assertion): The record is imported separately but the typedef was already imported before with a different record (in fact not imported, the import returned the existing typedef, this is caused by the existence of PointerType or any other type that has reference to unnamed struct but is not a RecordType at the typedef). The ToTU AST contains then two instances of the unnamed record (the last one is created when the second import in the test happens). The problem can be solved probably only if the import of existing equivalent AST nodes from another TU (in the root context, not in namespace with no name) is changed: The import should return the existing one and not create a new. I do not know how difficult is to change this.
But until a better fix is found the current solution is acceptable (with FIXME included).

[balazske]

Added a simple test and another test.
Instead of calling getTypeDeclType only the reuse of existing type is done
(this was the part that fixes the problem). In this way the underlying type
is still imported. The result is not correct but fixes some crash.

[vabridgers]

Hi @balazske . Can we go back to the original proposed fix and treat the new issue separately? We have an internal crash open that is corrected by the original patch I proposed, and passes all LITs and unit tests. I think it would be better to separate these concerns so we can move forward.
Thanks

[balazske]

In D145868#4189574, @vabridgers wrote:

Hi @balazske , all LIT and unittests pass with this change. By your logic, then we are missing some LIT or unittest cases that support your statement. Can you think of a case that demonstrates this? Thanks!

The test ImportTypedefWithDifferentUnderlyingType was added for this purpose.

[vabridgers]

@balazske , I tested these changes - again - and it all seems to work for me. I would have preferred we do not see the build status failures before doing this, but maybe we can be sure to avoid this next time (because I'll want to test again before our final merge). Please move ahead with your changes and let us know when you're ready for final review.

[balazske]

ping
This is a solution for a problem at least until a better fix is found.

[donat.nagy]

This commit eliminates crashes caused by a rare corner case (typedefs to types derived from unnamed structs) of the AST import procedure; but introduces some incorrect behavior in a rare sub-case of of this corner case (which is explored by the testcases added by this commit). I think this is a good trade (one surprising behavior is less bad than 10 crashes) and it's worth to merge this simple change in its current state. However, it would be important to continue this project and create a separate followup commit that fixes the remaining little issues in this area, because if we don't handle them now, they might cause unpleasant surprises later.

[bjope]

clang/unittests/AST/ASTImporterTest.cpp

8627

With Werror we get:

../../clang/unittests/AST/ASTImporterTest.cpp:8627:9: error: unused variable 'ToTU' [-Werror,-Wunused-variable]
  Decl *ToTU = getToTuDecl("", Lang_CXX11);
        ^

[balazske]

clang/unittests/AST/ASTImporterTest.cpp

8627

Problem was fixed in 1689a5d756f81d615bcfca8de2631b0ccc8a3917.

[bjope]

clang/unittests/AST/ASTImporterTest.cpp

8627

Great. Thanks!

clang/lib/AST/ASTImporter.cpp

ExpectedType ASTNodeImporter::VisitParenType(const ParenType *T) {
 
   return Importer.getToContext().getParenType(*ToInnerTypeOrErr);
 }
 
 ExpectedType ASTNodeImporter::VisitTypedefType(const TypedefType *T) {
   Expected<TypedefNameDecl *> ToDeclOrErr = import(T->getDecl());
   if (!ToDeclOrErr)
     return ToDeclOrErr.takeError();
+
+  TypedefNameDecl *ToDecl = *ToDeclOrErr;
+  if (ToDecl->getTypeForDecl())
+    return QualType(ToDecl->getTypeForDecl(), 0);
+
   ExpectedType ToUnderlyingTypeOrErr = import(T->desugar());
   if (!ToUnderlyingTypeOrErr)
     return ToUnderlyingTypeOrErr.takeError();
 
-  return Importer.getToContext().getTypedefType(*ToDeclOrErr,
-                                                *ToUnderlyingTypeOrErr);
+  return Importer.getToContext().getTypedefType(ToDecl, *ToUnderlyingTypeOrErr);
 }
 
 ExpectedType ASTNodeImporter::VisitTypeOfExprType(const TypeOfExprType *T) {
   ExpectedExpr ToExprOrErr = import(T->getUnderlyingExpr());
   if (!ToExprOrErr)
     return ToExprOrErr.takeError();
   return Importer.getToContext().getTypeOfExprType(*ToExprOrErr, T->getKind());
 }

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, ImportTwoTypedefsToUnnamedRecord) {
   auto *Typedef1 =
       FirstDeclMatcher<TypedefDecl>().match(ToTU, typedefDecl(hasName("T1")));
   auto *Typedef2 =
       FirstDeclMatcher<TypedefDecl>().match(ToTU, typedefDecl(hasName("T2")));
   EXPECT_NE(Typedef1->getUnderlyingType().getTypePtr(),
             Typedef2->getUnderlyingType().getTypePtr());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+       ImportExistingTypedefToUnnamedRecordPtr) {
+  const char *Code =
+      R"(
+      typedef const struct { int fff; } * const T;
+      extern T x;
+      )";
+  Decl *ToTU = getToTuDecl(Code, Lang_C99);
+  Decl *FromTU = getTuDecl(Code, Lang_C99);
+
+  auto *FromX =
+      FirstDeclMatcher<VarDecl>().match(FromTU, varDecl(hasName("x")));
+  auto *ToX = Import(FromX, Lang_C99);
+  EXPECT_TRUE(ToX);
+
+  auto *Typedef1 =
+      FirstDeclMatcher<TypedefDecl>().match(ToTU, typedefDecl(hasName("T")));
+  auto *Typedef2 =
+      LastDeclMatcher<TypedefDecl>().match(ToTU, typedefDecl(hasName("T")));
+  // FIXME: These should be imported separately, like in the test above.
+  // Or: In the test above these should be merged too.
+  EXPECT_EQ(Typedef1, Typedef2);
+
+  auto *FromR = FirstDeclMatcher<RecordDecl>().match(
+      FromTU, recordDecl(hasDescendant(fieldDecl(hasName("fff")))));
+  auto *ToRExisting = FirstDeclMatcher<RecordDecl>().match(
+      ToTU, recordDecl(hasDescendant(fieldDecl(hasName("fff")))));
+  ASSERT_TRUE(FromR);
+  auto *ToRImported = Import(FromR, Lang_C99);
+  // FIXME: If typedefs are not imported separately, do not import ToRImported
+  // separately.
+  EXPECT_NE(ToRExisting, ToRImported);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase,
+       ImportTypedefWithDifferentUnderlyingType) {
+  const char *Code =
+      R"(
+      using X1 = int;
+      using Y1 = int;
+
+      using RPB1 = X1*;
+      typedef RPB1 RPX1;
+      using RPB1 = Y1*; // redeclared
+      typedef RPB1 RPY1;
+
+      auto X = 0 ? (RPX1){} : (RPY1){};
+      )";
+  Decl *ToTU = getToTuDecl("", Lang_CXX11);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+
+  auto *FromX =
+      FirstDeclMatcher<VarDecl>().match(FromTU, varDecl(hasName("X")));
+
+  auto *FromXType = FromX->getType()->getAs<TypedefType>();
+  EXPECT_FALSE(FromXType->typeMatchesDecl());
+
+  auto *ToX = Import(FromX, Lang_CXX11);
+  auto *ToXType = ToX->getType()->getAs<TypedefType>();
+  // FIXME: This should be false.
+  EXPECT_TRUE(ToXType->typeMatchesDecl());
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportPath,
                          ::testing::Values(std::vector<std::string>()));
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportExpr,
                          DefaultTestValuesForRunOptions);