[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: cfe/trunk/unittests/AST/ASTImporterTest.cpp:100
 
+  // This traverses the AST to catch certain bugs like poorly or not
+  // implemented subtrees.

r.stahl wrote:
> a.sidorin wrote:
> > I just saw this change and I cannot find the reason, why do we need to 
> > print the imported declaration after we have printed the entire translation 
> > unit? Is there some special case?
> Sorry for the late reply.
> 
> The particular bug here would only be detected when dumping, but not when 
> printing. The output of printing was just not listing the missing AST nodes, 
> but dumping crashed and therefore failed the test as expected.
> 
> I'm not familiar with the inner workings of print and dump, so I cannot 
> explain why that is.
I have a rough guess that this might be because the code buffer that is used in 
`vfs::InMemoryFileSystem` has a reference to the actual code string. That 
actual code string is a string literal in most of the cases which should be in 
the data segment, but there is no guarantee for that. E.g. some optimizing 
compilers might put it onto the stack.
We experienced a similar issue during building a test Fixture 
(https://reviews.llvm.org/D43967).

What I suggest is to build the `ASTTests` target with asan and ubsan enabled 
(set LLVM_USE_SANITIZER for cmake) and trigger the dump which caused the bug.


Repository:
  rL LLVM

https://reviews.llvm.org/D38943



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

[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl added inline comments.
Herald added a subscriber: martong.



Comment at: cfe/trunk/unittests/AST/ASTImporterTest.cpp:100
 
+  // This traverses the AST to catch certain bugs like poorly or not
+  // implemented subtrees.

a.sidorin wrote:
> I just saw this change and I cannot find the reason, why do we need to print 
> the imported declaration after we have printed the entire translation unit? 
> Is there some special case?
Sorry for the late reply.

The particular bug here would only be detected when dumping, but not when 
printing. The output of printing was just not listing the missing AST nodes, 
but dumping crashed and therefore failed the test as expected.

I'm not familiar with the inner workings of print and dump, so I cannot explain 
why that is.


Repository:
  rL LLVM

https://reviews.llvm.org/D38943



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

[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added inline comments.
Herald added subscribers: llvm-commits, rnkovacs.



Comment at: cfe/trunk/unittests/AST/ASTImporterTest.cpp:100
 
+  // This traverses the AST to catch certain bugs like poorly or not
+  // implemented subtrees.

I just saw this change and I cannot find the reason, why do we need to print 
the imported declaration after we have printed the entire translation unit? Is 
there some special case?


Repository:
  rL LLVM

https://reviews.llvm.org/D38943



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

[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL316069: [ASTImporter] Import SubStmt of CaseStmt (authored 
by xazax).

Changed prior to commit:
  https://reviews.llvm.org/D38943?vs=119162=119444#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38943

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


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -3983,12 +3983,16 @@
   Expr *ToRHS = Importer.Import(S->getRHS());
   if (!ToRHS && S->getRHS())
 return nullptr;
+  Stmt *ToSubStmt = Importer.Import(S->getSubStmt());
+  if (!ToSubStmt && S->getSubStmt())
+return nullptr;
   SourceLocation ToCaseLoc = Importer.Import(S->getCaseLoc());
   SourceLocation ToEllipsisLoc = Importer.Import(S->getEllipsisLoc());
   SourceLocation ToColonLoc = Importer.Import(S->getColonLoc());
-  return new (Importer.getToContext()) CaseStmt(ToLHS, ToRHS,
-ToCaseLoc, ToEllipsisLoc,
-ToColonLoc);
+  CaseStmt *ToStmt = new (Importer.getToContext())
+  CaseStmt(ToLHS, ToRHS, ToCaseLoc, ToEllipsisLoc, ToColonLoc);
+  ToStmt->setSubStmt(ToSubStmt);
+  return ToStmt;
 }
 
 Stmt *ASTNodeImporter::VisitDefaultStmt(DefaultStmt *S) {
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -97,6 +97,10 @@
   llvm::raw_svector_ostream ToNothing(ImportChecker);
   ToCtx.getTranslationUnitDecl()->print(ToNothing);
 
+  // This traverses the AST to catch certain bugs like poorly or not
+  // implemented subtrees.
+  Imported->dump(ToNothing);
+
   return Verifier.match(Imported, AMatcher);
 }
 
@@ -267,6 +271,15 @@
   
hasUnaryOperand(cxxThisExpr();
 }
 
+TEST(ImportExpr, ImportSwitch) {
+  MatchVerifier Verifier;
+  EXPECT_TRUE(
+  testImport("void declToImport() { int b; switch (b) { case 1: break; } 
}",
+ Lang_CXX, "", Lang_CXX, Verifier,
+ functionDecl(hasBody(compoundStmt(
+ has(switchStmt(has(compoundStmt(has(caseStmt()));
+}
+
 TEST(ImportExpr, ImportStmtExpr) {
   MatchVerifier Verifier;
   // NOTE: has() ignores implicit casts, using hasDescendant() to match it


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -3983,12 +3983,16 @@
   Expr *ToRHS = Importer.Import(S->getRHS());
   if (!ToRHS && S->getRHS())
 return nullptr;
+  Stmt *ToSubStmt = Importer.Import(S->getSubStmt());
+  if (!ToSubStmt && S->getSubStmt())
+return nullptr;
   SourceLocation ToCaseLoc = Importer.Import(S->getCaseLoc());
   SourceLocation ToEllipsisLoc = Importer.Import(S->getEllipsisLoc());
   SourceLocation ToColonLoc = Importer.Import(S->getColonLoc());
-  return new (Importer.getToContext()) CaseStmt(ToLHS, ToRHS,
-ToCaseLoc, ToEllipsisLoc,
-ToColonLoc);
+  CaseStmt *ToStmt = new (Importer.getToContext())
+  CaseStmt(ToLHS, ToRHS, ToCaseLoc, ToEllipsisLoc, ToColonLoc);
+  ToStmt->setSubStmt(ToSubStmt);
+  return ToStmt;
 }
 
 Stmt *ASTNodeImporter::VisitDefaultStmt(DefaultStmt *S) {
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -97,6 +97,10 @@
   llvm::raw_svector_ostream ToNothing(ImportChecker);
   ToCtx.getTranslationUnitDecl()->print(ToNothing);
 
+  // This traverses the AST to catch certain bugs like poorly or not
+  // implemented subtrees.
+  Imported->dump(ToNothing);
+
   return Verifier.match(Imported, AMatcher);
 }
 
@@ -267,6 +271,15 @@
   hasUnaryOperand(cxxThisExpr();
 }
 
+TEST(ImportExpr, ImportSwitch) {
+  MatchVerifier Verifier;
+  EXPECT_TRUE(
+  testImport("void declToImport() { int b; switch (b) { case 1: break; } }",
+ Lang_CXX, "", Lang_CXX, Verifier,
+ functionDecl(hasBody(compoundStmt(
+ has(switchStmt(has(compoundStmt(has(caseStmt()));
+}
+
 TEST(ImportExpr, ImportStmtExpr) {
   MatchVerifier Verifier;
   // NOTE: has() ignores implicit casts, using hasDescendant() to match it
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl added a comment.

If all is good, I will need someone to commit this for me please.




Comment at: unittests/AST/ASTImporterTest.cpp:100
 
+  // This might catch other cases.
+  Imported->dump(ToNothing);

xazax.hun wrote:
> r.stahl wrote:
> > xazax.hun wrote:
> > > I would elaborate a bit more on the purpose of the code below.
> > I will need help for that, since I do not have a better rationale myself. 
> > My guess would be that print has different assumptions and error checking 
> > than dump, so executing both increases coverage.
> So the reason you need this to traverse the AST and crash on the null 
> pointers (when the patch is not applied right?)
> I think you could comment something like traverse the AST to catch certain 
> bugs like poorly (not) imported subtrees. 
> If we do not want to actually print the whole tree (because it might be 
> noisy) you can also try using a no-op recursive AST visitor.
Yes, of course this referred to the case when the patch is not applied.

There is no noise since the dump is done on a buffer that will be discarded.


https://reviews.llvm.org/D38943



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

[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: unittests/AST/ASTImporterTest.cpp:100
 
+  // This might catch other cases.
+  Imported->dump(ToNothing);

r.stahl wrote:
> xazax.hun wrote:
> > I would elaborate a bit more on the purpose of the code below.
> I will need help for that, since I do not have a better rationale myself. My 
> guess would be that print has different assumptions and error checking than 
> dump, so executing both increases coverage.
So the reason you need this to traverse the AST and crash on the null pointers 
(when the patch is not applied right?)
I think you could comment something like traverse the AST to catch certain bugs 
like poorly (not) imported subtrees. 
If we do not want to actually print the whole tree (because it might be noisy) 
you can also try using a no-op recursive AST visitor.


https://reviews.llvm.org/D38943



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

[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl added a comment.

Thanks for the fast response. See inline comment.




Comment at: unittests/AST/ASTImporterTest.cpp:100
 
+  // This might catch other cases.
+  Imported->dump(ToNothing);

xazax.hun wrote:
> I would elaborate a bit more on the purpose of the code below.
I will need help for that, since I do not have a better rationale myself. My 
guess would be that print has different assumptions and error checking than 
dump, so executing both increases coverage.


https://reviews.llvm.org/D38943



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

[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt

[Gábor Horváth via Phabricator via cfe-commits]

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

LGTM with a nit.




Comment at: unittests/AST/ASTImporterTest.cpp:100
 
+  // This might catch other cases.
+  Imported->dump(ToNothing);

I would elaborate a bit more on the purpose of the code below.


https://reviews.llvm.org/D38943



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

[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl created this revision.

This fixes importing of CaseStmts, because the importer was not importing its 
SubStmt.

A test was added and the test procedure was adjusted to also dump the imported 
Decl, because otherwise the bug would not be detected.


https://reviews.llvm.org/D38943

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -97,6 +97,9 @@
   llvm::raw_svector_ostream ToNothing(ImportChecker);
   ToCtx.getTranslationUnitDecl()->print(ToNothing);
 
+  // This might catch other cases.
+  Imported->dump(ToNothing);
+
   return Verifier.match(Imported, AMatcher);
 }
 
@@ -267,6 +270,15 @@
   
hasUnaryOperand(cxxThisExpr();
 }
 
+TEST(ImportExpr, ImportSwitch) {
+  MatchVerifier Verifier;
+  EXPECT_TRUE(
+  testImport("void declToImport() { int b; switch (b) { case 1: break; } 
}",
+ Lang_CXX, "", Lang_CXX, Verifier,
+ functionDecl(hasBody(compoundStmt(
+ has(switchStmt(has(compoundStmt(has(caseStmt()));
+}
+
 TEST(ImportExpr, ImportStmtExpr) {
   MatchVerifier Verifier;
   // NOTE: has() ignores implicit casts, using hasDescendant() to match it
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -3983,12 +3983,16 @@
   Expr *ToRHS = Importer.Import(S->getRHS());
   if (!ToRHS && S->getRHS())
 return nullptr;
+  Stmt *ToSubStmt = Importer.Import(S->getSubStmt());
+  if (!ToSubStmt && S->getSubStmt())
+return nullptr;
   SourceLocation ToCaseLoc = Importer.Import(S->getCaseLoc());
   SourceLocation ToEllipsisLoc = Importer.Import(S->getEllipsisLoc());
   SourceLocation ToColonLoc = Importer.Import(S->getColonLoc());
-  return new (Importer.getToContext()) CaseStmt(ToLHS, ToRHS,
-ToCaseLoc, ToEllipsisLoc,
-ToColonLoc);
+  CaseStmt *ToStmt = new (Importer.getToContext())
+  CaseStmt(ToLHS, ToRHS, ToCaseLoc, ToEllipsisLoc, ToColonLoc);
+  ToStmt->setSubStmt(ToSubStmt);
+  return ToStmt;
 }
 
 Stmt *ASTNodeImporter::VisitDefaultStmt(DefaultStmt *S) {


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -97,6 +97,9 @@
   llvm::raw_svector_ostream ToNothing(ImportChecker);
   ToCtx.getTranslationUnitDecl()->print(ToNothing);
 
+  // This might catch other cases.
+  Imported->dump(ToNothing);
+
   return Verifier.match(Imported, AMatcher);
 }
 
@@ -267,6 +270,15 @@
   hasUnaryOperand(cxxThisExpr();
 }
 
+TEST(ImportExpr, ImportSwitch) {
+  MatchVerifier Verifier;
+  EXPECT_TRUE(
+  testImport("void declToImport() { int b; switch (b) { case 1: break; } }",
+ Lang_CXX, "", Lang_CXX, Verifier,
+ functionDecl(hasBody(compoundStmt(
+ has(switchStmt(has(compoundStmt(has(caseStmt()));
+}
+
 TEST(ImportExpr, ImportStmtExpr) {
   MatchVerifier Verifier;
   // NOTE: has() ignores implicit casts, using hasDescendant() to match it
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -3983,12 +3983,16 @@
   Expr *ToRHS = Importer.Import(S->getRHS());
   if (!ToRHS && S->getRHS())
 return nullptr;
+  Stmt *ToSubStmt = Importer.Import(S->getSubStmt());
+  if (!ToSubStmt && S->getSubStmt())
+return nullptr;
   SourceLocation ToCaseLoc = Importer.Import(S->getCaseLoc());
   SourceLocation ToEllipsisLoc = Importer.Import(S->getEllipsisLoc());
   SourceLocation ToColonLoc = Importer.Import(S->getColonLoc());
-  return new (Importer.getToContext()) CaseStmt(ToLHS, ToRHS,
-ToCaseLoc, ToEllipsisLoc,
-ToColonLoc);
+  CaseStmt *ToStmt = new (Importer.getToContext())
+  CaseStmt(ToLHS, ToRHS, ToCaseLoc, ToEllipsisLoc, ToColonLoc);
+  ToStmt->setSubStmt(ToSubStmt);
+  return ToStmt;
 }
 
 Stmt *ASTNodeImporter::VisitDefaultStmt(DefaultStmt *S) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits