[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM

Thank you for adding the additional test.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53699



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

[PATCH] D56581: [ASTImporter] Set the described template if not set

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Can you add a test?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56581



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

[PATCH] D56651: [ASTImporter] Fix importing OperatorDelete from CXXConstructorDecl

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Thank you, this looks good but perhaps some refactoring would help improve the 
change.




Comment at: lib/AST/ASTImporter.cpp:3243
+
+  if (R) {
+CXXDestructorDecl *ToDtor = cast(*R);

a_sidorin wrote:
> It's better to move this code to VisitFunctionDecl to keep all related stuff 
> together.
I am not sure I agree. Having everything in `VisitFunctionDecl` makes this code 
harder to maintain. 





Comment at: lib/AST/ASTImporter.cpp:3246
+
+auto Imp = importSeq(const_cast(D->getOperatorDelete()),
+ D->getOperatorDeleteThisArg());

a_sidorin wrote:
> Moving this code to VisitFunctionDecl will also allow us not to create a dtor 
> if this import fails.
At what point in `VisitFunctionDecl` would you expect this change to bail out 
of? A possible refactor would be to move this code to a separate function and 
then call that function at that point in `VisitFunctionDecl`. This would 
prevent `VisitFunctionDecl` from becoming even larger and I think address your 
concern.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56651



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

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: martong, teemperor, balazske, aaron.ballman.
Herald added a subscriber: rnkovacs.

When importing classes we may add a CXXMethodDecl more than once to a 
CXXRecordDecl. This patch will fix the cases we currently know about and handle 
both the case where we are only dealing with declarations and when we have to 
merge a definition into an existing declaration.


https://reviews.llvm.org/D56936

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

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2233,6 +2233,174 @@
 }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, ImportOverriddenMethodTwice) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto BP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher().match(FromTU0, DP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher().match(FromTU1, BP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DP), 1u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceDefinitionFirst) {
+  auto CodeWithOutDef =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeWithDef =
+  R"(
+struct B { virtual void f(){}; };
+struct D:B { void f(){}; };
+  )";
+  auto BP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+  auto BFDef = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFDef = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto FDefAll = cxxMethodDecl(hasName("f"), isDefinition());
+
+  Decl *ImportedD;
+  {
+Decl *FromTU = getTuDecl(CodeWithDef, Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(FromTU, DP);
+ImportedD = Import(FromD, Lang_CXX);
+  }
+  {
+Decl *FromTU = getTuDecl(CodeWithOutDef, Lang_CXX, "input1.cc");
+auto *FromB = FirstDeclMatcher().match(FromTU, BP);
+Import(FromB, Lang_CXX);
+  }
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFDef), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFDef), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, FDefAll), 2u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDef) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){};
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto BFPIsDef = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFPIsDef), 0u);
+
+  auto *ToB = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("B")));
+  auto *ToBFInClass = FirstDeclMatcher().match(ToTU, BFP);
+  auto *ToBFOutOfClass = FirstDeclMatcher().match(
+  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+
+  // The definition should be out-of-class.
+  EXPECT_NE(ToBFInClass, ToBFOutOfClass);
+  EXPECT_NE(ToBFInClass->getLexicalDeclContext(),
+ToBFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToBFOutOfClass->getDeclContext(), ToB);
+  EXPECT_EQ(ToBFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  // Check that the redecl chain is intact.
+  EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDefTwoTUs) {
+  auto CodeTU0 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeTU1 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){}
+  void D::f(){}
+  void foo(B &b) { b.f(); }
+  )";
+
+  auto BFP =
+  cxxM

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@martong the only issue is that I am seeing a regression on 
`Analysis/ctu-main.cpp` when I run `check-clang`. I am going to look into it 
but if you have any insights that would be helpful.


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

https://reviews.llvm.org/D56936



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

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked 17 inline comments as done.
shafik added a comment.

@martong I don't follow the discussion on `VisitParmVarDecl` an what seems like 
an alternate fix. Can you elaborate?




Comment at: lib/AST/ASTImporter.cpp:3042
 
+  if (FoundByLookup) {
+if (auto *MD = dyn_cast(FoundByLookup)) {

martong wrote:
> It is not trivial why we add this block to the code, so I think a comment 
> would be really appreciated here.
> I was thinking about something like this:
> ```
> +  // We do not allow more than one in-class prototype of a function.  This is
> +  // because AST clients like VTableBuilder asserts on this.  VTableBuilder
> +  // assumes that only one function can override a function. Building a 
> redecl
> +  // chain would result there are more than one function which override the
> +  // base function (even if they are part of the same redecl chain inside the
> +  // derived class.)
> ```
Just for clarification, from a C++ perspective, I would say "in-class 
declaration" rather than "in-class prototype" and therefore `VTableBuilder` 
assumes only one in class declaration and building a redecal chain would result 
in more than one in-class declaration being present. 

Does that makes sense?



Comment at: unittests/AST/ASTImporterTest.cpp:2344
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDefTwoTUs) {
+  auto CodeTU0 =

balazske wrote:
> The previous tests use two TUs too so the name for this is not exact (here 
> the code is different, maybe use 
> `ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode`?). This test does 
> roughly the same as `ImportOverriddenMethodTwiceDefinitionFirst` but with the 
> definition last and out-of-class version. (Name can be 
> `ImportOverriddenMethodTwiceOutOfClassDefLast` too.) I am not sure why is the 
> `foo` used here (`B::F` can be imported instead).
I originally thought I did not need `foo` either but I could get it to import 
the definition of `B::f()` any other way.

I also saw similar behavior with `clang-import-test` where I need an expression 
that included something like `foo()` in order to obtain the out-of-line 
definition. It is not obvious to me why.

So in this case if I switch to using `BFP` instead of `FooDef` then this fails:

auto *ToBFOutOfClass = FirstDeclMatcher().match(
  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));




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

https://reviews.llvm.org/D56936



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

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 183251.
shafik added a comment.

Addressing comments on naming in the unit test and refactoring of duplicated 
code.


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

https://reviews.llvm.org/D56936

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

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2233,6 +2233,187 @@
 }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, ImportOverriddenMethodTwice) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto BFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *DF = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(DF, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *BF = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(BF, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceDefinitionFirst) {
+  auto CodeWithoutDef =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeWithDef =
+  R"(
+struct B { virtual void f(){}; };
+struct D:B { void f(){}; };
+  )";
+  auto BFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+  auto BFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), isDefinition());
+  auto FDefAllP = cxxMethodDecl(hasName("f"), isDefinition());
+
+  {
+Decl *FromTU = getTuDecl(CodeWithDef, Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(FromTU, DFP);
+Import(FromD, Lang_CXX);
+  }
+  {
+Decl *FromTU = getTuDecl(CodeWithoutDef, Lang_CXX, "input1.cc");
+auto *FromB = FirstDeclMatcher().match(FromTU, BFP);
+Import(FromB, Lang_CXX);
+  }
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, FDefAllP), 2u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDef) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){};
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto BFPIsDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), unless(isDefinition()) );
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFPIsDefP), 0u);
+
+  auto *ToB = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("B")));
+  auto *ToBFInClass = FirstDeclMatcher().match(ToTU, BFP);
+  auto *ToBFOutOfClass = FirstDeclMatcher().match(
+  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+
+  // The definition should be out-of-class.
+  EXPECT_NE(ToBFInClass, ToBFOutOfClass);
+  EXPECT_NE(ToBFInClass->getLexicalDeclContext(),
+ToBFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToBFOutOfClass->getDeclContext(), ToB);
+  EXPECT_EQ(ToBFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  // Check that the redecl chain is intact.
+  EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode) {
+  auto CodeTU0 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeTU1 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){}
+  void D::f(){}
+  void foo(B &b, D &d) { b.f(); d.f(); }
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto BFPIsDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B")))

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@martong so I just tested your `lit-tests.patch` and it looks good!




Comment at: lib/AST/ASTImporter.cpp:2959
+}
+
 ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {

balazske wrote:
> Code formatting is not OK in this function (and other places). (Indentation 
> and spaces around "(" ")".)
Apologies, I forgot to run clang-format


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

https://reviews.llvm.org/D56936



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

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 183406.
shafik marked 3 inline comments as done.
shafik added a comment.

Changes based on comments

- Refactoring ImportFunctionDeclBody to return Error
- Refactoring test/Analysis/ctu-main.cpp based on Gabor's patch
- Removing merging of function body for later PR


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

https://reviews.llvm.org/D56936

Files:
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/ctu-main.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2233,6 +2233,191 @@
 }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, ImportOverriddenMethodTwice) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *DF = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(DF, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *BF = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(BF, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceDefinitionFirst) {
+  auto CodeWithoutDef =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeWithDef =
+  R"(
+struct B { virtual void f(){}; };
+struct D:B { void f(){}; };
+  )";
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+  auto BFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), isDefinition());
+  auto FDefAllP = cxxMethodDecl(hasName("f"), isDefinition());
+
+  {
+Decl *FromTU = getTuDecl(CodeWithDef, Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(FromTU, DFP);
+Import(FromD, Lang_CXX);
+  }
+  {
+Decl *FromTU = getTuDecl(CodeWithoutDef, Lang_CXX, "input1.cc");
+auto *FromB = FirstDeclMatcher().match(FromTU, BFP);
+Import(FromB, Lang_CXX);
+  }
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, FDefAllP), 2u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDef) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){};
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto BFPIsDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))),
+   unless(isDefinition()));
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFPIsDefP), 0u);
+
+  auto *ToB = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("B")));
+  auto *ToBFInClass = FirstDeclMatcher().match(ToTU, BFP);
+  auto *ToBFOutOfClass = FirstDeclMatcher().match(
+  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+
+  // The definition should be out-of-class.
+  EXPECT_NE(ToBFInClass, ToBFOutOfClass);
+  EXPECT_NE(ToBFInClass->getLexicalDeclContext(),
+ToBFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToBFOutOfClass->getDeclContext(), ToB);
+  EXPECT_EQ(ToBFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  // Check that the redecl chain is intact.
+  EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
+}
+
+TEST_P(ImportFunctions,
+   ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode) {
+  auto CodeTU0 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeTU1 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B

[PATCH] D56581: [ASTImporter] Set the described template if not set

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@martong Unfortunately this causes a regression on one of the lldb tests 
`TestDataFormatterLibcxxVector.py` e.g.

  lldb-dotest -p TestDataFormatterLibcxxVector.py -t -G gmodules 
--no-multiprocess 

So this is specific to modules. If you can't reproduce this I can provide a 
back-trace.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56581



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

[PATCH] D56581: [ASTImporter] Set the described template if not set

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a subscriber: aprantl.
shafik added a comment.

I don't believe it is directly related to modules but that it just triggers the 
issue because it may be bringing in more. You can find a description here 
https://clang.llvm.org/docs/CommandGuide/clang.html#cmdoption-g and @aprantl 
pointed me to this talk if you want to even more 
http://llvm.org/devmtg/2015-10/#talk19


Repository:
  rC Clang

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

https://reviews.llvm.org/D56581



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

[PATCH] D57232: [ASTImporter] Check visibility/linkage of functions and variables

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: lib/AST/ASTImporter.cpp:2954
+return Found->hasExternalFormalLinkage();
+  else if (Importer.GetFromTU(Found) == From->getTranslationUnitDecl()) {
+if (From->isInAnonymousNamespace())

We don't really need an `else ` here and it would be more like an early exit 
which is what llvm style guide suggests.

In the same vein I would do:

if (Importer.GetFromTU(Found) != From->getTranslationUnitDecl())
   return false; 

so a second early exit and remove a level of nesting at the same time.



Comment at: unittests/AST/ASTImporterTest.cpp:65
 // -fms-compatibility or -fdelayed-template-parsing.
-struct ParameterizedTestsFixture : ::testing::TestWithParam {
+class CompilerOptionSpecificTest : public ::testing::Test {
+protected:

Are these changes directly related to the visibility change? There is a lot of 
noise that is not obviously related to the description in the PR.


If not maybe it should be a separate PR?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57232



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

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 183923.
shafik marked 2 inline comments as done.
shafik added a comment.

Addressing comments on commenting and naming.


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

https://reviews.llvm.org/D56936

Files:
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/ctu-main.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2233,6 +2233,191 @@
 }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, ImportOverriddenMethodTwice) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *DF = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(DF, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *BF = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(BF, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceDefinitionFirst) {
+  auto CodeWithoutDef =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeWithDef =
+  R"(
+struct B { virtual void f(){}; };
+struct D:B { void f(){}; };
+  )";
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+  auto BFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), isDefinition());
+  auto FDefAllP = cxxMethodDecl(hasName("f"), isDefinition());
+
+  {
+Decl *FromTU = getTuDecl(CodeWithDef, Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(FromTU, DFP);
+Import(FromD, Lang_CXX);
+  }
+  {
+Decl *FromTU = getTuDecl(CodeWithoutDef, Lang_CXX, "input1.cc");
+auto *FromB = FirstDeclMatcher().match(FromTU, BFP);
+Import(FromB, Lang_CXX);
+  }
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, FDefAllP), 2u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDef) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){};
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto BFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))),
+   unless(isDefinition()));
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFDefP), 0u);
+
+  auto *ToB = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("B")));
+  auto *ToBFInClass = FirstDeclMatcher().match(ToTU, BFP);
+  auto *ToBFOutOfClass = FirstDeclMatcher().match(
+  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+
+  // The definition should be out-of-class.
+  EXPECT_NE(ToBFInClass, ToBFOutOfClass);
+  EXPECT_NE(ToBFInClass->getLexicalDeclContext(),
+ToBFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToBFOutOfClass->getDeclContext(), ToB);
+  EXPECT_EQ(ToBFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  // Check that the redecl chain is intact.
+  EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
+}
+
+TEST_P(ImportFunctions,
+   ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode) {
+  auto CodeTU0 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeTU1 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){}
+  void D::f(){}
+  void foo(B &b, D &d) { b.f(); d.f(); }
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRec

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352436: [ASTImporter] Fix handling of overriden methods 
during ASTImport (authored by shafik, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D56936

Files:
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/ctu-main.cpp
  unittests/AST/ASTImporterTest.cpp

Index: test/Analysis/Inputs/ctu-other.cpp
===
--- test/Analysis/Inputs/ctu-other.cpp
+++ test/Analysis/Inputs/ctu-other.cpp
@@ -24,33 +24,38 @@
 int fens(int x) {
   return x - 3;
 }
-}
+} // namespace embed_ns
 
 class embed_cls {
 public:
-  int fecl(int x) {
-return x - 7;
-  }
+  int fecl(int x);
 };
