This is an archive of the discontinued LLVM Phabricator instance.

Differential D100800

[lldb] Fix demangler leaks in the DWARF AST parser
ClosedPublic

Authored by MaskRay on Apr 19 2021, 2:58 PM.

Details

Reviewers
labath
rupprecht
shafik
teemperor
Commits
rGa2cd6d07691a: [lldb] Fix demangler leaks in the DWARF AST parser
Summary

This fixes 6 check-lldb-shell failures in a -DLLVM_USE_SANITIZER=Leaks build.

Diff Detail

Event Timeline

MaskRay created this revision.Apr 19 2021, 2:58 PM
Herald added a reviewer: shafik. · View Herald TranscriptApr 19 2021, 2:58 PM
MaskRay requested review of this revision.Apr 19 2021, 2:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2021, 2:58 PM
Herald added a subscriber: lldb-commits. · View Herald Transcript
Harbormaster completed remote builds in B99574: Diff 338653.Apr 19 2021, 3:52 PM
Herald added a subscriber: JDevlieghere. · View Herald TranscriptApr 19 2021, 3:52 PM
teemperor added a subscriber: teemperor.Apr 19 2021, 3:54 PM

Thanks for fixing this, I guess we really need a leak sanitizer/valgrind bot for LLDB...

I just have some minor comments but otherwise this LGTM.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
1229

name_buf ? From what I can see this could also be a unique_ptr with a custom deleter so that using name is always safe?

1249

std::free ?

MaskRay updated this revision to Diff 338664.Apr 19 2021, 4:07 PM
MaskRay marked an inline comment as done.

buf -> name_buf

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
1229

LG. Will update to use name_buf

1229

A custom deleter is too inconvenient...

1249

std:: for C library functions is uncommon.

For some common functions (free,strcpy,memset,memcpy,...), the unqualified version is more common. I can find some ::foo as well but std::foo is really rare.

MaskRay added a comment.Apr 19 2021, 4:08 PM
In D100800#2699940, @teemperor wrote:

Thanks for fixing this, I guess we really need a leak sanitizer/valgrind bot for LLDB...

I just have some minor comments but otherwise this LGTM.

Agree.. The 45+ check-lldb failures need to be fixed first..

teemperor accepted this revision.Apr 19 2021, 4:26 PM

LGTM. I still kinda like the unique_ptr deleter but let's not block bot-fixes with refactoring requests. I'll open a review for the unique_ptr as a follow up.

In D100800#2699984, @MaskRay wrote:
In D100800#2699940, @teemperor wrote:

Thanks for fixing this, I guess we really need a leak sanitizer/valgrind bot for LLDB...

I just have some minor comments but otherwise this LGTM.

Agree.. The 45+ check-lldb failures need to be fixed first..

Do you have a list of test failures around? Otherwise I can run the test suite myself when I'm back in the (home) office.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
1249

I believe we're using that more often in newer code (including the demangler), but that was more of nit-pick.

This revision is now accepted and ready to land.Apr 19 2021, 4:26 PM
shafik added inline comments.Apr 19 2021, 4:27 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
1249

In C++ we should be using std::. It seems we are not consistent in when we include the .h files instead of cname versions.

So if we use the cname version e.g. cstdlib then it is unspecified if the functions are defined in the global namespace or not see headers.

JDevlieghere added a comment.EditedApr 19 2021, 4:28 PM
In D100800#2700001, @teemperor wrote:

LGTM. I still kinda like the unique_ptr deleter but let's not block bot-fixes with refactoring requests. I'll open a review for the unique_ptr as a follow up.

In D100800#2699984, @MaskRay wrote:
In D100800#2699940, @teemperor wrote:

Thanks for fixing this, I guess we really need a leak sanitizer/valgrind bot for LLDB...

I just have some minor comments but otherwise this LGTM.

Agree.. The 45+ check-lldb failures need to be fixed first..

Do you have a list of test failures around? Otherwise I can run the test suite myself when I'm back in the (home) office.

We have a sanitized bot that I just revived again. On macOS there's only one more failure remaining.

edit: The bot only runs ASan + UBSan. I saw sanitized but missed the "leaks" part :-)

This revision was landed with ongoing or failed builds.Apr 19 2021, 4:37 PM
Closed by commit rGa2cd6d07691a: [lldb] Fix demangler leaks in the DWARF AST parser (authored by MaskRay). · Explain Why
This revision was automatically updated to reflect the committed changes.
MaskRay added a commit: rGa2cd6d07691a: [lldb] Fix demangler leaks in the DWARF AST parser.
MaskRay added a comment.EditedApr 19 2021, 4:38 PM

The remaining failures are on Linux x86-64 in a -DLLVM_USE_SANITIZER=Leaks build (Address should achieve the same effect because on many targets asan enables LeakSanitizer)

Failed Tests (45):
  lldb-shell :: Reproducer/Functionalities/TestDataFormatter.test
  lldb-shell :: Reproducer/Functionalities/TestImageList.test
  lldb-shell :: Reproducer/Functionalities/TestStepping.test
  lldb-shell :: Reproducer/TestCrash.test
  lldb-shell :: Reproducer/TestDriverOptions.test
  lldb-shell :: Reproducer/TestDump.test
  lldb-shell :: Reproducer/TestGDBRemoteRepro.test
  lldb-shell :: Reproducer/TestHomeDir.test
  lldb-shell :: Reproducer/TestLuaImport.test
  lldb-shell :: Reproducer/TestMultipleTargets.test
  lldb-shell :: Reproducer/TestProcessList.test
  lldb-shell :: Reproducer/TestRelativePath.test
  lldb-shell :: Reproducer/TestReuseDirectory.test
  lldb-shell :: Reproducer/TestVerify.test
  lldb-shell :: Reproducer/TestWorkingDir.test
  lldb-shell :: ScriptInterpreter/Lua/independent_state.test
  lldb-shell :: SymbolFile/NativePDB/ast-functions.cpp
  lldb-shell :: SymbolFile/NativePDB/ast-methods.cpp
  lldb-shell :: SymbolFile/NativePDB/ast-types.cpp
  lldb-shell :: SymbolFile/NativePDB/bitfields.cpp
  lldb-shell :: SymbolFile/NativePDB/break-by-function.cpp
  lldb-shell :: SymbolFile/NativePDB/break-by-line.cpp
  lldb-shell :: SymbolFile/NativePDB/disassembly.cpp
  lldb-shell :: SymbolFile/NativePDB/function-types-classes.cpp
  lldb-shell :: SymbolFile/NativePDB/global-classes.cpp
  lldb-shell :: SymbolFile/NativePDB/load-pdb.cpp
  lldb-shell :: SymbolFile/NativePDB/nested-types.cpp
  lldb-shell :: SymbolFile/NativePDB/s_constant.cpp
  lldb-shell :: SymbolFile/NativePDB/source-list.cpp
  lldb-shell :: SymbolFile/NativePDB/tag-types.cpp
  lldb-unit :: Core/./LLDBCoreTests/RichManglingContextTest.FromCxxMethodName
  lldb-unit :: Utility/./UtilityTests/PassiveReplayTest.InstrumentedBar
  lldb-unit :: Utility/./UtilityTests/PassiveReplayTest.InstrumentedBarPtr
  lldb-unit :: Utility/./UtilityTests/PassiveReplayTest.InstrumentedBarRef
  lldb-unit :: Utility/./UtilityTests/PassiveReplayTest.InstrumentedFoo
  lldb-unit :: Utility/./UtilityTests/PassiveReplayTest.InstrumentedFooInvalid
  lldb-unit :: Utility/./UtilityTests/RecordReplayTest.InstrumentedBar
  lldb-unit :: Utility/./UtilityTests/RecordReplayTest.InstrumentedBarPtr
  lldb-unit :: Utility/./UtilityTests/RecordReplayTest.InstrumentedBarRef
  lldb-unit :: Utility/./UtilityTests/RecordReplayTest.InstrumentedFoo
  lldb-unit :: Utility/./UtilityTests/RecordReplayTest.InstrumentedFooSameThis
  lldb-unit :: Utility/./UtilityTests/SerializationRountripTest.SerializeDeserializeCStringArray
  lldb-unit :: Utility/./UtilityTests/SerializationRountripTest.SerializeDeserializePodPointers
  lldb-unit :: Utility/./UtilityTests/SerializationRountripTest.SerializeDeserializePodReferences
  lldb-unit :: tools/lldb-server/tests/./LLDBServerTests/TestBase.LaunchModePreservesEnvironment

@rupprecht has a patch fixing another demangler usage D100795.

If a maintainer would like to fix Reproducer:

Direct leak of 1 byte(s) in 1 object(s) allocated from:                                                                                                        
    #0 0x567bd8 in operator new(unsigned long) /home/maskray/llvm/compiler-rt/lib/lsan/lsan_interceptors.cpp:238:35                                                                                 
    #1 0x5f7b8d in Read<bool *> /home/maskray/llvm/lldb/include/lldb/Utility/ReproducerInstrumentation.h:408:12                                                                                     
    #2 0x5f7b8d in Deserialize<bool *> /home/maskray/llvm/lldb/include/lldb/Utility/ReproducerInstrumentation.h:327:11                                                                              
    #3 0x5f7b8d in SerializationRountripTest_SerializeDeserializePodPointers_Test::TestBody() /home/maskray/llvm/lldb/unittests/Utility/ReproducerInstrumentationTest.cpp:450:3                     
    #4 0x7787b2 in HandleExceptionsInMethodIfSupported<testing::Test, void> /home/maskray/llvm/llvm/utils/unittest/googletest/src/gtest.cc                                                          
    #5 0x7787b2 in testing::Test::Run() /home/maskray/llvm/llvm/utils/unittest/googletest/src/gtest.cc:2474:5                                                                                       
    #6 0x779cef in testing::TestInfo::Run() /home/maskray/llvm/llvm/utils/unittest/googletest/src/gtest.cc:2656:11                                                                                  
    #7 0x77a550 in testing::TestCase::Run() /home/maskray/llvm/llvm/utils/unittest/googletest/src/gtest.cc:2774:28                                                                                  
    #8 0x783882 in testing::internal::UnitTestImpl::RunAllTests() /home/maskray/llvm/llvm/utils/unittest/googletest/src/gtest.cc:4649:43                                                            
    #9 0x7833fb in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> /home/maskray/llvm/llvm/utils/unittest/googletest/src/gtest.cc                                        
    #10 0x7833fb in testing::UnitTest::Run() /home/maskray/llvm/llvm/utils/unittest/googletest/src/gtest.cc:4257:10                                                                                 
    #11 0x766acb in RUN_ALL_TESTS /home/maskray/llvm/llvm/utils/unittest/googletest/include/gtest/gtest.h:2233:46                                                                                   
    #12 0x766acb in main /home/maskray/llvm/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:10                                                                                                     
    #13 0x7ffff7a51d09 in __libc_start_main csu/../csu/libc-start.c:308:16

(otherwise I can figure out it by myself..)

NativePDB:

Direct leak of 6144 byte(s) in 6 object(s) allocated from:
    #0 0x27e108 in malloc /home/maskray/llvm/compiler-rt/lib/lsan/lsan_interceptors.cpp:56:3
    #1 0x7ffff763dfb8 in initializeOutputStream /home/maskray/llvm/llvm/include/llvm/Demangle/Utility.h:178:31
    #2 0x7ffff763dfb8 in llvm::ms_demangle::Node::toString[abi:cxx11](llvm::ms_demangle::OutputFlags) const /home/maskray/llvm/llvm/lib/Demangle/MicrosoftDemangleNodes.cpp:120:3
    #3 0x7ffff585da19 in lldb_private::npdb::PdbAstBuilder::CreateDeclInfoForType[abi:cxx11](llvm::codeview::TagRecord const&, llvm::codeview::TypeIndex) /home/maskray/llvm/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:228:28
    #4 0x7ffff5862369 in lldb_private::npdb::PdbAstBuilder::CreateEnumType(lldb_private::npdb::PdbTypeSymId, llvm::codeview::EnumRecord const&) /home/maskray/llvm/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1103:35
    #5 0x7ffff5862235 in lldb_private::npdb::PdbAstBuilder::CreateType(lldb_private::npdb::PdbTypeSymId) /home/maskray/llvm/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:924:14
    #6 0x7ffff585e4b8 in lldb_private::npdb::PdbAstBuilder::GetOrCreateType(lldb_private::npdb::PdbTypeSymId) /home/maskray/llvm/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:969:8
    #7 0x7ffff5861700 in lldb_private::npdb::PdbAstBuilder::CreateModifierType(llvm::codeview::ModifierRecord const&) /home/maskray/llvm/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:754:37
    #8 0x7ffff586219f in lldb_private::npdb::PdbAstBuilder::CreateType(lldb_private::npdb::PdbTypeSymId) /home/maskray/llvm/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:909:12
    #9 0x7ffff585e4b8 in lldb_private::npdb::PdbAstBuilder::GetOrCreateType(lldb_private::npdb::PdbTypeSymId) /home/maskray/llvm/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:969:8
    #10 0x7ffff58409c7 in lldb_private::npdb::SymbolFileNativePDB::CreateAndCacheType(lldb_private::npdb::PdbTypeSymId) /home/maskray/llvm/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:694:31
    #11 0x7ffff584756e in lldb_private::npdb::SymbolFileNativePDB::ResolveTypeUID(unsigned long) /home/maskray/llvm/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:1532:20
    #12 0x7ffff41213fa in lldb_private::SymbolFileType::GetType() /home/maskray/llvm/lldb/source/Symbol/Type.cpp:135:41
    #13 0x7ffff4069254 in lldb_private::ValueObjectVariable::GetCompilerTypeImpl() /home/maskray/llvm/lldb/source/Core/ValueObjectVariable.cpp:69:35
    #14 0x7ffff405134f in lldb_private::ValueObject::MaybeCalculateCompleteType() /home/maskray/llvm/lldb/source/Core/ValueObject.cpp:247:30
    #15 0x7ffff406f511 in GetCompilerType /home/maskray/llvm/lldb/include/lldb/Core/ValueObject.h:352:43
    #16 0x7ffff406f511 in lldb_private::FormatManager::GetTypeForCache(lldb_private::ValueObject&, lldb::DynamicValueType) /home/maskray/llvm/lldb/source/DataFormatters/FormatManager.cpp:560:31
    #17 0x7ffff4072ee3 in lldb_private::FormattersMatchData::FormattersMatchData(lldb_private::ValueObject&, lldb::DynamicValueType) /home/maskray/llvm/lldb/source/DataFormatters/FormatClasses.cpp:25:22
    #18 0x7ffff406f78a in std::shared_ptr<lldb_private::TypeFormatImpl> lldb_private::FormatManager::Get<std::shared_ptr<lldb_private::TypeFormatImpl> >(lldb_private::ValueObject&, lldb::DynamicValueType) /home/maskray/llvm/lldb/source/DataFormatters/FormatManager.cpp:613:23
    #19 0x7ffff406f74d in lldb_private::FormatManager::GetFormat(lldb_private::ValueObject&, lldb::DynamicValueType) /home/maskray/llvm/lldb/source/DataFormatters/FormatManager.cpp:672:10
    #20 0x7ffff406a3d2 in lldb_private::DataVisualization::GetFormat(lldb_private::ValueObject&, lldb::DynamicValueType) /home/maskray/llvm/lldb/source/DataFormatters/DataVisualization.cpp:33:29
    #21 0x7ffff4050c8b in lldb_private::ValueObject::UpdateFormatsIfNeeded() /home/maskray/llvm/lldb/source/Core/ValueObject.cpp:217:20
    #22 0x7ffff4050791 in lldb_private::ValueObject::UpdateValueIfNeeded(bool) /home/maskray/llvm/lldb/source/Core/ValueObject.cpp:116:26
    #23 0x7ffff4084ca8 in lldb_private::ValueObjectPrinter::GetMostSpecializedValue() /home/maskray/llvm/lldb/source/DataFormatters/ValueObjectPrinter.cpp:101:40
    #24 0x7ffff4084ba1 in lldb_private::ValueObjectPrinter::PrintValueObject() /home/maskray/llvm/lldb/source/DataFormatters/ValueObjectPrinter.cpp:74:8
    #25 0x7ffff405958b in lldb_private::ValueObject::Dump(lldb_private::Stream&, lldb_private::DumpValueObjectOptions const&) /home/maskray/llvm/lldb/source/Core/ValueObject.cpp:2563:11
    #26 0x7ffff57946c0 in CommandObjectTargetVariable::DumpValueObject(lldb_private::Stream&, std::shared_ptr<lldb_private::Variable>&, std::shared_ptr<lldb_private::ValueObject>&, char const*) /home/maskray/llvm/lldb/source/Commands/CommandObjectTarget.cpp:794:16
    #27 0x7ffff5793492 in CommandObjectTargetVariable::DoExecute(lldb_private::Args&, lldb_private::CommandReturnObject&) /home/maskray/llvm/lldb/source/Commands/CommandObjectTarget.cpp:886:17
    #28 0x7ffff40d50e6 in lldb_private::CommandObjectParsed::Execute(char const*, lldb_private::CommandReturnObject&) /home/maskray/llvm/lldb/source/Interpreter/CommandObject.cpp:999:19
    #29 0x7ffff40c7c1b in lldb_private::CommandInterpreter::HandleCommand(char const*, lldb_private::LazyBool, lldb_private::CommandReturnObject&) /home/maskray/llvm/lldb/source/Interpreter/CommandInterpreter.cpp:1800:14
    #30 0x7ffff40cb517 in lldb_private::CommandInterpreter::IOHandlerInputComplete(lldb_private::IOHandler&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /home/maskray/llvm/lldb/source/Interpreter/CommandInterpreter.cpp:2848:3
    #31 0x7ffff402079f in lldb_private::IOHandlerEditline::Run() /home/maskray/llvm/lldb/source/Core/IOHandler.cpp:579:22
