[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145868/new/

https://reviews.llvm.org/D145868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145868/new/

https://reviews.llvm.org/D145868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145868/new/

https://reviews.llvm.org/D145868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 505481.
balazske added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145868/new/

https://reviews.llvm.org/D145868

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8549,6 +8549,69 @@
 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().match(FromTU, varDecl(hasName("x")));
+  auto *ToX = Import(FromX, Lang_C99);
+  EXPECT_TRUE(ToX);
+
+  auto *Typedef1 =
+  FirstDeclMatcher().match(ToTU, typedefDecl(hasName("T")));
+  auto *Typedef2 =
+  LastDeclMatcher().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().match(
+  FromTU, recordDecl(hasDescendant(fieldDecl(hasName("fff");
+  auto *ToRExisting = FirstDeclMatcher().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().match(FromTU, varDecl(hasName("X")));
+
+  auto *FromXType = FromX->getType()->getAs();
+  EXPECT_FALSE(FromXType->typeMatchesDecl());
+
+  auto *ToX = Import(FromX, Lang_CXX11);
+  auto *ToXType = ToX->getType()->getAs();
+  // FIXME: This should be false.
+  EXPECT_TRUE(ToXType->typeMatchesDecl());
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1363,12 +1363,16 @@
   Expected 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) {


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8549,6 +8549,69 @@
 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().match(FromTU, varDecl(hasName("x")));
+  auto *ToX = Import(FromX, Lang_C99);
+  EXPECT_TRUE(ToX);
+
+  auto *Typedef1 =
+  FirstDeclMatcher().match(ToTU, typedefDecl(hasName("T")));
+  auto *Typedef2 =
+  LastDeclMatcher().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().match(
+  

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

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().match(FromTU, varDecl(hasName("x")));
auto *ToX = Import(FromX, Lang_C99);
EXPECT_TRUE(ToX);
  
auto *Typedef1 =
FirstDeclMatcher().match(ToTU, typedefDecl(hasName("T")));
auto *Typedef2 =
LastDeclMatcher().match(ToTU, typedefDecl(hasName("T")));
EXPECT_EQ(Typedef1, Typedef2);

auto *FromR =
FirstDeclMatcher().match(FromTU, 
recordDecl(hasDescendant(fieldDecl(hasName("fff");
auto *ToRExisting =
FirstDeclMatcher().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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145868/new/

https://reviews.llvm.org/D145868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145868/new/

https://reviews.llvm.org/D145868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Balázs Kéri via Phabricator via cfe-commits]

balazske commandeered this revision.
balazske edited reviewers, added: vabridgers; removed: balazske.
balazske added a comment.
Herald added subscribers: steakhal, Szelethus, dkrupp.

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 `TypedefDecl`) 
returns true. In this case the type object is stored in the `TypedefDecl` 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145868/new/

https://reviews.llvm.org/D145868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145868/new/

https://reviews.llvm.org/D145868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers created this revision.
vabridgers added reviewers: a.sidorin, shafik, donat.nagy, gamesh411, balazske.
Herald added a subscriber: martong.
Herald added a project: All.
vabridgers requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix crash in ASTImporter for anonymous structures. 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 the problem exposed
by that change for importing anonymous 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: /clang/lib/AST/ASTContext.cpp:4787:

  clang::QualType clang::ASTContext::getTypedefType(const 
clang::TypedefNameDecl*,
  clang::QualType) const: Assertion `hasSameType(Decl->getUnderlyingType(), 
Underlying)' failed.

...
 #9  clang::ASTContext::getTypedefType(clang::TypedefNameDecl const*, 
clang::QualType) const

  /clang/lib/AST/ASTContext.cpp:4789:26
  /clang/lib/AST/ASTImporter.cpp:1374:71
  /tools/clang/include/clang/AST/TypeNodes.inc:75:1
  /clang/lib/AST/ASTImporter.cpp:8663:8


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145868

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8474,6 +8474,45 @@
   ToVaList->getUnderlyingType(), ToBuiltinVaList->getUnderlyingType()));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportAnonymousStruct) {
+  // Tests importing of anonymous structures. This is a reduction of a real
+  // issue found in the wild into unittest case form. This crashes without
+  // the fix. Just don't crash when this unittest case is run.
+  Decl *ToTU = getToTuDecl(
+  R"(  
+  typedef struct _XDisplay Display;
+  typedef struct {
+int var;
+  } *_XPrivDisplay;
+  extern Display  *xterm_dpy;
+  int function(int);
+  int internal(Display *dpy) {
+return function(((_XPrivDisplay)dpy)->var );
+  }
+  )",
+  Lang_C99);
+  auto *ToX = FirstDeclMatcher().match(
+  ToTU, functionDecl(hasName("internal"), isDefinition()));
+  ASSERT_TRUE(ToX);
+
+  Decl *FromTU = getTuDecl(
+  R"(  
+  typedef struct _XDisplay Display;
+  typedef struct {
+int var;
+  } *_XPrivDisplay;
+  extern Display  *xterm_dpy;
+  int function(int var) {
+return ((_XPrivDisplay)xterm_dpy)->var;
+  }
+  )",
+  Lang_C99, "input1.c");
+  auto *FromF = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("function"), isDefinition()));
+  auto *ToF = Import(FromF, Lang_C99);
+  EXPECT_TRUE(ToF);
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1363,12 +1363,7 @@
   Expected ToDeclOrErr = import(T->getDecl());
   if (!ToDeclOrErr)
 return ToDeclOrErr.takeError();
-  ExpectedType ToUnderlyingTypeOrErr = import(T->desugar());
-  if (!ToUnderlyingTypeOrErr)
-return ToUnderlyingTypeOrErr.takeError();
-
-  return Importer.getToContext().getTypedefType(*ToDeclOrErr,
-*ToUnderlyingTypeOrErr);
+  return Importer.getToContext().getTypeDeclType(*ToDeclOrErr);
 }
 
 ExpectedType ASTNodeImporter::VisitTypeOfExprType(const TypeOfExprType *T) {


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8474,6 +8474,45 @@
   ToVaList->getUnderlyingType(), ToBuiltinVaList->getUnderlyingType()));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportAnonymousStruct) {
+  // Tests importing of anonymous structures. This is a reduction of a real
+  // issue found in the wild into unittest case form. This crashes without
+  // the fix. Just don't crash when this unittest case is run.
+  Decl *ToTU = getToTuDecl(
+  R"(  
+  typedef struct _XDisplay Display;
+  typedef struct {
+int var;
+  } *_XPrivDisplay;
+  extern Display  *xterm_dpy;
+  int function(int);
+  int internal(Display *dpy) {
+return