+int embed_cls::fecl(int x) {
+  return x - 7;
 }
+} // namespace myns
 
 class mycls {
 public:
-  int fcl(int x) {
-return x + 5;
-  }
-  static int fscl(int x) {
-return x + 6;
-  }
+  int fcl(int x);
+  static int fscl(int x);
 
   class embed_cls2 {
   public:
-int fecl2(int x) {
-  return x - 11;
-}
+int fecl2(int x);
   };
 };
 
+int mycls::fcl(int x) {
+  return x + 5;
+}
+int mycls::fscl(int x) {
+  return x + 6;
+}
+int mycls::embed_cls2::fecl2(int x) {
+  return x - 11;
+}
+
 namespace chns {
 int chf2(int x);
 
Index: test/Analysis/ctu-main.cpp
===
--- test/Analysis/ctu-main.cpp
+++ test/Analysis/ctu-main.cpp
@@ -78,6 +78,6 @@
   clang_analyzer_eval(fun_using_anon_struct(8) == 8); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(other_macro_diag(1) == 1); // expected-warning{{TRUE}}
-  // expected-warning@Inputs/ctu-other.cpp:75{{REACHABLE}}
+  // expected-warning@Inputs/ctu-other.cpp:80{{REACHABLE}}
   MACRODIAG(); // expected-warning{{REACHABLE}}
 }
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -437,6 +437,8 @@
 
 Error ImportTemplateInformation(FunctionDecl *FromFD, FunctionDecl *ToFD);
 
+Error ImportFunctionDeclBody(FunctionDecl *FromFD, FunctionDecl *ToFD);
+
 bool IsStructuralMatch(Decl *From, Decl *To, bool Complain);
 bool IsStructuralMatch(RecordDecl *FromRecord, RecordDecl *ToRecord,
bool Complain = true);
@@ -2944,6 +2946,17 @@
   return FoundSpec;
 }
 
+Error ASTNodeImporter::ImportFunctionDeclBody(FunctionDecl *FromFD,
+  FunctionDecl *ToFD) {
+  if (Stmt *FromBody = FromFD->getBody()) {
+if (ExpectedStmt ToBodyOrErr = import(FromBody))
+  ToFD->setBody(*ToBodyOrErr);
+else
+  return ToBodyOrErr.takeError();
+  }
+  return Error::success();
+}
+
 ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
 
   SmallVector Redecls = getCanonicalForwardRedeclChain(D);
@@ -2967,7 +2980,7 @@
   if (ToD)
 return ToD;
 
-  const FunctionDecl *FoundByLookup = nullptr;
+  FunctionDecl *FoundByLookup = nullptr;
   FunctionTemplateDecl *FromFT = D->getDescribedFunctionTemplate();
 
   // If this is a function template specialization, then try to find the same
@@ -3038,6 +3051,25 @@
 }
   }
 
+  // We do not allow more than one in-class declaration of a function. This is
+  // because AST clients like VTableBuilder asserts on this. VTableBuilder
+  // assumes there is only one in-class declaration. Building a redecl
+  // chain would result in more than one in-class declaration for
+  // overrides (even if they are part of the same redecl chain inside the
+  // derived class.)
+  if (FoundByLookup) {
+if (auto *MD = dyn_cast(FoundByLookup)) {
+  if (D->getLexicalDeclContext() == D->getDeclContext()) {
+if (!D->doesThisDeclarationHaveABody())
+  return Importer.MapImported(D, FoundByLookup);
+else {
+  // Let's continue and build up the redecl chain in this case.
+  // FIXME Merge the functions into one decl.
+}
+  }
+}
+  }
+
   DeclarationNameInfo NameInfo(Name, Loc);
   // Import additional name location/type info.
   if (Error Err = ImportDeclarationNameLoc(D->getNameInfo(), NameInfo))
@@ -3199,12 +3231,10 @@
   }
 
   if (D->doesThisDeclarationHaveABody()) {
-if (Stmt *FromBody = D->getBody()) {
-  if (ExpectedStmt ToBodyOrErr = import(FromBody))
-ToFunction->setBody(*ToBodyOrErr);
-  else
-return ToBodyOrErr.takeError();
-}
+Error Err = ImportFunctionDeclBody(D, ToFunction);
+
+if (Err)
+  return std::move(Err);
   }
 
   // FIXME: Other bits to merge?
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2232,6 +2232,191 @@
 }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, ImportOver

[PATCH] D57322: [ASTImporter] Refactor unittests to be able to parameterize them in a more flexible way

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: unittests/AST/ASTImporterTest.cpp:468
 
+  template  DeclT *Import(DeclT *From, Language Lang) {
+return cast_or_null(Import(cast(From), Lang));

Is this being used in this PR?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57322



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

[PATCH] D56581: [ASTImporter] Set the described template if not set

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@martong have you had a chance to look at this some more? We ran into another 
problem that this fixes partially. Can I help in any way?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56581



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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I am happy to discuss and review a revised version of this change.


Repository:
  rC Clang

https://reviews.llvm.org/D44100



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

[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I reverted this commit since it caused a regression in the lldb test suite, 
specifically TestDataFormatterLibcxxVector.py.

See the logs here 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/12003/

Please monitor the lldb built bots when modifying ASTImporter.


Repository:
  rC Clang

https://reviews.llvm.org/D53697



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

[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@martong Hello, if I look at the specific test that was failing 
`TestDataFormatterLibcxxVector.py` it only has the following decorator 
`@add_test_categories(["libc++"])` so that should not prevent it from running 
on Linux if you are also building libc++. If you do a local cmake build and you 
build libc++ as well does it still skip the test?


Repository:
  rC Clang

https://reviews.llvm.org/D53697



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

[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: include/clang/AST/Expr.h:1407
 public:
-  enum CharacterKind {
-Ascii,
-Wide,
-UTF8,
-UTF16,
-UTF32
-  };
+  enum CharacterKind { Ascii, Wide, UTF8, UTF16, UTF32 };
 

Minor comment, does it make sense to covert this to a scoped enum since it 
looks like it is being strictly used as a set of values.


Repository:
  rC Clang

https://reviews.llvm.org/D54324



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

[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: include/clang/AST/Expr.h:1407
 public:
-  enum CharacterKind {
-Ascii,
-Wide,
-UTF8,
-UTF16,
-UTF32
-  };
+  enum CharacterKind { Ascii, Wide, UTF8, UTF16, UTF32 };
 

riccibruno wrote:
> shafik wrote:
> > Minor comment, does it make sense to covert this to a scoped enum since it 
> > looks like it is being strictly used as a set of values.
> Does it really add anything ?
> It means that instead of writing `CharacterLiteral::UTF32`
> you have to write `CharacterLiteral::CharacterKind::UTF32`
> 
> Seems a bit verbose. But I don't have any strong opinion on this.
It adds the protection against unintended conversions to and from the scoped 
enum, which in general is a good thing. The other benefits is not having the 
enumerators leak out into the surrounding scope but is limited in this case.

It does add verbosity though.


Repository:
  rC Clang

https://reviews.llvm.org/D54324



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

[PATCH] D53702: [ASTImporter] Set redecl chain of functions before any other import

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Did you ever resolve the issue of libcxx tests not running 
https://reviews.llvm.org/D53697


Repository:
  rC Clang

https://reviews.llvm.org/D53702



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

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added subscribers: rsmith, shafik.
shafik added a comment.
Herald added a reviewer: shafik.
Herald added a subscriber: gamesh411.

I think these changes make sense at a high level but I am not sure about the 
refactoring strategy. I am especially concerned we may end up in place where 
all the effected users of the API don't get updated and we are stuck with this 
parallel API.

Tagging in @rsmith since he has reviewed a lot of recent changes involving 
ASTImpoter that I have seen recently and he will have a better feeling for how 
these types of refactoring on handled on the clang side. I am mostly focused on 
the lldb side but care about the ASTImporter since we depend on it.


Repository:
  rC Clang

https://reviews.llvm.org/D53751



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

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik requested changes to this revision.
shafik added a comment.
This revision now requires changes to proceed.

The rest of the changes look good.




Comment at: lib/AST/ExternalASTMerger.cpp:147
   ToTag->setHasExternalLexicalStorage();
-  ToTag->setMustBuildLookupTable();
+  ToTag->getPrimaryContext()->setMustBuildLookupTable();
   assert(Parent.CanComplete(ToTag));

This change looks unrelated to the refactoring of `GetAlreadyImportedOrNull()` 
so should probably be in another PR



Comment at: lib/AST/ExternalASTMerger.cpp:154
   ToContainer->setHasExternalLexicalStorage();
-  ToContainer->setMustBuildLookupTable();
+  ToContainer->getPrimaryContext()->setMustBuildLookupTable();
   assert(Parent.CanComplete(ToContainer));

martong wrote:
> a_sidorin wrote:
> > Do we have to do the same for NamespaceDecls?
> Yes, I think so.
> The primary context of a `NamespaceDecl` can be other than itself:
> ```
> DeclContext *DeclContext::getPrimaryContext() {
>   switch (DeclKind) {
>   case Decl::TranslationUnit:
>   case Decl::ExternCContext:
>   case Decl::LinkageSpec:
>   case Decl::Export:
>   case Decl::Block:
>   case Decl::Captured:
>   case Decl::OMPDeclareReduction:
> // There is only one DeclContext for these entities.
> return this;
> 
>   case Decl::Namespace:
> // The original namespace is our primary context.
> return static_cast(this)->getOriginalNamespace();
> ```
> Consequently, we should call `setMustBuildLookupTable` only on a 
> `NamespaceDecl` if we know for sure that is a primary context.
This change looks unrelated to the refactoring of `GetAlreadyImportedOrNull()` 
so should probably be in another PR


Repository:
  rC Clang

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

https://reviews.llvm.org/D53755



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

[PATCH] D53693: [ASTImporter] Typedef import brings in the complete type

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Apologies for my late comments.




Comment at: cfe/trunk/unittests/AST/ASTImporterTest.cpp:3780
+  typedefNameDecl(hasName("U")));
+  ASSERT_TRUE(ToD->getUnderlyingType()->isIncompleteType());
+

As far as I can tell `S` is complete in `Code`, am I missing something? 



Comment at: cfe/trunk/unittests/AST/ASTImporterTest.cpp:3792
+  typedefNameDecl(hasName("U")));
+  ASSERT_FALSE(FromD->getUnderlyingType()->isIncompleteType());
+

Given my previous comment, I don't see why the addition code would change where 
`S` was complete or not.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53693



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

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D53751#1311037 , @martong wrote:

> > I didn't actually see this comment get addressed other than to say it won't 
> > be a problem in practice, which I'm not certain I agree with. Was there a 
> > reason why this got commit before finding out if the reviewer with the 
> > concern agrees with your rationale? FWIW, I share the concern that having 
> > parallel APIs for any length of time is a dangerous thing. Given that 
> > "almost ready to go" is not "ready to go" but there's not an imminent 
> > release, I don't understand the rush to commit this.
>
> @aaron.ballman Thanks for your time and review on this.
>
> I completely understand you concern and agree that having such parallel API 
> even for a short time is not good. Please let me explain why we chose to do 
> this still:
>  `ASTImporter::Import` functions are used externally by LLDB and CTU as 
> clients. However, the functions are used internally too, inside `ASTImporter` 
> and `ASTNodeImporter`.  E.g. when we import an expression, then first we use 
> the `Import(QualType)` function to import its type.
>  Our goal is twofold: (1) enforce `ASTImporter` and `ASTNodeImporter` 
> implementation functions to always check the result of used import functions 
> and (2) make sure that clients can have detailed error information, so e.g. 
> in case of CTU we can handle unsupported constructs differently than ODR 
> errors.
>  As @balazske mentioned we could have changed the API in one lockstep but the 
> impact would have been too huge.


I believe an alternate approach would have been to change one function at a 
time and do this change across. These would have been smaller changes and 
easier to review. This would have prevented the need for an intermediate name 
which causes churn in the commit history.

> In the context of this patch, you can think of the newly introduced 
> `Import_New` functions as the internal only functions. I was thinking about 
> that we could make them private and `ASTNodeImporter` as a friend,  that way 
> we could hide them from clients and then the dual API would cease to exist.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53751



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

[PATCH] D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

As pointed out in this comment in another review

https://reviews.llvm.org/D44100#1311687

We need to make sure we are running the lldb test suite before committing and 
watching the bots to make sure the commit does not break them.

Thank you


Repository:
  rC Clang

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

https://reviews.llvm.org/D54898



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

[PATCH] D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Apologies, I meant to make the comment in the child PR


Repository:
  rC Clang

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

https://reviews.llvm.org/D54898



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

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

As pointed out in this comment in another review

https://reviews.llvm.org/D44100#1311687

We need to make sure we are running the lldb test suite before committing and 
watching the bots to make sure the commit does not break them.

The change is not purely a refactor since previously the error was consumed.

Thank you


Repository:
  rC Clang

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

https://reviews.llvm.org/D53755



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

[PATCH] D57590: [ASTImporter] Improve import of FileID.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM I don't see any LLDB test regressions but I would prefer another reviewer 
as well.

I am going to be modifying this soon to fix some issues w/ handling built-ins.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57590



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

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: martong, a.sidorin, teemperor, aaron.ballman.
Herald added subscribers: jdoerfert, rnkovacs.

Currently when we see a built-in we try and import the include location. 
Instead what we do now is find the buffer like we do for the invalid case and 
copy that over to the to context.


https://reviews.llvm.org/D58743

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager &FromSM = FromContext.getSourceManager();
+  bool IsBuiltin =  FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
 return {};
   SourceManager &ToSM = ToContext.getSourceManager();
@@ -8252,7 +8253,7 @@
 return llvm::make_error();
   return ToID;
 }
-FileID ASTImporter::Import(FileID FromID) {
+FileID ASTImporter::Import(FileID FromID, bool isBuiltin) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
 return Pos->second;
@@ -8278,25 +8279,30 @@
 }
 ToID = ToSM.getFileID(MLoc);
   } else {
-// Include location of this file.
-SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
-
 const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
-if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
-  // FIXME: We probably want to use getVirtualFile(), so we don't hit the
-  // disk again
-  // FIXME: We definitely want to re-use the existing MemoryBuffer, rather
-  // than mmap the files several times.
-  const FileEntry *Entry =
-  ToFileManager.getFile(Cache->OrigEntry->getName());
-  // FIXME: The filename may be a virtual name that does probably not
-  // point to a valid file and we get no Entry here. In this case try with
-  // the memory buffer below.
-  if (Entry)
-ToID = ToSM.createFileID(Entry, ToIncludeLoc,
- FromSLoc.getFile().getFileCharacteristic());
+
+if (!isBuiltin) {
+   // Include location of this file.
+   SourceLocation ToIncludeLoc = 
Import(FromSLoc.getFile().getIncludeLoc());
+
+
+if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
+  // FIXME: We probably want to use getVirtualFile(), so we don't hit 
the
+  // disk again
+  // FIXME: We definitely want to re-use the existing MemoryBuffer, 
rather
+  // than mmap the files several times.
+  const FileEntry *Entry =
+  ToFileManager.getFile(Cache->OrigEntry->getName());
+  // FIXME: The filename may be a virtual name that does probably not
+  // point to a valid file and we get no Entry here. In this case try 
with
+  // the memory buffer below.
+  if (Entry)
+ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+ 
FromSLoc.getFile().getFileCharacteristic());
+}
 }
-if (ToID.isInvalid()) {
+
+if (ToID.isInvalid() || isBuiltin) {
   // FIXME: We want to re-use the existing MemoryBuffer!
   bool Invalid = true;
   const llvm::MemoryBuffer *FromBuf = Cache->getBuffer(
Index: include/clang/AST/ASTImporter.h
===
--- include/clang/AST/ASTImporter.h
+++ include/clang/AST/ASTImporter.h
@@ -339,7 +339,7 @@
 /// context, or the import error.
 llvm::Expected Import_New(FileID);
 // FIXME: Remove this version.
-FileID Import(FileID);
+FileID Import(FileID, bool isBuiltin=false);
 
 /// Import the given C++ constructor initializer from the "from"
 /// context into the "to" context.


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager &FromSM = FromContext.getSourceManager();
+  bool IsBuiltin =  FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
 return {};
   SourceManager &ToSM = ToContext.getSourceManager();
@@ -8252,7 +8253,7 @@
 return llvm::make_error();
   return ToID;
 }
-FileID ASTImporter::Import(FileID FromID) {
+FileID ASTImporter::Import(FileID FromID, bool isBuiltin) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
 return Pos->second;
@@ -8278,25 +8279,30 @@
 }
 ToID = ToSM.getFileID(MLoc);
   } else {
-// Include location of

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 188753.
shafik marked 7 inline comments as done.
shafik added a comment.

Addressed comments on formatting and missing changes to the `Import_New` 
version of the code.


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

https://reviews.llvm.org/D58743

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager &FromSM = FromContext.getSourceManager();
+  bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
 return {};
   SourceManager &ToSM = ToContext.getSourceManager();
@@ -8246,13 +8247,13 @@
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
-Expected ASTImporter::Import_New(FileID FromID) {
-  FileID ToID = Import(FromID);
+Expected ASTImporter::Import_New(FileID FromID, bool IsBuiltin) {
+  FileID ToID = Import(FromID, IsBuiltin);
   if (ToID.isInvalid() && FromID.isValid())
 return llvm::make_error();
   return ToID;
 }
-FileID ASTImporter::Import(FileID FromID) {
+FileID ASTImporter::Import(FileID FromID, bool IsBuiltin) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
 return Pos->second;
@@ -8278,25 +8279,29 @@
 }
 ToID = ToSM.getFileID(MLoc);
   } else {
-// Include location of this file.
-SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
-
 const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
-if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
-  // FIXME: We probably want to use getVirtualFile(), so we don't hit the
-  // disk again
-  // FIXME: We definitely want to re-use the existing MemoryBuffer, rather
-  // than mmap the files several times.
-  const FileEntry *Entry =
-  ToFileManager.getFile(Cache->OrigEntry->getName());
-  // FIXME: The filename may be a virtual name that does probably not
-  // point to a valid file and we get no Entry here. In this case try with
-  // the memory buffer below.
-  if (Entry)
-ToID = ToSM.createFileID(Entry, ToIncludeLoc,
- FromSLoc.getFile().getFileCharacteristic());
+
+if (!IsBuiltin) {
+  // Include location of this file.
+  SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
+
+  if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
+// FIXME: We probably want to use getVirtualFile(), so we don't hit the
+// disk again
+// FIXME: We definitely want to re-use the existing MemoryBuffer, 
rather
+// than mmap the files several times.
+const FileEntry *Entry =
+ToFileManager.getFile(Cache->OrigEntry->getName());
+// FIXME: The filename may be a virtual name that does probably not
+// point to a valid file and we get no Entry here. In this case try 
with
+// the memory buffer below.
+if (Entry)
+  ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+   FromSLoc.getFile().getFileCharacteristic());
+  }
 }
-if (ToID.isInvalid()) {
+
+if (ToID.isInvalid() || IsBuiltin) {
   // FIXME: We want to re-use the existing MemoryBuffer!
   bool Invalid = true;
   const llvm::MemoryBuffer *FromBuf = Cache->getBuffer(
Index: include/clang/AST/ASTImporter.h
===
--- include/clang/AST/ASTImporter.h
+++ include/clang/AST/ASTImporter.h
@@ -337,9 +337,9 @@
 ///
 /// \returns The equivalent file ID in the source manager of the "to"
 /// context, or the import error.
-llvm::Expected Import_New(FileID);
+llvm::Expected Import_New(FileID, bool IsBuiltin = false);
 // FIXME: Remove this version.
-FileID Import(FileID);
+FileID Import(FileID, bool IsBuiltin = false);
 
 /// Import the given C++ constructor initializer from the "from"
 /// context into the "to" context.


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager &FromSM = FromContext.getSourceManager();
+  bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
 return {};
   SourceManager &ToSM = ToContext.getSourceManager();
@@ -8246,13 +8247,13 @@
   return S

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D58743#1413249 , @lebedev.ri wrote:

> Is there a test missing here?


This origin of this fix is the LLDB expression parsing issue. We were not able 
to reduce to a test we could put in `ASTImpoterTest.cpp` but we have a LLDB 
test that exercises this issue and will pass once this fix is in place.

See this differential https://reviews.llvm.org/D58790




Comment at: lib/AST/ASTImporter.cpp:8284
+
+if (!isBuiltin) {
+   // Include location of this file.

balazske wrote:
> The `Cache` can be moved into this block and the block to `else if`.
I am not sure I understand what you are asking. Do you mean just duplicate the 

const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();

in both blocks?



Comment at: lib/AST/ASTImporter.cpp:8301
+ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+ 
FromSLoc.getFile().getFileCharacteristic());
+}

balazske wrote:
> Is it possible that in the `isBuiltin` true case this part of the code does 
> run (always) without assertion or other error and the result is always an 
> invalid `ToID`? (If yes the whole change is not needed, so I want to know 
> what was the reason for this change, was there any crash or bug.)
This change was the result of a assert where evaluating an expression resulted 
in the built-in being imported and it would assert in `SourceManager` when when 
`Import(SourceLocation)`   attempted `getDecomposedLoc`  at the next step.

AFAICT the The Preprocessor sets it up the buffer 
[here](https://github.com/llvm-mirror/clang/blob/master/lib/Lex/Preprocessor.cpp#L552)
 and then creates the `FileID` for in the `SourceManager` and copying over the 
buffer should be the correct thing to do since that seems to be sufficient. 

I also while testing verified that the `BufferIndetifer()` did indeed equal 
`""` similar to [this 
check](https://github.com/llvm-mirror/clang/blob/master/lib/Basic/SourceManager.cpp#L2009)
 in `SourceManager`.



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

https://reviews.llvm.org/D58743



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

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@teemperor ping


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

https://reviews.llvm.org/D58743



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

[PATCH] D58502: [ASTImporter] Fix redecl failures of Class and ClassTemplate

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM, thank you !


Repository:
  rC Clang

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

https://reviews.llvm.org/D58502



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

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL355332: [ASTImporter] Handle built-in when importing 
SourceLocation and FileID (authored by shafik, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58743?vs=188753&id=189185#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58743

Files:
  cfe/trunk/include/clang/AST/ASTImporter.h
  cfe/trunk/lib/AST/ASTImporter.cpp


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager &FromSM = FromContext.getSourceManager();
+  bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
 return {};
   SourceManager &ToSM = ToContext.getSourceManager();
@@ -8246,13 +8247,13 @@
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
-Expected ASTImporter::Import_New(FileID FromID) {
-  FileID ToID = Import(FromID);
+Expected ASTImporter::Import_New(FileID FromID, bool IsBuiltin) {
+  FileID ToID = Import(FromID, IsBuiltin);
   if (ToID.isInvalid() && FromID.isValid())
 return llvm::make_error();
   return ToID;
 }
-FileID ASTImporter::Import(FileID FromID) {
+FileID ASTImporter::Import(FileID FromID, bool IsBuiltin) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
 return Pos->second;
@@ -8278,25 +8279,29 @@
 }
 ToID = ToSM.getFileID(MLoc);
   } else {
-// Include location of this file.
-SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
-
 const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
-if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
-  // FIXME: We probably want to use getVirtualFile(), so we don't hit the
-  // disk again
-  // FIXME: We definitely want to re-use the existing MemoryBuffer, rather
-  // than mmap the files several times.
-  const FileEntry *Entry =
-  ToFileManager.getFile(Cache->OrigEntry->getName());
-  // FIXME: The filename may be a virtual name that does probably not
-  // point to a valid file and we get no Entry here. In this case try with
-  // the memory buffer below.
-  if (Entry)
-ToID = ToSM.createFileID(Entry, ToIncludeLoc,
- FromSLoc.getFile().getFileCharacteristic());
+
+if (!IsBuiltin) {
+  // Include location of this file.
+  SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
+
+  if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
+// FIXME: We probably want to use getVirtualFile(), so we don't hit the
+// disk again
+// FIXME: We definitely want to re-use the existing MemoryBuffer, 
rather
+// than mmap the files several times.
+const FileEntry *Entry =
+ToFileManager.getFile(Cache->OrigEntry->getName());
+// FIXME: The filename may be a virtual name that does probably not
+// point to a valid file and we get no Entry here. In this case try 
with
+// the memory buffer below.
+if (Entry)
+  ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+   FromSLoc.getFile().getFileCharacteristic());
+  }
 }
-if (ToID.isInvalid()) {
+
+if (ToID.isInvalid() || IsBuiltin) {
   // FIXME: We want to re-use the existing MemoryBuffer!
   bool Invalid = true;
   const llvm::MemoryBuffer *FromBuf = Cache->getBuffer(
Index: cfe/trunk/include/clang/AST/ASTImporter.h
===
--- cfe/trunk/include/clang/AST/ASTImporter.h
+++ cfe/trunk/include/clang/AST/ASTImporter.h
@@ -337,9 +337,9 @@
 ///
 /// \returns The equivalent file ID in the source manager of the "to"
 /// context, or the import error.
-llvm::Expected Import_New(FileID);
+llvm::Expected Import_New(FileID, bool IsBuiltin = false);
 // FIXME: Remove this version.
-FileID Import(FileID);
+FileID Import(FileID, bool IsBuiltin = false);
 
 /// Import the given C++ constructor initializer from the "from"
 /// context into the "to" context.


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager &FromSM = FromContext.getSourceManager();
+  bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::p

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM outside of the question I had.




Comment at: lib/AST/ASTImporter.cpp:4967
+template  static auto getTemplateDefinition(T *D) -> T * {
+  auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
   if (!ToTemplatedDef)

Can we guarantee that `D->getTemplatedDecl()` will always return a valid 
pointer? 



Comment at: lib/AST/ASTImporter.cpp:4967
+template  static auto getTemplateDefinition(T *D) -> T * {
+  auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
   if (!ToTemplatedDef)

shafik wrote:
> Can we guarantee that `D->getTemplatedDecl()` will always return a valid 
> pointer? 
What other types besides `CXXRecordDecl` do we expect here? 



Comment at: lib/AST/ASTImporter.cpp:5544
   // type, and in the same context as the function we're importing.
+  // FIXME Split this into a separate function.
   if (!LexicalDC->isFunctionOrMethod()) {

Would it make sense to do the split into a separate function in the PR?



Comment at: lib/AST/ASTImporter.cpp:5595
+  auto *PrevTemplated =
+  FoundByLookup->getTemplatedDecl()->getMostRecentDecl();
+  if (TemplatedFD != PrevTemplated)

Can we guarantee that `FoundByLookup->getTemplatedDecl()` will always return a 
valid pointer? 


Repository:
  rC Clang

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

https://reviews.llvm.org/D58494



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

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: lib/AST/ASTImporter.cpp:5147
   }
+} else { // ODR violation.
+  // FIXME HandleNameConflict

ODR violations are ill-formed no diagnostic required. So currently will this 
fail for cases that clang proper would not?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

[PATCH] D58830: [ASTImporter] Import member expr with explicit template args

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM and I don't see any regressions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58830



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

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D58668



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: teemperor, martong, a_sidorin.
Herald added a subscriber: rnkovacs.

https://reviews.llvm.org/D51633 added error handling to the 
`ASTNodeImporter::VisitEnumDecl(...)` for the conflicting names case. This 
could lead to erroneous return of an error in that case since we should have 
been using `SearchName`. `Name` may be empty in the case where we find the name 
via `getTypedefNameForAnonDecl(...)`.


https://reviews.llvm.org/D59665

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2460,7 +2460,7 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),
  ConflictingDecls.size());
   if (!Name)


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2460,7 +2460,7 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),
  ConflictingDecls.size());
   if (!Name)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I was not able to come up with a test that would detect this issue using either 
`clang-import-test` nor via any of the methods used in `ASTImpoterTest.cpp`. I 
created a regression test on the lldb side, which should pass once this is 
committed:

https://reviews.llvm.org/D59667


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

https://reviews.llvm.org/D59665



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

[PATCH] D53757: [ASTImporter] Changed use of Import to Import_New in ASTNodeImporter.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: lib/AST/ASTImporter.cpp:3418
 
-  for (const auto *Attr : D->attrs())
-ToIndirectField->addAttr(Importer.Import(Attr));

Why is this section of code removed?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53757



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D59665#1439070 , @martong wrote:

> Hi Shafik,
>
> Thank you for looking into this. This is indeed a bug, because whenever a we 
> encounter an unnamed EnumDecl in the "from" context then we return with a 
> nameconflict error.
>  However, I think we should fix this differently.
>  First of all, currently HandleNameConflict just returns the parameter `Name` 
> it received. This may be unnamed in some cases and thus converts to true. So 
> I think HandleNameConflict should be changed that it would return a default 
> initialized DeclarationName; so once we have anything in the ConflictingDecls 
> then we return with the NameConflict error:
>
>   DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
>   DeclContext *DC,
>   unsigned IDNS,
>   NamedDecl **Decls,
>   unsigned NumDecls) {
>   -  return Name;
>   +  return DeclarationName();
>   }
>
>
> And most importantly, I think we should push into the ConflictingDecls only 
> if the structural match indicates a non-match:
>
> if (auto *FoundEnum = dyn_cast(FoundDecl)) {
>   if (isStructuralMatch(D, FoundEnum, true))
> return Importer.MapImported(D, FoundEnum);
>   +   ConflictingDecls.push_back(FoundDecl);
> }
>   -
>   - ConflictingDecls.push_back(FoundDecl);
>
>
> I am aware of similar errors like this with other AST nodes. We had a patch 
> in our fork to fix that in January 
> (https://github.com/Ericsson/clang/pull/569/files) I am going to prepare a 
> patch from that cause I see now this affects you guys in LLDB too.


This sounds reasonable, let me test it out and make sure it looks good.


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

https://reviews.llvm.org/D59665



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked an inline comment as done.
shafik added a comment.

@martong your idea does not work b/c default construction `DeclarationName()` 
treats it the same as being empty. So `if (!Name)` is still `true`.




Comment at: lib/AST/ASTImporter.cpp:2463
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),

a_sidorin wrote:
> If I understand correctly, this will replace Name with SearchName causing an 
> anonymous enum to be imported as a named a few lines below. It doesn't look 
> like a correct behaviour to me.
This is correct because either `SearchName` is `Name` or it is the name of the 
typedef for the anonymous enum set via `ImportInto(SearchName, 
D->getTypedefNameForAnonDecl()->getDeclName())`


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

https://reviews.llvm.org/D59665



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked an inline comment as done.
shafik added a comment.

In D59665#1442826 , @martong wrote:

> In D59665#1442328 , @shafik wrote:
>
> > @martong your idea does not work b/c default construction 
> > `DeclarationName()` treats it the same as being empty. So `if (!Name)` is 
> > still `true`.
>
>
> And did you try moving the `push_back` to the other scope? I'd expect the the 
> ConflictingDecls to be empty then.


Yes, I did and I think it looks good but I have to run a regression.




Comment at: lib/AST/ASTImporter.cpp:2463
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),

martong wrote:
> shafik wrote:
> > a_sidorin wrote:
> > > If I understand correctly, this will replace Name with SearchName causing 
> > > an anonymous enum to be imported as a named a few lines below. It doesn't 
> > > look like a correct behaviour to me.
> > This is correct because either `SearchName` is `Name` or it is the name of 
> > the typedef for the anonymous enum set via `ImportInto(SearchName, 
> > D->getTypedefNameForAnonDecl()->getDeclName())`
> Okay, this is indeed correct. But then we should make a similar change in 
> VisitRecordDecl too (there we do exactly the same with typedefs).
This is fixing a specific bug, so I would want to keep this change specifically 
related to that and I can add a second review for `VisitRecordDecl`


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

https://reviews.llvm.org/D59665



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

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: martong, teemperor, friss, a_sidorin.
Herald added subscribers: jdoerfert, rnkovacs.

We may try and re-import an EnumDecl while trying to complete it in 
`IsStructuralMatch(...)` specialization for `EnumDecl`. This change mirrors a 
similar fix for the specialization for `RecordDecl`.


https://reviews.llvm.org/D59845

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,17 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum);
+
+  if (ToOrigin) {
+auto *ToOriginEnum = dyn_cast(ToOrigin);
+
+if (ToOriginEnum)
+ToEnum = ToOriginEnum;
+  }
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), 
getStructuralEquivalenceKind(Importer));


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,17 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum);
+
+  if (ToOrigin) {
+auto *ToOriginEnum = dyn_cast(ToOrigin);
+
+if (ToOriginEnum)
+ToEnum = ToOriginEnum;
+  }
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

