[Lldb-commits] [lldb] [lldb] Don't clear a Module's UnwindTable when adding a SymbolFile (PR #86603)

[Jason Molenda via lldb-commits]

https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/86603

>From b6fcac7d6bb48b7fb665034712407c9ad7e4053a Mon Sep 17 00:00:00 2001
From: Jason Molenda <jason at molenda.com>
Date: Mon, 25 Mar 2024 16:47:11 -0700
Subject: [PATCH] [lldb] Don't clear a Module's UnwindTable when adding a
 SymbolFile

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.
---
 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 8ffa35518b3c36..a520523a96521a 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -1239,9 +1239,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;
 }
@@ -1359,15 +1359,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());