[ASTImporter] Fix crash caused by unimported type of FromatAttr

[martong]

Summary

During the import of FormatAttrs we forgot to import the type (e.g
__scanf__) of the attribute. This caused a segfault when we wanted to
traverse the AST (e.g. by the dump() method).

Here is the relevant valgrind trace of the ASTMerge test when accessing
an already freed memory region:

==1941== Invalid read of size 8
==1941==    at 0xB112EE6: clang::TextNodeDumper::VisitFormatAttr(clang::FormatAttr const*) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangAST.so.12git)
==1941==    by 0xB11CB6C: clang::attrvisitor::Base<llvm::make_const_ptr, clang::TextNodeDumper, void>::Visit(clang::Attr const*) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangAST.so.12git)
==1941==    by 0xADC0880: std::_Function_handler<void (bool), void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Attr const*)::{lambda()#1}>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Attr const*)::{lambda()#1})::{lambda(bool)#1}>::_M_invoke(std::_Any_data const&, bool&&) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangAST.so.12git)
==1941==    by 0xADC9DAD: std::_Function_handler<void (bool), void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::{lambda()#1}>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::{lambda()#1})::{lambda(bool)#1}>::_M_invoke(std::_Any_data const&, bool&&) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangAST.so.12git)
==1941==    by 0xADBE58A: void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::{lambda()#1}>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::{lambda()#1}) [clone .constprop.2050] (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangAST.so.12git)
==1941==    by 0xADBE2BA: clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::{lambda()#1}::operator()() const (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangAST.so.12git)
==1941==    by 0xADBE706: void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::{lambda()#1}>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::{lambda()#1}) [clone .constprop.2050] (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangAST.so.12git)
==1941==    by 0xADD4226: clang::Decl::dump(llvm::raw_ostream&, bool, clang::ASTDumpOutputFormat) const (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangAST.so.12git)
==1941==    by 0x73CA1B5: (anonymous namespace)::ASTPrinter::HandleTranslationUnit(clang::ASTContext&) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==1941==    by 0xB84A568: clang::ParseAST(clang::Sema&, bool, bool) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangParse.so.12git)
==1941==    by 0x7401AA0: clang::ASTMergeAction::ExecuteAction() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==1941==    by 0x747C298: clang::FrontendAction::Execute() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==1941==  Address 0xe42fab8 is 3,576 bytes inside a block of size 4,096 free'd
==1941==    at 0x4C3123B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1941==    by 0xB4A4D3F: clang::Preprocessor::~Preprocessor() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangLex.so.12git)
==1941==    by 0x7402D6C: std::_Sp_counted_deleter<clang::Preprocessor*, std::__shared_ptr<clang::Preprocessor, (__gnu_cxx::_Lock_policy)2>::_Deleter<std::allocator<clang::Preprocessor> >, std::allocator<clang::Preprocessor>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==1941==    by 0x57548D5: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangCodeGen.so.12git)
==1941==    by 0x741DB08: clang::ASTUnit::~ASTUnit() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==1941==    by 0x7401CFF: clang::ASTMergeAction::ExecuteAction() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==1941==    by 0x747C298: clang::FrontendAction::Execute() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==1941==    by 0x742EA69: clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==1941==    by 0x40DC565: clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontendTool.so.12git)
==1941==    by 0x11BE7B: cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (in /home/egbomrt/WORK/llvm5/build/release_assert/bin/clang-11)
==1941==    by 0x116E38: ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) (in /home/egbomrt/WORK/llvm5/build/release_assert/bin/clang-11)
==1941==    by 0x114DC3: main (in /home/egbomrt/WORK/llvm5/build/release_assert/bin/clang-11)
==1941==  Block was alloc'd at
==1941==    at 0x4C3017F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1941==    by 0x8709BE7: clang::IdentifierTable::get(llvm::StringRef) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangBasic.so.12git)
==1941==    by 0x8729BFC: AddKeyword(llvm::StringRef, clang::tok::TokenKind, unsigned int, clang::LangOptions const&, clang::IdentifierTable&) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangBasic.so.12git)
==1941==    by 0x872B71C: clang::IdentifierTable::AddKeywords(clang::LangOptions const&) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangBasic.so.12git)
==1941==    by 0x7418AF9: (anonymous namespace)::ASTInfoCollector::ReadTargetOptions(clang::TargetOptions const&, bool, bool) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==1941==    by 0x786502C: clang::ASTReader::ParseTargetOptions(llvm::SmallVector<unsigned long, 64u> const&, bool, clang::ASTReaderListener&, bool) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangSerialization.so.12git)
==1941==    by 0x7871E84: clang::ASTReader::ReadOptionsBlock(llvm::BitstreamCursor&, unsigned int, bool, clang::ASTReaderListener&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangSerialization.so.12git)
==1941==    by 0x787608D: clang::ASTReader::ReadControlBlock(clang::serialization::ModuleFile&, llvm::SmallVectorImpl<clang::ASTReader::ImportedModule>&, clang::serialization::ModuleFile const*, unsigned int) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangSerialization.so.12git)
==1941==    by 0x787700A: clang::ASTReader::ReadASTCore(llvm::StringRef, clang::serialization::ModuleKind, clang::SourceLocation, clang::serialization::ModuleFile*, llvm::SmallVectorImpl<clang::ASTReader::ImportedModule>&, long, long, clang::ASTFileSignature, unsigned int) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangSerialization.so.12git)
==1941==    by 0x78BDB8E: clang::ASTReader::ReadAST(llvm::StringRef, clang::serialization::ModuleKind, clang::SourceLocation, unsigned int, llvm::SmallVectorImpl<clang::ASTReader::ImportedSubmodule>*) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangSerialization.so.12git)
==1941==    by 0x741EDC6: clang::ASTUnit::LoadFromASTFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, clang::PCHContainerReader const&, clang::ASTUnit::WhatToLoad, llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine>, clang::FileSystemOptions const&, bool, bool, llvm::ArrayRef<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, llvm::MemoryBuffer*> >, clang::CaptureDiagsKind, bool, bool) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==1941==    by 0x7401A10: clang::ASTMergeAction::ExecuteAction() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)

