[Lldb-commits] [lldb] 6cfc90b - [Function] Lock the function when parsing call site info

[Vedant Kumar via lldb-commits]

Author: Vedant Kumar
Date: 2020-07-09T10:37:09-07:00
New Revision: 6cfc90b9b791a023368b9384f57c2c3120894247

URL: https://github.com/llvm/llvm-project/commit/6cfc90b9b791a023368b9384f57c2c3120894247
DIFF: https://github.com/llvm/llvm-project/commit/6cfc90b9b791a023368b9384f57c2c3120894247.diff

LOG: [Function] Lock the function when parsing call site info

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.

Reviewers: jasonmolenda, friss, jingham

Subscribers: aprantl, lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D83359

Added: 
    

Modified: 
    lldb/include/lldb/Symbol/Function.h
    lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
    lldb/source/Symbol/Function.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Symbol/Function.h b/lldb/include/lldb/Symbol/Function.h
index e548b8d626a3..bfaa1a13359c 100644
--- a/lldb/include/lldb/Symbol/Function.h
+++ b/lldb/include/lldb/Symbol/Function.h
@@ -17,6 +17,8 @@
 #include "lldb/Utility/UserID.h"
 #include "llvm/ADT/ArrayRef.h"
 
+#include <mutex>
+
 namespace lldb_private {
 
 class ExecutionContext;
@@ -655,6 +657,9 @@ class Function : public UserID, public SymbolContextScope {
   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.

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 7dd084f24e3e..9f64e5255fd5 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3844,6 +3844,11 @@ SymbolFileDWARF::CollectCallEdges(ModuleSP module, DWARFDIE function_die) {
 
 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);

diff  --git a/lldb/source/Symbol/Function.cpp b/lldb/source/Symbol/Function.cpp
index 533fdd465c42..67013f6dd8b1 100644
--- a/lldb/source/Symbol/Function.cpp
+++ b/lldb/source/Symbol/Function.cpp
@@ -290,6 +290,8 @@ void Function::GetEndLineSourceInfo(FileSpec &source_file, uint32_t &line_no) {
 }
 
 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;