[ASTImporter] import SubStmt of CaseStmt

[r.stahl]

Summary

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.

[xazax.hun]

LGTM with a nit.

unittests/AST/ASTImporterTest.cpp

100

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

[r.stahl]

Thanks for the fast response. See inline comment.

unittests/AST/ASTImporterTest.cpp

100

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.

[xazax.hun]

unittests/AST/ASTImporterTest.cpp

100

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.

[r.stahl]

addressed review comment

[r.stahl]

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

unittests/AST/ASTImporterTest.cpp

100

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.

[a.sidorin]

cfe/trunk/unittests/AST/ASTImporterTest.cpp

100 ↗

(On Diff #119444)

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?

[r.stahl]

cfe/trunk/unittests/AST/ASTImporterTest.cpp

100 ↗

(On Diff #119444)

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.

[martong]

cfe/trunk/unittests/AST/ASTImporterTest.cpp

100 ↗

(On Diff #119444)

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.

lib/AST/ASTImporter.cpp

 
 Stmt *ASTNodeImporter::VisitCaseStmt(CaseStmt *S) {
   Expr *ToLHS = Importer.Import(S->getLHS());
   if (!ToLHS)
     return nullptr;
   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) {
   SourceLocation ToDefaultLoc = Importer.Import(S->getDefaultLoc());
   SourceLocation ToColonLoc = Importer.Import(S->getColonLoc());
   Stmt *ToSubStmt = Importer.Import(S->getSubStmt());
   if (!ToSubStmt && S->getSubStmt())
     return nullptr;

unittests/AST/ASTImporterTest.cpp

if (!Imported)
     return testing::AssertionFailure() << "Import failed, nullptr returned!";
 
   // This should dump source locations and assert if some source locations
   // were not imported
   SmallString<1024> ImportChecker;
   llvm::raw_svector_ostream ToNothing(ImportChecker);
   ToCtx.getTranslationUnitDecl()->print(ToNothing);
 
+  // This might catch other cases.
+  Imported->dump(ToNothing);
+
   return Verifier.match(Imported, AMatcher);
 }
 
 TEST(ImportExpr, ImportStringLiteral) {
   MatchVerifier<Decl> Verifier;
   EXPECT_TRUE(testImport("void declToImport() { \"foo\"; }",
                          Lang_CXX, "", Lang_CXX, Verifier,
                          functionDecl(
EXPECT_TRUE(testImport(
               classTemplateDecl(hasTemplateDecl(cxxRecordDecl(hasMethod(allOf(
                   hasName("f"),
                   hasBody(compoundStmt(has(declStmt(hasSingleDecl(
                       varDecl(hasInitializer(parenListExpr(has(unaryOperator(
                           hasOperatorName("*"),
                           hasUnaryOperand(cxxThisExpr()))))))))))))))))))))))));
 }
 
+TEST(ImportExpr, ImportSwitch) {
+  MatchVerifier<Decl> 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<Decl> Verifier;
   // NOTE: has() ignores implicit casts, using hasDescendant() to match it
   EXPECT_TRUE(
         testImport(
           "void declToImport() { int b; int a = b ?: 1; int C = ({int X=4; X;}); }",
           Lang_CXX, "", Lang_CXX, Verifier,
           functionDecl(