This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by balazske on Mar 12 2023, 6:03 AM.

Details

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

Diff Detail

Event Timeline

vabridgers created this revision.Mar 12 2023, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 6:03 AM
Herald added a subscriber: martong. · View Herald Transcript
vabridgers requested review of this revision.Mar 12 2023, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 6:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vabridgers edited the summary of this revision. (Show Details)Mar 12 2023, 6:42 AM
vabridgers edited the summary of this revision. (Show Details)Mar 12 2023, 6:43 AM

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

vabridgers planned changes to this revision.Mar 13 2023, 4:04 AM
balazske commandeered this revision.EditedMar 13 2023, 4:59 AM
balazske edited reviewers, added: vabridgers; removed: 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.

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 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 updated this revision to Diff 505481.Mar 15 2023, 7:21 AM

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.

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

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.

@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 retitled this revision from [clang][ASTImporter] Fix import of anonymous structures to [clang][ASTImporter] Fix import of typedef with unnamed structures.Mar 27 2023, 6:24 AM
balazske edited the summary of this revision. (Show Details)

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

donat.nagy accepted this revision.Apr 11 2023, 1:25 AM

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.

This revision is now accepted and ready to land.Apr 11 2023, 1:25 AM
bjope added a subscriber: bjope.Apr 13 2023, 1:14 AM
bjope added inline comments.
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 marked an inline comment as done.Apr 13 2023, 5:23 AM
balazske added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
8627
bjope added inline comments.Apr 13 2023, 7:02 AM
clang/unittests/AST/ASTImporterTest.cpp
8627

Great. Thanks!