Reinitialize UnwindTable when the SymbolFile changes

[labath]

Summary

This is a preparatory step to enable adding of unwind plans by symbol
file plugins.

Although at the surface it seems that currently symbol files have
nothing to do with unwinding, this isn't entirely correct even now. The
mere act of adding a symbol file can have the effect of making more
sections (typically .debug_frame) available to the unwinding machinery,
so that it can have more unwind strategies to choose from.

Up until now, we've had a bug, which went largely unnoticed, where
unwind info in the manually added symbols files (target symbols add) was
being ignored during unwinding. Reinitializing the UnwindTable fixes
that bug too.

[clayborg]

source/Core/Module.cpp

1451–1454

Are we sure no one is holding onto info that was given out from an existing UnwindTable? What if we do a backtrace, then load symbol files and then do another backtrace? Seems like we will want to keep our existing info and just call m_unwind_table->Clear()?

[jasonmolenda]

source/Core/Module.cpp

1451–1454

I THINK it will be OK. RegisterContextLLDB holds shared pointers to the UnwindPlans that it got from the UnwindTable, so those will stay alive. An UnwindPlan doesn't refer back to the UnwindTable it came from.

[labath]

source/Core/Module.cpp

1451–1454

I agree with Jason. Nobody should store any pointers to the unwind table, and the FuncUnwinders and UnwindPlan objects are already handed out as shared pointers (but those shouldn't persist for long either, or otherwise our new unwind info wouldn't be effective). If we find a good reason to store the old unwind table, we can always do the thing like we do for SymbolFiles (where the Module keeps a list of all SymbolFile objects it ever had), but I hope that will not be necessary.

[labath]

Greg, what do you make of this patch? Do you still want me to clear the unwind table instead of resetting it?

[clayborg]

In D58347#1424745, @labath wrote:

Greg, what do you make of this patch? Do you still want me to clear the unwind table instead of resetting it?

No, Jason seems ok with it so I am good.

[clayborg]

Since unwinding is Jason's thing, he should give the ok for this patch.

[jasonmolenda]

Yeah, I'm ok with this, thanks.

include/lldb/Core/Module.h

public:
   /// function has yet to be unwound in a stack walk), it will be created when
   /// requested.  Specifically, we do not create FuncUnwinders objects for
   /// functions until they are needed.
   ///
   /// @return
   ///     Returns the unwind table for this module. If this object has no
   ///     associated object file, an empty UnwindTable is returned.
   //------------------------------------------------------------------
-  UnwindTable &GetUnwindTable() { return m_unwind_table; }
+  UnwindTable &GetUnwindTable();
 
   llvm::VersionTuple GetVersion();
 
   //------------------------------------------------------------------
   /// Load an object file from memory.
   ///
   /// If available, the size of the object file in memory may be passed to
   /// avoid additional round trips to process memory. If the size is not
protected:
   ConstString m_object_name; ///< The name an object within this module that is
                              ///selected, or empty of the module is represented
                              ///by \a m_file.
   uint64_t m_object_offset;
   llvm::sys::TimePoint<> m_object_mod_time;
   lldb::ObjectFileSP m_objfile_sp; ///< A shared pointer to the object file
                                    ///parser for this module as it may or may
                                    ///not be shared with the SymbolFile
-  UnwindTable m_unwind_table{*this}; ///< Table of FuncUnwinders objects created
-                                     /// for this Module's functions
+  llvm::Optional<UnwindTable> m_unwind_table; ///< Table of FuncUnwinders
+                                              /// objects created for this
+                                              /// Module's functions
   lldb::SymbolVendorUP
       m_symfile_up; ///< A pointer to the symbol vendor for this module.
   std::vector<lldb::SymbolVendorUP>
       m_old_symfiles; ///< If anyone calls Module::SetSymbolFileFileSpec() and
                       ///changes the symbol file,
   ///< we need to keep all old symbol files around in case anyone has type
   ///references to them
   TypeSystemMap m_type_system_map;   ///< A map of any type systems associated

lit/SymbolFile/Inputs/target-symbols-add-unwind.c

+void _start() {}

lit/SymbolFile/target-symbols-add-unwind.test

+# 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 %S/Inputs/target-symbols-add-unwind.c -nostdlib -g \
+# RUN:   -fno-unwind-tables -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 _start
+# CHECK-LABEL: image show-unwind -n _start
+# 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 _start
+# CHECK-LABEL: image show-unwind -n _start
+# CHECK: debug_frame UnwindPlan:
+# CHECK-NEXT: This UnwindPlan originally sourced from DWARF CFI
+# CHECK-NEXT: This UnwindPlan is sourced from the compiler: yes.

source/Core/Module.cpp

void Module::SectionFileAddressesChanged() {
   ObjectFile *obj_file = GetObjectFile();
   if (obj_file)
     obj_file->SectionFileAddressesChanged();
   SymbolVendor *sym_vendor = GetSymbolVendor();
   if (sym_vendor != nullptr)
     sym_vendor->SectionFileAddressesChanged();
 }
 
+UnwindTable &Module::GetUnwindTable() {
+  if (!m_unwind_table)
+    m_unwind_table.emplace(*this);
+  return *m_unwind_table;
+}
+
 SectionList *Module::GetUnifiedSectionList() {
   if (!m_sections_up)
     m_sections_up = llvm::make_unique<SectionList>();
   return m_sections_up.get();
 }
 
 const Symbol *Module::FindFirstSymbolWithNameAndType(const ConstString &name,
                                                      SymbolType symbol_type) {
if (section_list && symbol_file) {
           // we don't have to do anything.
           return;
         }
 
         // Cleare the current symtab as we are going to replace it with a new
         // 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());

source/Symbol/UnwindTable.cpp

void UnwindTable::Initialize() {
 
   if (m_initialized) // check again once we've acquired the lock
     return;
   m_initialized = true;
   ObjectFile *object_file = m_module.GetObjectFile();
   if (!object_file)
     return;
 
-  SectionList *sl = object_file->GetSectionList();
+  SectionList *sl = m_module.GetSectionList();
   if (!sl)
     return;
 
   SectionSP sect = sl->FindSectionByType(eSectionTypeEHFrame, true);
   if (sect.get()) {
     m_eh_frame_up.reset(
         new DWARFCallFrameInfo(*object_file, sect, DWARFCallFrameInfo::EH));
   }