LLDB regression test that goes with this fix: https://reviews.llvm.org/D59847


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

https://reviews.llvm.org/D59845



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

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 192466.
shafik added a comment.

Fixes based on comments.


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

https://reviews.llvm.org/D59845

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,12 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum))
+if (auto *ToOriginEnum = dyn_cast(ToOrigin))
+ToEnum = ToOriginEnum;
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), 
getStructuralEquivalenceKind(Importer));


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,12 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum))
+if (auto *ToOriginEnum = dyn_cast(ToOrigin))
+ToEnum = ToOriginEnum;
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked 2 inline comments as done.
shafik added inline comments.



Comment at: lib/AST/ASTImporter.cpp:1951
+  // something we're trying to import while completin ToEnum
+  Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum);
+

JDevlieghere wrote:
> ```
> if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum)) 
>   if (auto *ToOriginEnum = dyn_cast(ToOrigin))
> ToEnum = ToOriginEnum;
> ```
I normally just match the local style but this is indeed much better style.


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

https://reviews.llvm.org/D59845



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

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
shafik marked an inline comment as done.
Closed by commit rL357100: [ASTImporter] Fix IsStructuralMatch specialization 
for EnumDecl to prevent re… (authored by shafik, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59845?vs=192466&id=192475#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59845

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,12 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum))
+if (auto *ToOriginEnum = dyn_cast(ToOrigin))
+ToEnum = ToOriginEnum;
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), 
getStructuralEquivalenceKind(Importer));


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,12 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum))
+if (auto *ToOriginEnum = dyn_cast(ToOrigin))
+ToEnum = ToOriginEnum;
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59761: [ASTImporter] Convert ODR diagnostics inside ASTImporter implementation

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D59761



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC357940: [ASTImporter] Call to HandleNameConflict in 
VisitEnumDecl mistakeningly using… (authored by shafik, committed by ).
Herald added a project: clang.

