martong created this revision. martong added reviewers: teemperor, shafik. Herald added subscribers: cfe-commits, pengfei, gamesh411, Szelethus, arphaman, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. Herald added a project: clang. martong requested review of this revision.
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== Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D89318 Files: clang/lib/AST/ASTImporter.cpp clang/test/ASTMerge/attr/Inputs/RestrictAttr.cpp clang/test/ASTMerge/attr/testRestrictAttr.cpp clang/unittests/AST/ASTImporterTest.cpp Index: clang/unittests/AST/ASTImporterTest.cpp =================================================================== --- clang/unittests/AST/ASTImporterTest.cpp +++ clang/unittests/AST/ASTImporterTest.cpp @@ -5767,6 +5767,31 @@ 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()); +} + template <typename T> auto ExtendWithOptions(const T &Values, const std::vector<std::string> &Args) { auto Copy = Values; Index: clang/test/ASTMerge/attr/testRestrictAttr.cpp =================================================================== --- /dev/null +++ clang/test/ASTMerge/attr/testRestrictAttr.cpp @@ -0,0 +1,2 @@ +// 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 Index: clang/test/ASTMerge/attr/Inputs/RestrictAttr.cpp =================================================================== --- /dev/null +++ clang/test/ASTMerge/attr/Inputs/RestrictAttr.cpp @@ -0,0 +1,2 @@ +void *foo(unsigned, unsigned) +__attribute__((__malloc__)); Index: clang/lib/AST/ASTImporter.cpp =================================================================== --- clang/lib/AST/ASTImporter.cpp +++ clang/lib/AST/ASTImporter.cpp @@ -8094,8 +8094,6 @@ return ToTOrErr.takeError(); } To->setInherited(From->isInherited()); - To->setPackExpansion(From->isPackExpansion()); - To->setImplicit(From->isImplicit()); ToAttr = To; break; } @@ -8106,8 +8104,11 @@ ToAttr->setRange(ToRange); break; } + ToAttr->setAttributeSpellingListIndex( + FromAttr->getAttributeSpellingListIndex()); + ToAttr->setPackExpansion(FromAttr->isPackExpansion()); + ToAttr->setImplicit(FromAttr->isImplicit()); assert(ToAttr && "Attribute should be created."); - return ToAttr; }
Index: clang/unittests/AST/ASTImporterTest.cpp =================================================================== --- clang/unittests/AST/ASTImporterTest.cpp +++ clang/unittests/AST/ASTImporterTest.cpp @@ -5767,6 +5767,31 @@ 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()); +} + template <typename T> auto ExtendWithOptions(const T &Values, const std::vector<std::string> &Args) { auto Copy = Values; Index: clang/test/ASTMerge/attr/testRestrictAttr.cpp =================================================================== --- /dev/null +++ clang/test/ASTMerge/attr/testRestrictAttr.cpp @@ -0,0 +1,2 @@ +// 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 Index: clang/test/ASTMerge/attr/Inputs/RestrictAttr.cpp =================================================================== --- /dev/null +++ clang/test/ASTMerge/attr/Inputs/RestrictAttr.cpp @@ -0,0 +1,2 @@ +void *foo(unsigned, unsigned) +__attribute__((__malloc__)); Index: clang/lib/AST/ASTImporter.cpp =================================================================== --- clang/lib/AST/ASTImporter.cpp +++ clang/lib/AST/ASTImporter.cpp @@ -8094,8 +8094,6 @@ return ToTOrErr.takeError(); } To->setInherited(From->isInherited()); - To->setPackExpansion(From->isPackExpansion()); - To->setImplicit(From->isImplicit()); ToAttr = To; break; } @@ -8106,8 +8104,11 @@ ToAttr->setRange(ToRange); break; } + ToAttr->setAttributeSpellingListIndex( + FromAttr->getAttributeSpellingListIndex()); + ToAttr->setPackExpansion(FromAttr->isPackExpansion()); + ToAttr->setImplicit(FromAttr->isImplicit()); assert(ToAttr && "Attribute should be created."); - return ToAttr; }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits