Page MenuHomePhabricator

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

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

Details

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
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

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.

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.

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

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.

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
This revision was automatically updated to reflect the committed changes.
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