Repository:
  rC Clang

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

https://reviews.llvm.org/D59665

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2441,7 +2441,7 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),
  ConflictingDecls.size());
   if (!Name)


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2441,7 +2441,7 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),
  ConflictingDecls.size());
   if (!Name)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/trunk/unittests/AST/ASTImporterTest.cpp:4054
+}
   }
 

aganea wrote:
> Fixes
> ```
> [2097/2979] Building CXX object 
> tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/LookupTest.cpp.o
> /mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp: In lambda function:
> /mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp:220:8: warning: suggest 
> explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
>  if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
> ^
> ```
You mixed up the error messages but I see what is going on.

So you may want to add a comment since it is not apparent that what is going on 
is due the `EXPECT_TRUE` macro eventually expanding to an `if..else` which is 
what is triggering the warning. Since someone may come by in the future and 
just remove the braces since it is not apparent why they are there.

Same below as well.


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

https://reviews.llvm.org/D61046



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

[PATCH] D61140: Copy Argument Passing Restrictions setting when importing a CXXRecordDecl definition

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: martong, teemperor, aprantl, a_sidorin.
Herald added a subscriber: rnkovacs.

For a `CXXRecordDecl` the `RecordDeclBits` are stored in the `DeclContext`. 
Currently when we import the definition of a `CXXRecordDecl` via the 
`ASTImporter` we do not copy over this data.

We had a LLDB expression parsing bug where we would set it to not pass in 
registers:

  setArgPassingRestrictions(clang::RecordDecl::APK_CannotPassInRegs);

but when imported this setting would be lost. So dumping the `CXXRecordDecl` 
before importing we would see:

  CXXRecordDecl 0x7faaba292e50 <>  struct Bounds 
definition
  |-DefinitionData standard_layout has_user_declared_ctor can_const_default_init
  ...

but after importing it would show the following:

  CXXRecordDecl 0x7ff286823c50 <>  struct Bounds 
definition
  |-DefinitionData pass_in_registers standard_layout has_user_declared_ctor 
can_const_default_init
 ^

There will be a separate LLDB PR that will have a test that covers this and 
introduces a related fix for LLDB.

Note, we did not copy over any other of the `RecordDeclBits` since we don't 
have tests for those. We know that copying over 
`LoadedFieldsFromExternalStorage` would be a error and that may be the case for 
others as well.


https://reviews.llvm.org/D61140

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1767,6 +1767,9 @@
 ToData.HasDeclaredCopyAssignmentWithConstParam
   = FromData.HasDeclaredCopyAssignmentWithConstParam;
 
+// Copy over the data stored in RecordDeclBits
+ToCXX->setArgPassingRestrictions(FromCXX->getArgPassingRestrictions());
+
 SmallVector Bases;
 for (const auto &Base1 : FromCXX->bases()) {
   ExpectedType TyOrErr = import(Base1.getType());


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1767,6 +1767,9 @@
 ToData.HasDeclaredCopyAssignmentWithConstParam
   = FromData.HasDeclaredCopyAssignmentWithConstParam;
 
+// Copy over the data stored in RecordDeclBits
+ToCXX->setArgPassingRestrictions(FromCXX->getArgPassingRestrictions());
+
 SmallVector Bases;
 for (const auto &Base1 : FromCXX->bases()) {
   ExpectedType TyOrErr = import(Base1.getType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61140: Copy Argument Passing Restrictions setting when importing a CXXRecordDecl definition

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 196757.
shafik added a comment.

Added test


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

https://reviews.llvm.org/D61140

Files:
  lib/AST/ASTImporter.cpp
  test/Import/cxx-record-flags/Inputs/F.cpp
  test/Import/cxx-record-flags/test.cpp


Index: test/Import/cxx-record-flags/test.cpp
===
--- /dev/null
+++ test/Import/cxx-record-flags/test.cpp
@@ -0,0 +1,14 @@
+// RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s | 
FileCheck %s
+
+// CHECK: FTrivial
+// CHECK: DefinitionData
+// CHECK-SAME: pass_in_registers
+
+// CHECK: FNonTrivial
+// CHECK-NOT: pass_in_registers
+// CHECK: DefaultConstructor
+
+void expr() {
+  FTrivial f1;
+  FNonTrivial f2;
+}
Index: test/Import/cxx-record-flags/Inputs/F.cpp
===
--- /dev/null
+++ test/Import/cxx-record-flags/Inputs/F.cpp
@@ -0,0 +1,9 @@
+class FTrivial {
+  int i;
+};
+
+struct FNonTrivial {
+  virtual ~FNonTrivial() = default;
+  int i;
+};
+
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1767,6 +1767,9 @@
 ToData.HasDeclaredCopyAssignmentWithConstParam
   = FromData.HasDeclaredCopyAssignmentWithConstParam;
 
+// Copy over the data stored in RecordDeclBits
+ToCXX->setArgPassingRestrictions(FromCXX->getArgPassingRestrictions());
+
 SmallVector Bases;
 for (const auto &Base1 : FromCXX->bases()) {
   ExpectedType TyOrErr = import(Base1.getType());


Index: test/Import/cxx-record-flags/test.cpp
===
--- /dev/null
+++ test/Import/cxx-record-flags/test.cpp
@@ -0,0 +1,14 @@
+// RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s | FileCheck %s
+
+// CHECK: FTrivial
+// CHECK: DefinitionData
+// CHECK-SAME: pass_in_registers
+
+// CHECK: FNonTrivial
+// CHECK-NOT: pass_in_registers
+// CHECK: DefaultConstructor
+
+void expr() {
+  FTrivial f1;
+  FNonTrivial f2;
+}
Index: test/Import/cxx-record-flags/Inputs/F.cpp
===
--- /dev/null
+++ test/Import/cxx-record-flags/Inputs/F.cpp
@@ -0,0 +1,9 @@
+class FTrivial {
+  int i;
+};
+
+struct FNonTrivial {
+  virtual ~FNonTrivial() = default;
+  int i;
+};
+
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1767,6 +1767,9 @@
 ToData.HasDeclaredCopyAssignmentWithConstParam
   = FromData.HasDeclaredCopyAssignmentWithConstParam;
 
+// Copy over the data stored in RecordDeclBits
+ToCXX->setArgPassingRestrictions(FromCXX->getArgPassingRestrictions());
+
 SmallVector Bases;
 for (const auto &Base1 : FromCXX->bases()) {
   ExpectedType TyOrErr = import(Base1.getType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57740: [ASTImporter] Import every Decl in lambda record

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

LGTM although I wish we had more lambda tests.

Please remember to check the LLDB build bots when landing the patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57740



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

[PATCH] D57322: [ASTImporter] Refactor unittests to be able to parameterize them in a more flexible way

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

This looks reasonable to me but I don't have strong opinions on refactoring 
gtest tests.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57322



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

[PATCH] D57902: [AST] Fix structural inequivalence of operators

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: unittests/AST/StructuralEquivalenceTest.cpp:251
+
+TEST_F(StructuralEquivalenceFunctionTest, CtorVsDtor) {
+  auto t = makeDecls(

Curious, is this test just for completeness or is this somehow related to the 
overloaded operator check?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57902



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

[PATCH] D57232: [ASTImporter] Check visibility/linkage of functions and variables

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jdoerfert.

I ran `check-lldb` locally and it looks good.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57232



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

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

This looks reasonable, I will wait for @martong and/or @a_sidorin to review.

FYI LLDB is the other big user of ASTImpoter so it is helpful if you can run 
`check-lldb` especially on MacOS so you can to catch regressions before 
committing. After committing please make sure to monitor the GreenDragon build 
bots:

  http://lab.llvm.org:8080/green/view/LLDB/

Thank you!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58292



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

[PATCH] D57910: [ASTImporter] Find previous friend function template

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM, I just ran `check-lldb` and I don't see any regressions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57910



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik requested changes to this revision.
shafik added a comment.
This revision now requires changes to proceed.

Actually I was mistaken, we can see the difference for `EnumDecl` and 
`ClassTemplateSpecializationDecl` as well.

For `EnumDecl` before:

  EnumDecl 0x7fd0ae884800  col:6 referenced B

After:

  EnumDecl 0x7fa703882600  line:2:6 referenced B

For `ClassTemplateSpecializationDecl` before:

  ClassTemplateSpecializationDecl 0x7f8c338846a0  col:7 
class A definition

after:

  ClassTemplateSpecializationDecl 0x7fca150832a0  line:5:7 
class A definition

So once we have tests similar to D61140  I 
would feel good about this change. Especially since it is not clear when the 
other changes will be in and tests are important to guard against regressions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60499



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

[PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

This LGTM now but I will wait for @teemperor to take a look at it.




Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:620
 
 if (predicate(decl->getKind())) {
   if (log) {

I think a comment on the `predicate` would be helpful as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61333



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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D44100



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

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

First round of review.




Comment at: clang/lib/AST/ASTImporter.cpp:2632
+  ExpectedName NameOrErr = Importer.HandleNameConflict(
+  Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+  if (!NameOrErr)

`Name` -> `SearchName`



Comment at: clang/unittests/AST/ASTImporterTest.cpp:2392
 
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+

What about tests for name conflicts for:

`NamespaceDecl` 
`TypedefNameDecl`
`TypeAliasTemplateDecl`
`EnumConstantDecl`
`RecordDecl`
`VarDecl`

Who were also modified above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59692



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

[PATCH] D65577: [ASTImporter] Import default expression of param before creating the param.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3859
   ToTypeSourceInfo, D->getStorageClass(),
   /*DefaultArg*/ nullptr))
 return ToParm;

This should be `DefaultArg` now?



Comment at: clang/test/Analysis/Inputs/ctu-other.cpp:135
+
+struct DefParmContext {
+  static const int I;

martong wrote:
> Perhaps we could write `Default` instead of `Def`.
+1 to this and the name suggestion below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65577



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

[PATCH] D65573: Add User docs for ASTImporter

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Thank you for writing this up! I just have a few minor comments.




Comment at: clang/docs/LibASTImporter.rst:110
+
+Now we create the Importer and do the import:
+

Maybe helpful to link to the [Matching the Clang 
AST](https://clang.llvm.org/docs/LibASTMatchers.html) and [AST Matcher 
Reference](https://clang.llvm.org/docs/LibASTMatchersReference.html)



Comment at: clang/docs/LibASTImporter.rst:283
+However, there may be cases when both of the contexts have a definition for a 
given symbol.
+If these definitions differ then we have a name conflict, otherwise known as 
ODR (one definition rule) violation.
+Let's modify the previous tool we had written and try to import a 
``ClassTemplateSpecializationDecl`` with a conflicting definition:

Note ODR is a C++ concept C does not have ODR


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65573



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

[PATCH] D66028: [ASTDump] Add is_anonymous to VisitCXXRecordDecl

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: aaron.ballman, aprantl.

Adding is_anonymous the ASTDump for `CXXRecordDecl`. This turned out to be 
useful when debugging some problems with how LLDB creates ASTs from DWARF.


https://reviews.llvm.org/D66028

Files:
  lib/AST/TextNodeDumper.cpp
  test/AST/ast-dump-records.cpp


Index: test/AST/ast-dump-records.cpp
===
--- test/AST/ast-dump-records.cpp
+++ test/AST/ast-dump-records.cpp
@@ -65,7 +65,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -87,7 +87,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -194,7 +194,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -217,7 +217,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -1537,6 +1537,7 @@
 FLAG(isGenericLambda, generic);
 FLAG(isLambda, lambda);
 
+FLAG(isAnonymousStructOrUnion, is_anonymous);
 FLAG(canPassInRegisters, pass_in_registers);
 FLAG(isEmpty, empty);
 FLAG(isAggregate, aggregate);


Index: test/AST/ast-dump-records.cpp
===
--- test/AST/ast-dump-records.cpp
+++ test/AST/ast-dump-records.cpp
@@ -65,7 +65,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -87,7 +87,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -194,7 +194,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout trivially_

[PATCH] D66028: [ASTDump] Add is_anonymous to VisitCXXRecordDecl

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368591: [ASTDump] Add is_anonymous to VisitCXXRecordDecl 
(authored by shafik, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66028?vs=214436&id=214655#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66028

Files:
  cfe/trunk/lib/AST/TextNodeDumper.cpp
  cfe/trunk/test/AST/ast-dump-records.cpp


Index: cfe/trunk/lib/AST/TextNodeDumper.cpp
===
--- cfe/trunk/lib/AST/TextNodeDumper.cpp
+++ cfe/trunk/lib/AST/TextNodeDumper.cpp
@@ -1537,6 +1537,7 @@
 FLAG(isGenericLambda, generic);
 FLAG(isLambda, lambda);
 
+FLAG(isAnonymousStructOrUnion, is_anonymous);
 FLAG(canPassInRegisters, pass_in_registers);
 FLAG(isEmpty, empty);
 FLAG(isAggregate, aggregate);
Index: cfe/trunk/test/AST/ast-dump-records.cpp
===
--- cfe/trunk/test/AST/ast-dump-records.cpp
+++ cfe/trunk/test/AST/ast-dump-records.cpp
@@ -65,7 +65,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -87,7 +87,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -194,7 +194,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -217,7 +217,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit


Index: cfe/trunk/lib/AST/TextNodeDumper.cpp
===
--- cfe/trunk/lib/AST/TextNodeDumper.cpp
+++ cfe/trunk/lib/AST/TextNodeDumper.cpp
@@ -1537,6 +1537,7 @@
 FLAG(isGenericLambda, generic);
 FLAG(isLambda, lambda);
 
+FLAG(isAnonymousStructOrUnion, is_anonymous);
 FLAG(canPassInRegisters, pass_in_registers);
 FLAG(isEmpty, empty);
 FLAG(isAggregate, aggregate);
Index: cfe/trunk/test/AST/ast-dump-records.cpp
===
--- cfe/trunk/test/AST/ast-dump-records.cpp
+++ cfe/trunk/test/AST/ast-dump-records.cpp
@@ -65,7 +65,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -87,7 +87,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecord

[PATCH] D65935: [ASTImporter] Import ctor initializers after setting flags.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

I was hoping to be able reproduce this in LLDB via an expression like this:

  expr testImportOfDelegateConstructor(10) == 10

but it does not. I am assuming the test ctu test case invokes the issues 
without the patch? I wonder why we don't also see it in as well.

Otherwise LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65935



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

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Just wanted to see if you were planning on landing this soon, the fix `Name` -> 
`SearchName` is probably an important one since we have seen several issues w/ 
bugs like that but I really would like to see more tests. Are you having issues 
coming up w/ tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59692



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

[PATCH] D66348: [ASTImporter] Do not look up lambda classes

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I am not enthusiastic about this solution but I need to think about it some 
more.

We can see that p0624r2 
 added 
assignable lambdas:

  bool f1() {
  auto x = []{} = {}; auto x2 = x;
  
  return x == x2;
  }
  
  bool f2() {
  auto x = []{} = {};
  auto xb = []{} = {};
  
  return x == xb;
  }

see godbolt 

So I don't think this is a long-term solution, although I don't know what clang 
is doing to make this work yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66348



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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Done with first round.




Comment at: lib/AST/ASTImporter.cpp:1643
+if (!ImportedOrErr) {
+  // For RecordDecls, failed import of a field will break the layout of the
+  // structure. Handle it as an error.

What cases are failed import of a field ok? Is that because we will get the 
field later on by another path?



Comment at: lib/AST/ASTImporter.cpp:1655
+  // Reorder declarations in RecordDecls because they may have another
+  // order. Keeping field order is vitable because it determines structure
+  // layout.

vitable?



Comment at: lib/AST/ASTImporter.cpp:1663
+
+
+  // NOTE: Here and below, we cannot call field_begin() method and its callers

Maybe a comment here explaining the purpose of the loops below. IIUC removing 
fields since they may have been imported in the wrong order and then adding 
them back in what should be the correct order now.



Comment at: lib/AST/ASTImporter.cpp:1674
+  Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+  assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+  ToRD->removeDecl(ToD);

Are we sure `ToD` is never a `nullptr` b/c you are unconditionally derefenecing 
it here.



Comment at: lib/AST/ASTImporter.cpp:1679
+
+  if (!ToRD->hasExternalLexicalStorage())
+assert(ToRD->field_empty());

Can you add a comment explaining why if the Decl has ExternalLexicalStorage the 
fields might not be empty even though we just removed them?


Repository:
  rC Clang

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

https://reviews.llvm.org/D44100



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

[PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I ran `check-lldb` and I hit one regression in `TestFormatters.py`, part of 
what I am seeing is as follows:

  AssertionError: False is not True : FileCheck'ing result of `expression 
--show-types -- *(new foo(47))`
  Error output:
  error: no matching constructor for initialization of 'foo'
  candidate constructor (the implicit copy constructor) not viable: no known 
conversion from 'int' to 'const foo' for 1st argument
  candidate constructor (the implicit move constructor) not viable: no known 
conversion from 'int' to 'foo' for 1st argument




Comment at: clang/lib/AST/ASTImporter.cpp:1818
+  // The class will have DefinitionData, but no members.  Then,
+  // importDefinition is called from LLDB, which tries to get the members, se
+  // when we get here, the class already has the DefinitionData set, so we must

se -> so?



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:922
+
+  Log *log_ast(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST));
+  if (log_ast) {

I am going to say that the logging change is an excellent additions and stands 
alone from this change. Although I realize the test depends on this new 
feature. It makes sense to add the logging in a separate PR.

I also say this b/c I found a regression and it would be nice to get the 
logging in while we resolve the regression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61333



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

[PATCH] D62312: [ASTImporter] Added visibility context check for CXXRecordDecl.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Minor comments, I am going to run `check-lldb` now.




Comment at: unittests/AST/ASTImporterVisibilityTest.cpp:34
 };
+struct GetClassPattern {
+  using DeclTy = CXXRecordDecl;

`GetCXXRecordPattern` feels more consistent.



Comment at: unittests/AST/ASTImporterVisibilityTest.cpp:49
+// CXXRecordDecl:
+auto *ExternC = "class X;";
+auto *AnonC = "namespace { class X; }";

`const`? It is not consistent w/ the previous declarations. 



Comment at: unittests/AST/ASTImporterVisibilityTest.cpp:50
+auto *ExternC = "class X;";
+auto *AnonC = "namespace { class X; }";
 

`const`? It is not consistent w/ the previous declarations. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D62312



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

[PATCH] D62312: [ASTImporter] Added visibility context check for CXXRecordDecl.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D62312



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I don't see any regressions but I am a little uncomfortable since there are no 
tests. So I would feel better if this was split into three parts: Namespaces, 
Enums and Templates.

Are there small test programs that fail due to the missing data? We can add 
them as regression tests.




Comment at: lib/AST/ASTImporter.cpp:6129
 if (Error Err =
-ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+ImportTemplateArgumentListInfo(E->getLAngleLoc(), 
E->getRAngleLoc(),
+   E->template_arguments(), ToTAInfo))

Curious why you decided to add the new arguments to the front as opposed to the 
end?



Comment at: lib/AST/ASTImporter.cpp:7150
+  auto Imp = importSeq(E->getQualifierLoc(), E->getTemplateKeywordLoc(),
+   E->getDeclName(), E->getNameInfo().getLoc(),
+   E->getLAngleLoc(), E->getRAngleLoc());

Can you explain why `E->getNameInfo().getLoc()` is more correct than 
`E->getExprLoc()`?



Comment at: lib/AST/ASTImporter.cpp:7225
 
-  if (E->hasExplicitTemplateArgs() && E->getTemplateKeywordLoc().isValid()) {
+  if (E->hasExplicitTemplateArgs()) {
 TemplateArgumentListInfo ToTAInfo;

We still want to import a few lines down even if 
`!E->getTemplateKeywordLoc().isValid()`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

So an alternative to testing could be matching the AST output from a 
`clang-import-test` like we did here:

https://reviews.llvm.org/D61140

although it unfortunately looks like only only the AST dump of `NamespaceDecl` 
output the `SourceLocation` you are fixing, see it on godbolt 
 when I try a `clang-import-test` it looks like 
it is missing that `SourceLocation`:

  |-NamespaceDecl 0x7f8c1f884750 > col:11 

and with your patch applied I see the `SourceLocation`:

  |-NamespaceDecl 0x7fe3e6882f50  line:1:11 A


Repository:
  rC Clang

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

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I think the AST dump for `EnumDecl` and `ClassTemplateSpecializationDecl` 
should be dumping the missing `SourceLocations` but I am having trouble 
tracking down where that should be done.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60499



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

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Sounds good!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53755



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

[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: lib/AST/ASTStructuralEquivalence.cpp:302
+/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling
+/// conventions bits but must not compare some other bits, e.g. the noreturn
+/// bit.

This comment is confusing b/c it looks like the noreturn bits are the only one 
you are not checking.



Comment at: unittests/AST/StructuralEquivalenceTest.cpp:373
 
+TEST_F(StructuralEquivalenceFunctionTest,
+FunctionsWithDifferentAttributesButSameTypesShouldBeEqual) {

Can we get some more tests to be a little more thorough and can we also get a 
test where it is expected to to be false as well?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53699



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

[PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:118
+  template 
+  constexpr T X;
+  )";

Note this is not well-formed b/c it is not initialized, [see 
godbolt](https://godbolt.org/z/8xvFwh)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66951



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

[PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:118
+  template 
+  constexpr T X;
+  )";

shafik wrote:
> Note this is not well-formed b/c it is not initialized, [see 
> godbolt](https://godbolt.org/z/8xvFwh)
But it would be ok combined w/ a specialization:

```
template <>
constexpr int X = 0;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66951



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

[PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:151
+};
+
+template 

martong wrote:
> balazske wrote:
> > `FunctionTemplate` and `FunctionTemplateSpec` are missing?
> Yes, because `FunctionTemplates` overload with each other. So they are 
> imported always "liberally".
> 
> There is no point to liberally import conflicting 
> `FunctionTemplateSpecializations`.
> The only thing we can do in that case is to omit the conflicting declaration.
> And this is true in case of `ClassTemplateSpecialization`s too.
> 
> Perhaps we should remove `struct ClassTemplateSpec` as well from here (?).
> Because they are never going to be handled "liberally".
> 
> @shafik , what do you think about this?
What you say about `FunctionTemplateSpecializations` and 
`ClassTemplateSpecializations` seems to make sense, importing them liberally 
would require more than work in the importer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66951



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

[PATCH] D68634: [ASTImporter] Imported FunctionDecls inherit attributes from existing functions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:7842
+// This implementation is inspired by Sema::mergeDeclAttributes.
+void ASTNodeImporter::CopyInheritedAttributes(FunctionDecl *Old,
+  FunctionDecl *New) {

There are other attributes that [apply to functions as 
well](https://en.cppreference.com/w/cpp/language/attributes): `nodiscard`, 
`maybe_unused` and `deprecated`. Is there a reason not to support those as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68634



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

[PATCH] D68722: Add an example showing the alternative to nested designators

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added a reviewer: rsmith.

We have an example showing that a nested designators are a c99 extension but 
not an example showing how to achieve the same thing using a brace-init-list in 
a designated-initializer-clause.


https://reviews.llvm.org/D68722

Files:
  clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -37,7 +37,8 @@
   .x = 2 // reorder-error {{ISO C++ requires field designators to be specified 
in declaration order; field 'y' will be initialized after field 'x'}}
 };
 int arr[3] = {[1] = 5}; // pedantic-error {{array designators are a C99 
extension}}
-B b = {.a.x = 0}; // pedantic-error {{nested designators are a C99 extension}}
+B b1 = {.a.x = 0}; // pedantic-error {{nested designators are a C99 extension}}
+B b2 = {.a = {.x = 0, .y = 1}}; // ok, we don't need nested designators to 
initialize the members of A
 A a2 = {
   .x = 1, // pedantic-error {{mixture of designated and non-designated 
initializers in the same initializer list is a C99 extension}}
   2 // pedantic-note {{first non-designated initializer is here}}


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -37,7 +37,8 @@
   .x = 2 // reorder-error {{ISO C++ requires field designators to be specified in declaration order; field 'y' will be initialized after field 'x'}}
 };
 int arr[3] = {[1] = 5}; // pedantic-error {{array designators are a C99 extension}}
-B b = {.a.x = 0}; // pedantic-error {{nested designators are a C99 extension}}
+B b1 = {.a.x = 0}; // pedantic-error {{nested designators are a C99 extension}}
+B b2 = {.a = {.x = 0, .y = 1}}; // ok, we don't need nested designators to initialize the members of A
 A a2 = {
   .x = 1, // pedantic-error {{mixture of designated and non-designated initializers in the same initializer list is a C99 extension}}
   2 // pedantic-note {{first non-designated initializer is here}}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83972: Modify ImportDefiniton for ObjCInterfaceDecl so that we always the ImportDeclContext one we start the definition

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: teemperor, martong.
Herald added a subscriber: rnkovacs.

Once we start the definition of an `ObjCInterfaceDecl` we won't attempt to 
`ImportDeclContext` later on. Unlike `RecordDecl` case which uses 
`DefinitionCompleter` to force `completeDefinition` we don't seem to have a 
similar mechanism for `ObjCInterfaceDecl`.

This fix was needed due to a bug we see in LLDB expression parsing where an 
initial expression cause an `ObjCInterfaceDecl` to be defined and subsequent 
expressions during import do not call `ImportDeclContext` and we can end up in 
a situation where ivars are imported out of order and not all ivars are 
imported e.g.

  (lldb) expr chb1->hb->field2
  ObjCInterfaceDecl 0x7f9457495fe0 <>  
 HasBitfield1
  |-super ObjCInterface 0x7f9457495e78 'NSObject'
  `-ObjCIvarDecl 0x7f945749d478 <>  field2 
'unsigned int' public
`-IntegerLiteral 0x7f945749d458 <> 'int' 1

We have `field2` which is the second ivar but we are missing `field1`.

In this particular case `ASTContext::lookupFieldBitOffset(...)` assumes we have 
all ivars and they are in order to obtain the index of the ivar and we break 
this assumption. This leads eventually to bad IRGen since it has the wrong 
field offset.


https://reviews.llvm.org/D83972

Files:
  clang/lib/AST/ASTImporter.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/objc/bitfield_ivars/Makefile
  lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
  lldb/test/API/lang/objc/bitfield_ivars/main.m

Index: lldb/test/API/lang/objc/bitfield_ivars/main.m
===
--- lldb/test/API/lang/objc/bitfield_ivars/main.m
+++ lldb/test/API/lang/objc/bitfield_ivars/main.m
@@ -34,10 +34,31 @@
 
 @end
 
+@interface HasBitfield2 : NSObject {
+@public
+  unsigned int x;
+
+  unsigned field1 : 15;
+  unsigned field2 : 4;
+  unsigned field3 : 4;
+}
+@end
+
+@implementation HasBitfield2
+- (id)init {
+  return (self = [super init]);
+}
+@end
+
 int main(int argc, const char * argv[]) {
 ContainsAHasBitfield *chb = [[ContainsAHasBitfield alloc] init];
-printf("%d\n", chb->hb->field2); //% self.expect("expression -- chb->hb->field1", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["= 0"])
- //% self.expect("expression -- chb->hb->field2", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["= 1"]) # this must happen second
-return 0;
+HasBitfield2 *hb2 = [[HasBitfield2 alloc] init];
+
+hb2->x = 100;
+hb2->field1 = 10;
+hb2->field2 = 3;
+hb2->field3 = 4;
+
+return 0; // break here
 }
 
Index: lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
===
--- lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
+++ lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
@@ -1,12 +1,37 @@
-from lldbsuite.test import lldbinline
-from lldbsuite.test import decorators
-
-lldbinline.MakeInlineTest(
-__file__,
-globals(),
-[
-# This is a Darwin-only failure related to incorrect expression-
-# evaluation for single-bit ObjC bitfields.
-decorators.skipUnlessDarwin,
-decorators.expectedFailureAll(
-bugnumber="rdar://problem/17990991")])
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestBitfieldIvars(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+
+@skipUnlessDarwin
+def test(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.m"))
+
+self.expect_expr("chb->hb->field1", result_type="unsigned int", result_value="0")
+self.expect_expr("chb->hb->field2", result_type="unsigned int", result_value="1")
+
+self.expect_expr("hb2->field1", result_type="unsigned int", result_value="10")
+self.expect_expr("hb2->field2", result_type="unsigned int", result_value="3")
+self.expect_expr("hb2->field3", result_type="unsigned int", result_value="4")
+
+self.expect("frame var *hb2", substrs = [ 'x =', '100',
+ 'field1 =', '10',
+ 'field2 =', '3',
+ 'field3 =', '4'])
+
+@expectedFailureAll()
+def testExprWholeObject(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.m"))
+
+## FIXME expression with individual bit-fields obtains correct values but not with the whole object
+self.expect("expr *hb2", substrs = [ 'x =', '100',
+ 'field1 =', '10',
+ 'field2 =', '3',
+ 'field3 =', '4'])
Index: lldb/test/API/l

[PATCH] D83970: [ASTImporter] Refactor ASTImporter to support custom downstream tests

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

LGTM but I want @martong to take a look as well and accept.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83970



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

[PATCH] D83972: Modify ImportDefiniton for ObjCInterfaceDecl so that we always the ImportDeclContext one we start the definition

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 280291.
shafik added a comment.

Updating diff since the parent landed.


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

https://reviews.llvm.org/D83972

Files:
  clang/lib/AST/ASTImporter.cpp
  lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py


Index: lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
===
--- lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
+++ lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
@@ -14,9 +14,8 @@
 lldbutil.run_to_source_breakpoint(self, "// break here", 
lldb.SBFileSpec("main.m"))
 
 self.expect_expr("chb->hb->field1", result_type="unsigned int", 
result_value="0")
-
-## FIXME field2 should have a value of 1
-self.expect("expr chb->hb->field2", matching=False, substrs = ["= 1"]) 
# this must happen second
+## This should happen second
+self.expect_expr("chb->hb->field2", result_type="unsigned int", 
result_value="1")
 
 self.expect_expr("hb2->field1", result_type="unsigned int", 
result_value="10")
 self.expect_expr("hb2->field2", result_type="unsigned int", 
result_value="3")
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -4758,11 +4758,10 @@
   return ToImplOrErr.takeError();
   }
 
-  if (shouldForceImportDeclContext(Kind)) {
-// Import all of the members of this class.
-if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
-  return Err;
-  }
+  // Import all of the members of this class.
+  if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
+return Err;
+
   return Error::success();
 }
 


Index: lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
===
--- lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
+++ lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
@@ -14,9 +14,8 @@
 lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.m"))
 
 self.expect_expr("chb->hb->field1", result_type="unsigned int", result_value="0")
-
-## FIXME field2 should have a value of 1
-self.expect("expr chb->hb->field2", matching=False, substrs = ["= 1"]) # this must happen second
+## This should happen second
+self.expect_expr("chb->hb->field2", result_type="unsigned int", result_value="1")
 
 self.expect_expr("hb2->field1", result_type="unsigned int", result_value="10")
 self.expect_expr("hb2->field2", result_type="unsigned int", result_value="3")
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -4758,11 +4758,10 @@
   return ToImplOrErr.takeError();
   }
 
-  if (shouldForceImportDeclContext(Kind)) {
-// Import all of the members of this class.
-if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
-  return Err;
-  }
+  // Import all of the members of this class.
+  if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
+return Err;
+
   return Error::success();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83972: Modify ImportDefiniton for ObjCInterfaceDecl so that we always the ImportDeclContext one we start the definition

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0db2934b0fa9: [ASTImporter] Modify ImportDefiniton for 
ObjCInterfaceDecl so that we always… (authored by shafik).
Herald added projects: clang, LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83972

Files:
  clang/lib/AST/ASTImporter.cpp
  lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py


Index: lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
===
--- lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
+++ lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
@@ -14,9 +14,8 @@
 lldbutil.run_to_source_breakpoint(self, "// break here", 
lldb.SBFileSpec("main.m"))
 
 self.expect_expr("chb->hb->field1", result_type="unsigned int", 
result_value="0")
-
-## FIXME field2 should have a value of 1
-self.expect("expr chb->hb->field2", matching=False, substrs = ["= 1"]) 
# this must happen second
+## This should happen second
+self.expect_expr("chb->hb->field2", result_type="unsigned int", 
result_value="1")
 
 self.expect_expr("hb2->field1", result_type="unsigned int", 
result_value="10")
 self.expect_expr("hb2->field2", result_type="unsigned int", 
result_value="3")
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -4758,11 +4758,10 @@
   return ToImplOrErr.takeError();
   }
 
-  if (shouldForceImportDeclContext(Kind)) {
-// Import all of the members of this class.
-if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
-  return Err;
-  }
+  // Import all of the members of this class.
+  if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
+return Err;
+
   return Error::success();
 }
 


Index: lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
===
--- lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
+++ lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py
@@ -14,9 +14,8 @@
 lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.m"))
 
 self.expect_expr("chb->hb->field1", result_type="unsigned int", result_value="0")
-
-## FIXME field2 should have a value of 1
-self.expect("expr chb->hb->field2", matching=False, substrs = ["= 1"]) # this must happen second
+## This should happen second
+self.expect_expr("chb->hb->field2", result_type="unsigned int", result_value="1")
 
 self.expect_expr("hb2->field1", result_type="unsigned int", result_value="10")
 self.expect_expr("hb2->field2", result_type="unsigned int", result_value="3")
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -4758,11 +4758,10 @@
   return ToImplOrErr.takeError();
   }
 
-  if (shouldForceImportDeclContext(Kind)) {
-// Import all of the members of this class.
-if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
-  return Err;
-  }
+  // Import all of the members of this class.
+  if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
+return Err;
+
   return Error::success();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I am open to different approaches to this problem, this is opening attempt at 
fixing it but I may be missing other interactions.

AFAICT starting the definition of `ToRecord` the way  `CompleteDecl(…)`  does 
when we know `FromRecord` is not defined is just broken for the `RecordDecl` 
case if we have bases.

It took me a lot of time to come up with this reproducer from a real life issue 
debugging `llc`.


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

https://reviews.llvm.org/D78000



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

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: martong, balazske, teemperor, a_sidorin.
Herald added a subscriber: rnkovacs.
Herald added a reviewer: a.sidorin.
shafik added a comment.

I am open to different approaches to this problem, this is opening attempt at 
fixing it but I may be missing other interactions.

AFAICT starting the definition of `ToRecord` the way  `CompleteDecl(…)`  does 
when we know `FromRecord` is not defined is just broken for the `RecordDecl` 
case if we have bases.

It took me a lot of time to come up with this reproducer from a real life issue 
debugging `llc`.


In `ImportContext(…)` we may call into `CompleteDecl(…)` which if `FromRecrord` 
is not defined will start the definition of a `ToRecord` but from what I can 
tell at least one of the paths though here don't ensure we complete the 
definition. 
For a `RecordDecl` this can be problematic since this means we won’t import 
base classes and we won’t have any of the methods or types we inherit from 
these bases.

One such path is through `VisitTypedefNameDecl(…)` which is exercised by the 
reproducer.

For LLDB expression parsing this results in expressions failing but sometimes 
succeeding in subsequent attempts e.g.:

  (lldb) p BB->dump()
  error: no member named 'dump' in 'B'
  (lldb) p BB->dump()
  (lldb) 

This happens because the first time through `FromRecord` is not defined but in 
subsequent attempts through it may be.


https://reviews.llvm.org/D78000

Files:
  clang/lib/AST/ASTImporter.cpp
  
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
  
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp


Index: 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,36 @@
+struct B {
+  int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+  // Introduce a TypedefNameDecl
+  using Iterator = D *;
+};
+
+struct C {
+  // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+  // the DeclContext of C.
+  // We will invoke ImportContext(...) which should force the From Decl to
+  // be defined if it not already defined. We will then Import the definition
+  // to the To Decl.
+  // This will force processing of the base class of D which allows us to see
+  // base class methods such as dump().
+  D::Iterator iter;
+
+  bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int",
+ // result_value="42")
+  }
+};
+
+int main() {
+  C c;
+  D d;
+
+  c.f(&d);
+
+  return 0;
+}
Index: 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8166,7 +8166,19 @@
   FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
 return std::move(Err);
 } else {
-  CompleteDecl(ToRecord);
+  // If FromRecord is not defined we need to force it to be.
+  // Simply calling CompleteDecl(...) for a RecordDecl will break some 
cases
+  // it will start the definition but we never finish it.
+  // If a RecordDecl has base classes they won't be imported and we will
+  // be missing anything that we inherit from those bases.
+  if (FromRecord->hasExternalLexicalStorage() &&
+  !FromRecord->isCompleteDefinition())
+FromRecord->getASTContext().getExternalSource()->CompleteType(
+FromRecord);
+
+  if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+  FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+return std::move(Err);
 }
   } else if (auto *ToEnum = dyn_cast(ToDC)) {
 auto *FromEnum = cast(FromDC);


Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,36 @@
+struct B {
+  int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 257552.
shafik marked 4 inline comments as done.
shafik added a comment.

Addressing comments


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

https://reviews.llvm.org/D78000

Files:
  clang/lib/AST/ASTImporter.cpp
  
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
  
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp


Index: 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,35 @@
+struct B {
+  int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+  // Introduce a TypedefNameDecl
+  using Iterator = D *;
+};
+
+struct C {
+  // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+  // the DeclContext of C.
+  // We will invoke ImportContext(...) which should force the From Decl to
+  // be defined if it not already defined. We will then Import the definition
+  // to the To Decl.
+  // This will force processing of the base class of D which allows us to see
+  // base class methods such as dump().
+  D::Iterator iter;
+
+  bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int", 
result_value="42")
+  }
+};
+
+int main() {
+  C c;
+  D d;
+
+  c.f(&d);
+
+  return 0;
+}
Index: 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8166,7 +8166,20 @@
   FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
 return std::move(Err);
 } else {
-  CompleteDecl(ToRecord);
+  // If FromRecord is not defined we need to force it to be.
+  // Simply calling CompleteDecl(...) for a RecordDecl will break some 
cases
+  // it will start the definition but we never finish it.
+  // If there are base classes they won't be imported and we will
+  // be missing anything that we inherit from those bases.
+  if (FromRecord->hasExternalLexicalStorage() &&
+  !FromRecord->isCompleteDefinition())
+FromRecord->getASTContext().getExternalSource()->CompleteType(
+FromRecord);
+
+  if (FromRecord->isCompleteDefinition())
+if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+  return std::move(Err);
 }
   } else if (auto *ToEnum = dyn_cast(ToDC)) {
 auto *FromEnum = cast(FromDC);


Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,35 @@
+struct B {
+  int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+  // Introduce a TypedefNameDecl
+  using Iterator = D *;
+};
+
+struct C {
+  // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+  // the DeclContext of C.
+  // We will invoke ImportContext(...) which should force the From Decl to
+  // be defined if it not already defined. We will then Import the definition
+  // to the To Decl.
+  // This will force processing of the base class of D which allows us to see
+  // base class methods such as dump().
+  D::Iterator iter;
+
+  bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int", result_value="42")
+  }
+};
+
+int main() {
+  C c;
+  D d;
+
+  c.f(&d);
+
+  return 0;
+}
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/lib/AST/ASTImporter.cpp

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: teemperor, jingham, jasonmolenda, friss.
Herald added a subscriber: kristof.beyls.
shafik marked an inline comment as done.
shafik added a subscriber: rsmith.
shafik added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1190
   if (UseExternalLayout) {
-// FIXME: This appears to be reversed.
 if (Base->IsVirtual)

@rsmith you were correct, this was indeed reversed.


Currently the `ItaniumRecordLayoutBuilder` when laying out base classes has the 
virtual and non-virtual bases mixed up when pulling the base class layouts from 
the external source.

This came up in an LLDB bug where on arm64 because of differences in how it 
deals with tail padding would layout the bases differently without the correct 
layout from the external source (LLDB). This would result in some fields being 
off by 4 bytes.


https://reviews.llvm.org/D83008

Files:
  clang/lib/AST/RecordLayoutBuilder.cpp
  lldb/test/Shell/Expr/Inputs/layout.cpp
  lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test


Index: lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
===
--- /dev/null
+++ lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
@@ -0,0 +1,7 @@
+# RUN: %clangxx_host %p/Inputs/layout.cpp -g -o %t
+
+# RUN: %lldb %t -b -s %s | FileCheck %s
+
+expr (intptr_t)&sg.ID - (intptr_t)&sg
+# CHECK: (lldb) expr (intptr_t)&sg.ID - (intptr_t)&sg
+# CHECK: (long) $0 = 12
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,30 @@
+#include 
+
+struct B1 {
+  uint8_t a;
+};
+
+struct D1 : public B1 {
+  uint8_t a;
+  uint32_t ID;
+  uint8_t b;
+};
+
+struct Mixin : public D1 {
+  uint8_t a;
+  uint32_t *arr[3];
+};
+
+struct B2 {
+  uint32_t a;
+};
+
+class D2 : public B2, public Mixin {};
+
+D2 sg;
+
+int main() {
+  D2 s;
+
+  return s.ID;
+}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1187,11 +1187,10 @@
   // Query the external layout to see if it provides an offset.
   bool HasExternalLayout = false;
   if (UseExternalLayout) {
-// FIXME: This appears to be reversed.
 if (Base->IsVirtual)
-  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, 
Offset);
-else
   HasExternalLayout = External.getExternalVBaseOffset(Base->Class, Offset);
+else
+  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, 
Offset);
   }
 
   // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.


Index: lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
===
--- /dev/null
+++ lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
@@ -0,0 +1,7 @@
+# RUN: %clangxx_host %p/Inputs/layout.cpp -g -o %t
+
+# RUN: %lldb %t -b -s %s | FileCheck %s
+
+expr (intptr_t)&sg.ID - (intptr_t)&sg
+# CHECK: (lldb) expr (intptr_t)&sg.ID - (intptr_t)&sg
+# CHECK: (long) $0 = 12
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,30 @@
+#include 
+
+struct B1 {
+  uint8_t a;
+};
+
+struct D1 : public B1 {
+  uint8_t a;
+  uint32_t ID;
+  uint8_t b;
+};
+
+struct Mixin : public D1 {
+  uint8_t a;
+  uint32_t *arr[3];
+};
+
+struct B2 {
+  uint32_t a;
+};
+
+class D2 : public B2, public Mixin {};
+
+D2 sg;
+
+int main() {
+  D2 s;
+
+  return s.ID;
+}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1187,11 +1187,10 @@
   // Query the external layout to see if it provides an offset.
   bool HasExternalLayout = false;
   if (UseExternalLayout) {
-// FIXME: This appears to be reversed.
 if (Base->IsVirtual)
-  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, Offset);
-else
   HasExternalLayout = External.getExternalVBaseOffset(Base->Class, Offset);
+else
+  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, Offset);
   }
 
   // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked an inline comment as done.
