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

[via lldb-commits]

Author: Jason Molenda
Date: 2024-03-26T09:07:15-07:00
New Revision: 2f63718f8567413a1c596bda803663eb58d6da5a

URL: https://github.com/llvm/llvm-project/commit/2f63718f8567413a1c596bda803663eb58d6da5a
DIFF: https://github.com/llvm/llvm-project/commit/2f63718f8567413a1c596bda803663eb58d6da5a.diff

LOG: [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.

Added: 
    

Modified: 
    lldb/source/Core/Module.cpp

Removed: 
    


################################################################################
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());