[Function] Lock the function when parsing call site info

[vsk]

Summary

DWARF-parsing methods in SymbolFileDWARF which update module state
typically take the module lock. ParseCallEdgesInFunction doesn't do
this, but higher-level locking within lldb::Function (which owns the
storage for parsed call edges) is necessary.

The lack of locking could explain some as-of-yet unreproducible crashes
which occur in Function::GetTailCallingEdges(). In these crashes, the
m_call_edges vector is non-empty but contains a nullptr, which
shouldn't be possible. (If this vector is non-empty, it _must_ contain a
non-null unique_ptr.)

This may address rdar://55622443 and rdar://65119458.

[vsk]

Hm, this doesn't totally fix the race. If the mutex is contested, the Function instance may overwrite its m_call_edges vector.

[vsk]

Move locking up into lldb::Function, and leave a comment in SymbolFileDWARF explaining why.

[aprantl]

Looks plausible.

lldb/include/lldb/Symbol/Function.h

661

nit: When inline comments span multiple lines it's IMO less confusing to convert them to up-front comments.

/// Exclusive lock that controls read/write
/// access to m_call_edges and m_call_edges_resolved.
std::mutex m_call_edges_lock;

I also doubt that the ones in this file are formatted correctly, I think the continuation needs to also use ///<.

lldb/source/Symbol/Function.cpp

293

Can this be called on the same thread and would we benefit from a recursive_mutex?

[vsk]

lldb/include/lldb/Symbol/Function.h

661

Sounds good. I'll reformat all of the data member doxygen comments in this file in a follow up.

lldb/source/Symbol/Function.cpp

293

Function::GetCallEdges() cannot recursively call itself, so a recursive_mutex isn't necessary. Actually if the mutex were taken recursively, that would allow the m_call_edges vector to be clobbered by the n-1 GetCallEdges callers who took the lock before the n-th caller.

lldb/include/lldb/Symbol/Function.h

 #include "lldb/Core/AddressRange.h"
 #include "lldb/Core/Mangled.h"
 #include "lldb/Expression/DWARFExpression.h"
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/Declaration.h"
 #include "lldb/Utility/UserID.h"
 #include "llvm/ADT/ArrayRef.h"
 
+#include <mutex>
+
 namespace lldb_private {
 
 class ExecutionContext;
 
 /// \class FunctionInfo Function.h "lldb/Symbol/Function.h"
 /// A class that contains generic function information.
 ///
 /// This provides generic function information that gets reused between inline
protected:
   AddressRange m_range; ///< The function address range that covers the widest
                         ///range needed to contain all blocks
   DWARFExpression m_frame_base; ///< The frame base expression for variables
                                 ///that are relative to the frame pointer.
   Flags m_flags;
   uint32_t
       m_prologue_byte_size; ///< Compute the prologue size once and cache it
 
+  std::mutex
+      m_call_edges_lock; ///< Exclusive lock that controls read/write
+                         ///  access to m_call_edges and m_call_edges_resolved.
   bool m_call_edges_resolved = false; ///< Whether call site info has been
                                       ///  parsed.
   std::vector<std::unique_ptr<CallEdge>> m_call_edges; ///< Outgoing call edges.
 private:
   Function(const Function &) = delete;
   const Function &operator=(const Function &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SYMBOL_FUNCTION_H

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

for (DWARFDIE child = function_die.GetFirstChild(); child.IsValid();
 
     call_edges.push_back(std::move(edge));
   }
   return call_edges;
 }
 
 std::vector<std::unique_ptr<lldb_private::CallEdge>>
 SymbolFileDWARF::ParseCallEdgesInFunction(UserID func_id) {
+  // ParseCallEdgesInFunction must be called at the behest of an exclusively
+  // locked lldb::Function instance. Storage for parsed call edges is owned by
+  // the lldb::Function instance: locking at the SymbolFile level would be too
+  // late, because the act of storing results from ParseCallEdgesInFunction
+  // would be racy.
   DWARFDIE func_die = GetDIE(func_id.GetID());
   if (func_die.IsValid())
     return CollectCallEdges(GetObjectFile()->GetModule(), func_die);
   return {};
 }
 
 // PluginInterface protocol
 ConstString SymbolFileDWARF::GetPluginName() { return GetPluginNameStatic(); }

lldb/source/Symbol/Function.cpp

void Function::GetEndLineSourceInfo(FileSpec &source_file, uint32_t &line_no) {
   LineEntry line_entry;
   if (line_table->FindLineEntryByAddress(scratch_addr, line_entry, nullptr)) {
     line_no = line_entry.line;
     source_file = line_entry.file;
   }
 }
 
 llvm::ArrayRef<std::unique_ptr<CallEdge>> Function::GetCallEdges() {
+  std::lock_guard<std::mutex> guard(m_call_edges_lock);
+
   if (m_call_edges_resolved)
     return m_call_edges;
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
   LLDB_LOG(log, "GetCallEdges: Attempting to parse call site info for {0}",
            GetDisplayName());
 
   m_call_edges_resolved = true;