shafik added a subscriber: rsmith.
shafik added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1190
   if (UseExternalLayout) {
-// FIXME: This appears to be reversed.
 if (Base->IsVirtual)

@rsmith you were correct, this was indeed reversed.


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

https://reviews.llvm.org/D83008



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

[PATCH] D75740: [ASTImporter] Corrected import of repeated friend declarations.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75740



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

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 275865.
shafik added a comment.

Adding a second test that is not arm64 specific.


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

https://reviews.llvm.org/D83008

Files:
  clang/lib/AST/RecordLayoutBuilder.cpp
  lldb/test/Shell/Expr/Inputs/layout.cpp
  lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test


Index: lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
===
--- /dev/null
+++ lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
@@ -0,0 +1,11 @@
+# RUN: %clangxx_host %p/Inputs/layout.cpp -g -o %t
+
+# RUN: %lldb %t -b -s %s | FileCheck %s
+
+expr (intptr_t)&d2g.ID - (intptr_t)&d2g
+# CHECK: (lldb) expr (intptr_t)&d2g.ID - (intptr_t)&d2g
+# CHECK: (long) $0 = 12
+
+expr (intptr_t)&d3g.f2 - (intptr_t)&d3g
+# CHECK: (lldb) expr (intptr_t)&d3g.f2 - (intptr_t)&d3g
+# CHECK: (long) $1 = 8
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,43 @@
+#include 
+
+struct B1 {
+  uint8_t a;
+};
+
+struct D1 : public B1 {
+  uint8_t a;
+  uint32_t ID;
+  uint8_t b;
+};
+
+struct Mixin : public D1 {
+  uint8_t a;
+  uint32_t *arr[3];
+};
+
+struct B2 {
+  uint32_t a;
+};
+
+class D2 : public B2, public Mixin {};
+
+struct B3 {
+  char f1;
+};
+
+struct alignas(8) B4 {
+  char f2;
+};
+
+struct D3 : B3, B4 {
+};
+
+D2 d2g;
+D3 d3g;
+
+int main() {
+  D2 d2;
+  D3 d3;
+
+  return d2.ID + d3.f2;
+}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1187,11 +1187,10 @@
   // Query the external layout to see if it provides an offset.
   bool HasExternalLayout = false;
   if (UseExternalLayout) {
-// FIXME: This appears to be reversed.
 if (Base->IsVirtual)
-  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, 
Offset);
-else
   HasExternalLayout = External.getExternalVBaseOffset(Base->Class, Offset);
+else
+  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, 
Offset);
   }
 
   // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.


