[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 ↗

(On Diff #276280)

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 ↗

(On Diff #276280)

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

[vsk]

lldb/include/lldb/Symbol/Function.h

661 ↗

(On Diff #276280)

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 ↗

(On Diff #276280)

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/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) {
+  std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   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(); }