From 870ef20342d0211d3597425ab862a3acc36302c3 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Tue, 26 Mar 2024 09:07:15 -0700 Subject: [PATCH 1/2] [lldb] Don't clear a Module's UnwindTable when adding a SymbolFile (#86603) Fixing a crash in lldb when `symbols.auto-download` setting is enabled. When doing a backtrace, this feature has lldb search for a SymbolFile for stack frames when we are backtracing, and add them either synchoronously or asynchronously, depending on the specific setting used. Module::SetSymbolFileFileSpec clears the Module's UnwindTable, once we find a new SymbolFile. We may be adding a source of unwind information that we did not have when lldb was working only with the executable binary. What happens in practice is that we're using a reference to the Module's UnwindTable, and then the other thread getting the SymbolFile clears it and now the first thread is referring to freed memory and we can crash. When built with address sanitizer, it crashes much more reliably. Given that unwind information used for exception handling -- eh_frame, compact unwind -- is present in executable binaries, the only thing we're likely to *add* would be DWARF's `debug_frame` if that was also available. The actual value of re-creating the UnwindTable when we have added a SymbolFile is not large. I also tried fixing this by changing the Module to have a shared_ptr to the UnwindTable, so we could have two different UnwindTable's in use simultaneously for a brief period. This would be fine TODAY, but it introduces a very subtle bug that someone will have a heck of a time figuring out in the future. In the end, I believe the safest approach is to sacrifice the possible marginal gain of reconstructing the UnwindTable once a SymbolFile has been added, to sidestep this whole problem area. Also, in `Module::GetUnwindTable()`, call `DownloadSymbolFileAsync` before we create the UnwindTable for the first time, in case the symbol file is fetched synchronously, we will have it for that possible marginal gain. (cherry picked from commit 2f63718f8567413a1c596bda803663eb58d6da5a) --- lldb/source/Core/Module.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp index 3fe4cca461ba6..30b6b45d3f6b9 100644 --- a/lldb/source/Core/Module.cpp +++ b/lldb/source/Core/Module.cpp @@ -1371,9 +1371,9 @@ void Module::SectionFileAddressesChanged() { UnwindTable &Module::GetUnwindTable() { if (!m_unwind_table) { - m_unwind_table.emplace(*this); if (!m_symfile_spec) SymbolLocator::DownloadSymbolFileAsync(GetUUID()); + m_unwind_table.emplace(*this); } return *m_unwind_table; } @@ -1491,15 +1491,10 @@ void Module::SetSymbolFileFileSpec(const FileSpec &file) { // one obj_file->ClearSymtab(); - // Clear the unwind table too, as that may also be affected by the - // symbol file information. - m_unwind_table.reset(); - // The symbol file might be a directory bundle ("/tmp/a.out.dSYM") // instead of a full path to the symbol file within the bundle // ("/tmp/a.out.dSYM/Contents/Resources/DWARF/a.out"). So we need to // check this - if (FileSystem::Instance().IsDirectory(file)) { std::string new_path(file.GetPath()); std::string old_path(obj_file->GetFileSpec().GetPath()); From ed7d095e05f926a56f50bfda15d60163d0853ed2 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Tue, 26 Mar 2024 10:58:02 -0700 Subject: [PATCH 2/2] Remove shell test that was testing a behavior I removed. --- .../SymbolFile/target-symbols-add-unwind.test | 27 ------------------- 1 file changed, 27 deletions(-) delete mode 100644 lldb/test/Shell/SymbolFile/target-symbols-add-unwind.test diff --git a/lldb/test/Shell/SymbolFile/target-symbols-add-unwind.test b/lldb/test/Shell/SymbolFile/target-symbols-add-unwind.test deleted file mode 100644 index 5420213d405e8..0000000000000 --- a/lldb/test/Shell/SymbolFile/target-symbols-add-unwind.test +++ /dev/null @@ -1,27 +0,0 @@ -# TODO: When it's possible to run "image show-unwind" without a running -# process, we can remove the unsupported line below, and hard-code an ELF -# triple in the test. -# UNSUPPORTED: system-windows, system-darwin - -# RUN: cd %T -# RUN: %clang_host %S/Inputs/target-symbols-add-unwind.c -g \ -# RUN: -fno-unwind-tables -fno-asynchronous-unwind-tables \ -# RUN: -o target-symbols-add-unwind.debug -# RUN: llvm-objcopy --strip-debug target-symbols-add-unwind.debug \ -# RUN: target-symbols-add-unwind.stripped -# RUN: %lldb target-symbols-add-unwind.stripped -s %s -o quit | FileCheck %s - -process launch --stop-at-entry -image show-unwind -n main -# CHECK-LABEL: image show-unwind -n main -# CHECK-NOT: debug_frame UnwindPlan: - -target symbols add -s target-symbols-add-unwind.stripped target-symbols-add-unwind.debug -# CHECK-LABEL: target symbols add -# CHECK: symbol file {{.*}} has been added to {{.*}} - -image show-unwind -n main -# CHECK-LABEL: image show-unwind -n main -# CHECK: debug_frame UnwindPlan: -# CHECK-NEXT: This UnwindPlan originally sourced from DWARF CFI -# CHECK-NEXT: This UnwindPlan is sourced from the compiler: yes.