[ASTImporter] Fix crash caused by unset AttributeSpellingListIndex

[martong]

Summary

During the import of attributes we forgot to set the spelling list
index. 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:

==25717== Invalid read of size 8
==25717==    at 0x86E96FA: clang::AttributeCommonInfo::calculateAttributeSpellingListIndex() const (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangBasic.so.12git)
==25717==    by 0xAE73598: clang::RestrictAttr::getSpelling() const (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangAST.so.12git)
==25717==    by 0xB11D93B: 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)
==25717==    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)
==25717==    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)
==25717==    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)
==25717==    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)
==25717==    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)
==25717==    by 0xADD4226: clang::Decl::dump(llvm::raw_ostream&, bool, clang::ASTDumpOutputFormat) const (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangAST.so.12git)
==25717==    by 0x73CA1B5: (anonymous namespace)::ASTPrinter::HandleTranslationUnit(clang::ASTContext&) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==25717==    by 0xB84A568: clang::ParseAST(clang::Sema&, bool, bool) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangParse.so.12git)
==25717==    by 0x7401AA0: clang::ASTMergeAction::ExecuteAction() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==25717==  Address 0xe42fa88 is 3,528 bytes inside a block of size 4,096 free'd
==25717==    at 0x4C3123B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==25717==    by 0xB4A4D3F: clang::Preprocessor::~Preprocessor() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangLex.so.12git)
==25717==    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)
==25717==    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)
==25717==    by 0x741DB08: clang::ASTUnit::~ASTUnit() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==25717==    by 0x7401CFF: clang::ASTMergeAction::ExecuteAction() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==25717==    by 0x747C298: clang::FrontendAction::Execute() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==25717==    by 0x742EA69: clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==25717==    by 0x40DC565: clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontendTool.so.12git)
==25717==    by 0x11BE7B: cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (in /home/egbomrt/WORK/llvm5/build/release_assert/bin/clang-11)
==25717==    by 0x116E38: ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) (in /home/egbomrt/WORK/llvm5/build/release_assert/bin/clang-11)
==25717==    by 0x114DC3: main (in /home/egbomrt/WORK/llvm5/build/release_assert/bin/clang-11)
==25717==  Block was alloc'd at
==25717==    at 0x4C3017F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==25717==    by 0x8709BE7: clang::IdentifierTable::get(llvm::StringRef) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangBasic.so.12git)
==25717==    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)
==25717==    by 0x872B71C: clang::IdentifierTable::AddKeywords(clang::LangOptions const&) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangBasic.so.12git)
==25717==    by 0x7418AF9: (anonymous namespace)::ASTInfoCollector::ReadTargetOptions(clang::TargetOptions const&, bool, bool) (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==25717==    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)
==25717==    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)
==25717==    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)
==25717==    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)
==25717==    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)
==25717==    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)
==25717==    by 0x7401A10: clang::ASTMergeAction::ExecuteAction() (in /home/egbomrt/WORK/llvm5/build/release_assert/lib/libclangFrontend.so.12git)
==25717==

[shafik]

LGTM

clang/lib/AST/ASTImporter.cpp

8118

Why move these two but not setInherited(...) it is not really explained in the description.

[martong]

Thanks for the reivew!

clang/lib/AST/ASTImporter.cpp

8118

Yeah, setInherited is a member of InheritableAttr, so the Attr base class does not have it.
Anyway, I changed the code and moved the calls of setInherited to an if block where we dyn_cast to InheritableAttr.

[kastiglione]

@martong hi, this caused a test regression. In TestImportBuiltinFileID.py, this unreachable assertion is hit:

Ignored/unknown shouldn't get here
UNREACHABLE executed at tools/clang/include/clang/Sema/AttrSpellingListIndex.inc:13!

See http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/24213/ for more on the failure.

I will revert shortly if you're not available.

[kastiglione]

@martong here's a partial backtrace:

3: LLDB`llvm::llvm_unreachable_internal(char const*, char const*, unsigned int)
4: LLDB`clang::AttributeCommonInfo::calculateAttributeSpellingListIndex() const
5: LLDB`clang::ASTImporter::Import(clang::Attr const*)
6: LLDB`llvm::Expected<clang::Attr*> clang::ASTNodeImporter::import<clang::Attr>(clang::Attr*)
7: LLDB`clang::ASTNodeImporter::InitializeImportedDecl(clang::Decl*, clang::Decl*)
8: LLDB`bool clang::ASTNodeImporter::GetImportedOrCreateSpecialDecl<clang::FunctionDecl, clang::ASTNodeImporter::CallOverloadedCreateFun<clang::FunctionDecl>, clang::FunctionDecl, clang::ASTContext&, clang::DeclContext*&, clang::SourceLocation&, clang::DeclarationNameInfo&, clang::QualType&, clang::TypeSourceInfo*&, clang::StorageClass, bool, bool, clang::ConstexprSpecKind, clang::Expr*&>(clang::FunctionDecl*&, clang::ASTNodeImporter::CallOverloadedCreateFun<clang::FunctionDecl>, clang::FunctionDecl*, clang::ASTContext&, clang::DeclContext*&, clang::SourceLocation&, clang::DeclarationNameInfo&, clang::QualType&, clang::TypeSourceInfo*&, clang::StorageClass&&, bool&&, bool&&, clang::ConstexprSpecKind&&, clang::Expr*&)
9: LLDB`clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*)

[kastiglione]

Reverted here: 4cb4db11ee1323c5d4bf66d21deb046970f4e516

[martong]

Ups, sorry for the break and thanks for the notice! I am going to investigate.

[martong]

Unfortunately, I could not reproduce the mentioned crash on our macOS machine. The mentioned test just passed with the output below. I gave up.

myuser@msmarple ~/llvm3/build/release_assert $ /usr/local/Frameworks/Python.framework/Versions/3.8/bin/python3.8 /Users/myuser/llvm3/git/llvm-project/lldb/test/API/dotest.py -S nm -u CXXFLAGS -u CFLAGS --codesign-identity - --env LLVM_LIBS_DIR=/Users/myuser/llvm3/build/release_assert/./lib --arch x86_64 --build-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex -s /Users/myuser/llvm3/build/release_assert/lldb-test-traces --lldb-module-cache-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /Users/myuser/llvm3/build/release_assert/./bin/lldb --compiler /Users/myuser/llvm3/build/release_assert/./bin/clang --dsymutil /Users/myuser/llvm3/build/release_assert/./bin/dsymutil --filecheck /Users/myuser/llvm3/build/release_assert/./bin/FileCheck --yaml2obj /Users/myuser/llvm3/build/release_assert/./bin/yaml2obj --server /Users/myuser/llvm3/build/release_assert/./bin/debugserver --lldb-libs-dir /Users/myuser/llvm3/build/release_assert/./lib /Users/myuser/llvm3/git/llvm-project/lldb/test/API/commands/expression/import_builtin_fileid/ -p TestImportBuiltinFileID.py -t
lldb version 12.0.99 (https://github.com/llvm/llvm-project.git revision d454328ea88562a6ec6260529a040035ab9c4a06)
  clang revision d454328ea88562a6ec6260529a040035ab9c4a06
  llvm revision d454328ea88562a6ec6260529a040035ab9c4a06
libstdcxx tests will not be run because: Don't know how to build with libstdcxx on macosx
Skipping following debug info categories: ['dwo']

Session logs for test failures/errors/unexpected successes will go into directory '/Users/myuser/llvm3/build/release_assert/lldb-test-traces'
Change dir to: /Users/myuser/llvm3/git/llvm-project/lldb/test/API/commands/expression/import_builtin_fileid
runCmd: settings clear -all
output:

runCmd: settings set symbols.enable-external-lookup false
output:

runCmd: settings set target.inherit-tcc true
output:

runCmd: settings set target.auto-apply-fixits false
output:

runCmd: settings set plugin.process.gdb-remote.packet-timeout 60
output:

runCmd: settings set symbols.clang-modules-cache-path "/Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-lldb/lldb-api"
output:

runCmd: settings set use-color false
output:

runCmd: setting set target.prefer-dynamic-value no-dynamic-values
output:

runCmd: expr int (*DBG_CGImageGetRenderingIntent)(void *) = ((int (*)(void *))CGImageGetRenderingIntent); DBG_CGImageGetRenderingIntent((void *)0x00000000000000);
output: (int) $0 = 0


Ran command:
"expr int (*DBG_CGImageGetRenderingIntent)(void *) = ((int (*)(void *))CGImageGetRenderingIntent); DBG_CGImageGetRenderingIntent((void *)0x00000000000000);"

Got output:
(int) $0 = 0

Expecting sub string: "$0 = 0" (was found)

<bound method SBProcess.Kill of <lldb.SBProcess; proxy of <Swig Object of type 'lldb::SBProcess *' at 0x106e3f9f0> >>: success

PASS: LLDB (/Users/myuser/llvm3/build/release_assert/bin/clang-x86_64) :: test_import_builtin_fileid_gmodules (TestImportBuiltinFileID.TestImportBuiltinFileID)
Restore dir to: /Users/myuser/llvm3/build/release_assert
----------------------------------------------------------------------
Ran 1 test in 1.789s

RESULT: PASSED (1 passes, 0 failures, 0 errors, 0 skipped, 0 expected failures, 0 unexpected successes)

clang/lib/AST/ASTImporter.cpp

if (From->isAlignmentExpr()) {
       else
         return ToEOrErr.takeError();
     } else {
       if (auto ToTOrErr = Import(From->getAlignmentType()))
         To = CreateAlign(false, *ToTOrErr);
       else
         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.");
-  
+  assert(ToAttr && "Attribute should be created.");
+  if (const auto *InheritableFromAttr = dyn_cast<InheritableAttr>(FromAttr))
+    cast<InheritableAttr>(ToAttr)->setInherited(
+        InheritableFromAttr->isInherited());
+  ToAttr->setAttributeSpellingListIndex(
+      FromAttr->getAttributeSpellingListIndex());
+  ToAttr->setPackExpansion(FromAttr->isPackExpansion());
+  ToAttr->setImplicit(FromAttr->isImplicit());
   return ToAttr;
 }
 
 Decl *ASTImporter::GetAlreadyImportedOrNull(const Decl *FromD) const {
   auto Pos = ImportedDecls.find(FromD);
   if (Pos != ImportedDecls.end())
     return Pos->second;
   else

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

+void *foo(unsigned, unsigned) __attribute__((__malloc__));

clang/test/ASTMerge/attr/testRestrictAttr.cpp

+// RUN: %clang -x c++-header -o %t.a.ast %S/Inputs/RestrictAttr.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, ImportRestrictAttr) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      void *foo(unsigned, unsigned) __attribute__((__malloc__));
+      )",
+      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<RestrictAttr>();
+  auto *ToAttr = ToD->getAttr<RestrictAttr>();
+  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());
+}
+
 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);
     }
   }