[teemperor]

LGTM, thanks!

clang/test/ASTMerge/attr/Inputs/FormatAttr.cpp

3

(Not sure if we care about that in tests, but that's technically not in LLVM code style)

[shafik]

LGTM

[martong]

Thanks for the review!

clang/test/ASTMerge/attr/Inputs/FormatAttr.cpp

3

Yeah, actually clang-format indicated this as well, so just now I changed the code to the clang-formatted one.

clang/lib/AST/ASTImporter.cpp

if (From->isAlignmentExpr()) {
         return ToTOrErr.takeError();
     }
     To->setInherited(From->isInherited());
     To->setPackExpansion(From->isPackExpansion());
     To->setImplicit(From->isImplicit());
     ToAttr = To;
     break;
   }
+  case attr::Format: {
+    const auto *From = cast<FormatAttr>(FromAttr);
+    FormatAttr *To;
+    IdentifierInfo *ToAttrType = Import(From->getType());
+    To = FormatAttr::Create(ToContext, ToAttrType, From->getFormatIdx(),
+                            From->getFirstArg(), ToRange, From->getSyntax());
+    To->setInherited(From->isInherited());
+    ToAttr = To;
+    break;
+  }
   default:
     // FIXME: 'clone' copies every member but some of them should be imported.
     // Handle other Attrs that have parameters that should be imported.
     ToAttr = FromAttr->clone(ToContext);
     ToAttr->setRange(ToRange);
     break;
   }
   assert(ToAttr && "Attribute should be created.");

clang/test/ASTMerge/attr/Inputs/FormatAttr.cpp

+int foo(const char *fmt, ...) __attribute__((__format__(__scanf__, 1, 2)));

clang/test/ASTMerge/attr/testFormatAttr.cpp

+// RUN: %clang -x c++-header -o %t.a.ast %S/Inputs/FormatAttr.cpp
+// RUN: %clang_cc1 -x c++ -ast-merge %t.a.ast /dev/null -ast-dump

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, ImportExprOfAlignmentAttr) {
   auto *ToA = FirstDeclMatcher<CXXRecordDecl>().match(
       ToD->getTranslationUnitDecl(),
       cxxRecordDecl(hasName("A"), unless(isImplicit())));
   // Ensure that 'struct A' was imported (through reference from attribute of
   // 'S').
   EXPECT_TRUE(ToA);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportFormatAttr) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      int foo(const char * fmt, ...)
+      __attribute__ ((__format__ (__scanf__, 1, 2)));
+      )",
+      Lang_CXX03, "input.cc");
+  auto *FromD = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("foo")));
+  ASSERT_TRUE(FromD);
+
+  auto *ToD = Import(FromD, Lang_CXX03);
+  ASSERT_TRUE(ToD);
+  ToD->dump(); // Should not crash!
+
+  auto *FromAttr = FromD->getAttr<FormatAttr>();
+  auto *ToAttr = ToD->getAttr<FormatAttr>();
+  EXPECT_EQ(FromAttr->isInherited(), ToAttr->isInherited());
+  EXPECT_EQ(FromAttr->isPackExpansion(), ToAttr->isPackExpansion());
+  EXPECT_EQ(FromAttr->isImplicit(), ToAttr->isImplicit());
+  EXPECT_EQ(FromAttr->getSyntax(), ToAttr->getSyntax());
+  EXPECT_EQ(FromAttr->getAttributeSpellingListIndex(),
+            ToAttr->getAttributeSpellingListIndex());
+  EXPECT_EQ(FromAttr->getType()->getName(), ToAttr->getType()->getName());
+}
 template <typename T>
 auto ExtendWithOptions(const T &Values, const std::vector<std::string> &Args) {
   auto Copy = Values;
   for (std::vector<std::string> &ArgV : Copy) {
     for (const std::string &Arg : Args) {
       ArgV.push_back(Arg);
     }
   }