Index: lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
===
--- /dev/null
+++ lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
@@ -0,0 +1,11 @@
+# RUN: %clangxx_host %p/Inputs/layout.cpp -g -o %t
+
+# RUN: %lldb %t -b -s %s | FileCheck %s
+
+expr (intptr_t)&d2g.ID - (intptr_t)&d2g
+# CHECK: (lldb) expr (intptr_t)&d2g.ID - (intptr_t)&d2g
+# CHECK: (long) $0 = 12
+
+expr (intptr_t)&d3g.f2 - (intptr_t)&d3g
+# CHECK: (lldb) expr (intptr_t)&d3g.f2 - (intptr_t)&d3g
+# CHECK: (long) $1 = 8
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,43 @@
+#include 
+
+struct B1 {
+  uint8_t a;
+};
+
+struct D1 : public B1 {
+  uint8_t a;
+  uint32_t ID;
+  uint8_t b;
+};
+
+struct Mixin : public D1 {
+  uint8_t a;
+  uint32_t *arr[3];
+};
+
+struct B2 {
+  uint32_t a;
+};
+
+class D2 : public B2, public Mixin {};
+
+struct B3 {
+  char f1;
+};
+
+struct alignas(8) B4 {
+  char f2;
+};
+
+struct D3 : B3, B4 {
+};
+
+D2 d2g;
+D3 d3g;
+
+int main() {
+  D2 d2;
+  D3 d3;
+
+  return d2.ID + d3.f2;
+}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1187,11 +1187,10 @@
   // Query the external layout to see if it provides an offset.
   bool HasExternalLayout = false;
   if (UseExternalLayout) {
-// FIXME: This appears to be reversed.
 if (Base->IsVirtual)
-  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, Offset);
-else
   HasExternalLayout = External.getExternalVBaseOffset(Base->Class, Offset);
+else
+  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, Offset);
   }
 
   // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D83008#2131776 , @teemperor wrote:

> Thanks for tracking this down, this is a really nasty bug...
>
> The fix itself is obviously fine, but I think I'm out of the loop regarding 
> the testing strategy. We talked about adding a Clang test for this with the 
> help of this layout overwrite JSON file. I assume that extending this to 
> cover virtual bases turned out to be more complicated than expected? FWIW, 
> I'm not necessarily the biggest fan of this Clang test option so I would be 
> fine if we leave it as-is.
>
> I think having an LLDB test is a good idea, but it's not clear why it's a 
> Shell test. If I understand correctly this test requires running on arm64 
> (so, a remote test target), but from what I remember all the remote platform 
> support is only in dotest.py? Also pretty much all other expression 
> evaluation tests and the utilities for that are in the Python/API test 
> infrastructure, so it's a bit out of place.
>
> Also I think the test can be in general much simpler than an arm64-specific 
> test. We get all base class offsets wrong in LLDB, so we can just write a 
> simple test where you change the layout of some structs in a way that it 
> doesn't fit the default layout. E.g., just putting `alignas` on a base class 
> and then reading fields should be enough to trigger the same bug.


Good idea using `alignas` that actually did the trick, I was having trouble 
getting this to reproduce otherwise. I added a second test which should also 
reproduce on non-arm64 cases but I will keep the original test as well since 
they are hitting the bug from slightly different paths.

The goal of using a shell test was to avoid writing a device specific test and 
even though the first case should also pass on all other platforms.


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

https://reviews.llvm.org/D83008



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