Harbormaster completed remote builds in B99583: Diff 338664.Apr 19 2021, 4:52 PM
rupprecht mentioned this in D100795: [lldb] Fix RichManglingContext::FromCxxMethodName() leak.Apr 21 2021, 9:05 AM
dblaikie mentioned this in D101444: [demangler] Initial support for the new Rust mangling scheme.Apr 29 2021, 2:24 PM

Revision Contents

PathSize
lldb/
source/
Plugins/
SymbolFile/
DWARF/
8 lines

Diff 338664

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Show First 20 Lines • Show All 1,220 Lines • ▼ Show 20 Lines if (!type_handled) {
if (function_decl) { if (function_decl) {
LinkDeclContextToDIE(function_decl, die); LinkDeclContextToDIE(function_decl, die);
} }
} }
} }
if (!function_decl) { if (!function_decl) {
char *name_buf = nullptr;
teemperorUnsubmitted
Done
ReplyInline Actions

name_buf ? From what I can see this could also be a unique_ptr with a custom deleter so that using name is always safe?

teemperor: `name_buf` ? From what I can see this could also be a unique_ptr with a custom deleter so that…
MaskRayAuthorUnsubmitted
Done
ReplyInline Actions

LG. Will update to use name_buf

MaskRay: LG. Will update to use `name_buf`
MaskRayAuthorUnsubmitted
Done
ReplyInline Actions

A custom deleter is too inconvenient...

MaskRay: A custom deleter is too inconvenient...
llvm::StringRef name = attrs.name.GetStringRef(); llvm::StringRef name = attrs.name.GetStringRef();
// We currently generate function templates with template parameters in // We currently generate function templates with template parameters in
// their name. In order to get closer to the AST that clang generates // their name. In order to get closer to the AST that clang generates
// we want to strip these from the name when creating the AST. // we want to strip these from the name when creating the AST.
if (attrs.mangled_name) { if (attrs.mangled_name) {
llvm::ItaniumPartialDemangler D; llvm::ItaniumPartialDemangler D;
if (!D.partialDemangle(attrs.mangled_name)) if (!D.partialDemangle(attrs.mangled_name)) {
name = D.getFunctionBaseName(nullptr, nullptr); name_buf = D.getFunctionBaseName(nullptr, nullptr);
name = name_buf;
}
} }
// We just have a function that isn't part of a class // We just have a function that isn't part of a class
function_decl = m_ast.CreateFunctionDeclaration( function_decl = m_ast.CreateFunctionDeclaration(
ignore_containing_context ? m_ast.GetTranslationUnitDecl() ignore_containing_context ? m_ast.GetTranslationUnitDecl()
: containing_decl_ctx, : containing_decl_ctx,
GetOwningClangModule(die), name, clang_type, attrs.storage, GetOwningClangModule(die), name, clang_type, attrs.storage,
attrs.is_inline); attrs.is_inline);
std::free(name_buf);
teemperorUnsubmitted
Not Done
ReplyInline Actions

std::free ?

teemperor: `std::free` ?
MaskRayAuthorUnsubmitted
Done
ReplyInline Actions

std:: for C library functions is uncommon.

For some common functions (free,strcpy,memset,memcpy,...), the unqualified version is more common. I can find some ::foo as well but std::foo is really rare.

MaskRay: `std::` for C library functions is uncommon. For some common functions (free,strcpy,memset…
shafikUnsubmitted
Not Done
ReplyInline Actions

In C++ we should be using std::. It seems we are not consistent in when we include the .h files instead of cname versions.

So if we use the cname version e.g. cstdlib then it is unspecified if the functions are defined in the global namespace or not see headers.

shafik: In C++ we should be using `std::`. It seems we are not consistent in when we include the `.h`…
teemperorUnsubmitted
Not Done
ReplyInline Actions

I believe we're using that more often in newer code (including the demangler), but that was more of nit-pick.

teemperor: I believe we're using that more often in newer code (including the demangler), but that was…
if (has_template_params) { if (has_template_params) {
TypeSystemClang::TemplateParameterInfos template_param_infos; TypeSystemClang::TemplateParameterInfos template_param_infos;
ParseTemplateParameterInfos(die, template_param_infos); ParseTemplateParameterInfos(die, template_param_infos);
template_function_decl = m_ast.CreateFunctionDeclaration( template_function_decl = m_ast.CreateFunctionDeclaration(
ignore_containing_context ? m_ast.GetTranslationUnitDecl() ignore_containing_context ? m_ast.GetTranslationUnitDecl()
: containing_decl_ctx, : containing_decl_ctx,
GetOwningClangModule(die), attrs.name.GetStringRef(), clang_type, GetOwningClangModule(die), attrs.name.GetStringRef(), clang_type,
▲ Show 20 Lines • Show All 2,519 Lines • Show Last 20 Lines