Use a RAII guard to control access to DisassemblerLLVMC.

[teemperor]

Summary

This patch replaces the manual lock/unlock calls for gaining exclusive access to the disassembler with
a RAII-powered access scope. This should prevent that we somehow skip over these trailing Unlock calls
(e.g. with early returns).

We also have a second GetDisasmToUse method now that takes an already constructed access scope to
prevent deadlocks when we call this from other methods.

[vsk]

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h

81 ↗

(On Diff #162736)

This is nice. Do you think it might be even safer to have the guard own the disassembler shared_ptr instance? Users could then access the disassembler via the guard's operator->, and there's less chance of the shared_ptr escaping / being abused. We could even have GetDisassembler() return a guard instance.

85 ↗

(On Diff #162736)

It doesn't make sense to me that DisassemblerLLVMC's "Lock" method conflates locking and initialization. If you decide to have a guard own the disassembler instance, it'd make sense to split the initialization step out.

[teemperor]

• Got rid of Lock/Unlock.
• Replaced guard with a mandatory scope guard that grants exclusive access to the debugger.
• Added a second GetDisasmToUse that reuses an existing guard object to fix the resulting deadlocks.

[davide]

LGTM if Vedant is happy with this.

[vsk]

Looks great, thanks!

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

176 ↗

(On Diff #162775)

Were all callers into Decode locking the disasm instance prior to this patch? If not, this is a sweet fix.

[teemperor]

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

176 ↗

(On Diff #162775)

Not from what I can see. Otherwise I would have seen deadlocks when adding this lock here.

lldb/trunk/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h

static int OpInfoCallback(void *DisInfo, uint64_t PC, uint64_t Offset,
                             uint64_t Size, int TagType, void *TagBug);
 
   static const char *SymbolLookupCallback(void *DisInfo,
                                           uint64_t ReferenceValue,
                                           uint64_t *ReferenceType,
                                           uint64_t ReferencePC,
                                           const char **ReferenceName);
 
-  void Lock(InstructionLLVMC *inst,
-            const lldb_private::ExecutionContext *exe_ctx) {
-    m_mutex.lock();
-    m_inst = inst;
-    m_exe_ctx = exe_ctx;
-  }
-
-  void Unlock() {
-    m_inst = NULL;
-    m_exe_ctx = NULL;
-    m_mutex.unlock();
-  }
-
   const lldb_private::ExecutionContext *m_exe_ctx;
   InstructionLLVMC *m_inst;
   std::mutex m_mutex;
   bool m_data_from_file;
 
   // Since we need to make two actual MC Disassemblers for ARM (ARM & THUMB),
   // and there's a bit of goo to set up and own in the MC disassembler world,
   // this class was added to manage the actual disassemblers.
   class MCDisasmInstance;
   std::unique_ptr<MCDisasmInstance> m_disasm_up;
   std::unique_ptr<MCDisasmInstance> m_alternate_disasm_up;
 };
 
 #endif // liblldb_DisassemblerLLVM_h_

lldb/trunk/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

InstructionLLVMC(DisassemblerLLVMC &disasm,
         m_does_branch(eLazyBoolCalculate), m_has_delay_slot(eLazyBoolCalculate),
         m_is_call(eLazyBoolCalculate), m_is_valid(false),
         m_using_file_addr(false) {}
 
   ~InstructionLLVMC() override = default;
 
   bool DoesBranch() override {
     if (m_does_branch == eLazyBoolCalculate) {
-      std::shared_ptr<DisassemblerLLVMC> disasm_sp(GetDisassembler());
-      if (disasm_sp) {
-        disasm_sp->Lock(this, NULL);
+      DisassemblerScope disasm(*this);
+      if (disasm) {
         DataExtractor data;
         if (m_opcode.GetData(data)) {
           bool is_alternate_isa;
           lldb::addr_t pc = m_address.GetFileAddress();
 
           DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
-              GetDisasmToUse(is_alternate_isa);
+              GetDisasmToUse(is_alternate_isa, disasm);
           const uint8_t *opcode_data = data.GetDataStart();
           const size_t opcode_data_len = data.GetByteSize();
           llvm::MCInst inst;
           const size_t inst_size =
               mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
           // Be conservative, if we didn't understand the instruction, say it
           // might branch...
           if (inst_size == 0)
             m_does_branch = eLazyBoolYes;
           else {
             const bool can_branch = mc_disasm_ptr->CanBranch(inst);
             if (can_branch)
               m_does_branch = eLazyBoolYes;
             else
               m_does_branch = eLazyBoolNo;
           }
         }
-        disasm_sp->Unlock();
       }
     }
     return m_does_branch == eLazyBoolYes;
   }
 
   bool HasDelaySlot() override {
     if (m_has_delay_slot == eLazyBoolCalculate) {
-      std::shared_ptr<DisassemblerLLVMC> disasm_sp(GetDisassembler());
-      if (disasm_sp) {
-        disasm_sp->Lock(this, NULL);
+      DisassemblerScope disasm(*this);
+      if (disasm) {
         DataExtractor data;
         if (m_opcode.GetData(data)) {
           bool is_alternate_isa;
           lldb::addr_t pc = m_address.GetFileAddress();
 
           DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
-              GetDisasmToUse(is_alternate_isa);
+              GetDisasmToUse(is_alternate_isa, disasm);
           const uint8_t *opcode_data = data.GetDataStart();
           const size_t opcode_data_len = data.GetByteSize();
           llvm::MCInst inst;
           const size_t inst_size =
               mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
           // if we didn't understand the instruction, say it doesn't have a
           // delay slot...
           if (inst_size == 0)
             m_has_delay_slot = eLazyBoolNo;
           else {
             const bool has_delay_slot = mc_disasm_ptr->HasDelaySlot(inst);
             if (has_delay_slot)
               m_has_delay_slot = eLazyBoolYes;
             else
               m_has_delay_slot = eLazyBoolNo;
           }
         }
-        disasm_sp->Unlock();
       }
     }
     return m_has_delay_slot == eLazyBoolYes;
   }
 
   DisassemblerLLVMC::MCDisasmInstance *GetDisasmToUse(bool &is_alternate_isa) {
-    is_alternate_isa = false;
-    std::shared_ptr<DisassemblerLLVMC> disasm_sp(GetDisassembler());
-    if (disasm_sp) {
-      if (disasm_sp->m_alternate_disasm_up) {
-        const AddressClass address_class = GetAddressClass();
-
-        if (address_class == AddressClass::eCodeAlternateISA) {
-          is_alternate_isa = true;
-          return disasm_sp->m_alternate_disasm_up.get();
-        }
-      }
-      return disasm_sp->m_disasm_up.get();
-    }
-    return nullptr;
+    DisassemblerScope disasm(*this);
+    return GetDisasmToUse(is_alternate_isa, disasm);
   }
 
   size_t Decode(const lldb_private::Disassembler &disassembler,
                 const lldb_private::DataExtractor &data,
                 lldb::offset_t data_offset) override {
     // All we have to do is read the opcode which can be easy for some
     // architectures
     bool got_op = false;
-    std::shared_ptr<DisassemblerLLVMC> disasm_sp(GetDisassembler());
-    if (disasm_sp) {
-      const ArchSpec &arch = disasm_sp->GetArchitecture();
+    DisassemblerScope disasm(*this);
+    if (disasm) {
+      const ArchSpec &arch = disasm->GetArchitecture();
       const lldb::ByteOrder byte_order = data.GetByteOrder();
 
       const uint32_t min_op_byte_size = arch.GetMinimumOpcodeByteSize();
       const uint32_t max_op_byte_size = arch.GetMaximumOpcodeByteSize();
       if (min_op_byte_size == max_op_byte_size) {
         // Fixed size instructions, just read that amount of data.
         if (!data.ValidOffsetForDataOfSize(data_offset, min_op_byte_size))
           return false;
if (disasm) {
                                   min_op_byte_size);
           got_op = true;
           break;
         }
       }
       if (!got_op) {
         bool is_alternate_isa = false;
         DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
-            GetDisasmToUse(is_alternate_isa);
+            GetDisasmToUse(is_alternate_isa, disasm);
 
         const llvm::Triple::ArchType machine = arch.GetMachine();
         if (machine == llvm::Triple::arm || machine == llvm::Triple::thumb) {
           if (machine == llvm::Triple::thumb || is_alternate_isa) {
             uint32_t thumb_opcode = data.GetU16(&data_offset);
             if ((thumb_opcode & 0xe000) != 0xe000 ||
                 ((thumb_opcode & 0x1800u) == 0)) {
               m_opcode.SetOpcode16(thumb_opcode, byte_order);
if (disasm) {
           // The opcode isn't evenly sized, so we need to actually use the llvm
           // disassembler to parse it and get the size.
           uint8_t *opcode_data =
               const_cast<uint8_t *>(data.PeekData(data_offset, 1));
           const size_t opcode_data_len = data.BytesLeft(data_offset);
           const addr_t pc = m_address.GetFileAddress();
           llvm::MCInst inst;
 
-          disasm_sp->Lock(this, NULL);
           const size_t inst_size =
               mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
-          disasm_sp->Unlock();
           if (inst_size == 0)
             m_opcode.Clear();
           else {
             m_opcode.SetOpcodeBytes(opcode_data, inst_size);
             m_is_valid = true;
           }
         }
       }
void CalculateMnemonicOperandsAndComment(
       const lldb_private::ExecutionContext *exe_ctx) override {
     DataExtractor data;
     const AddressClass address_class = GetAddressClass();
 
     if (m_opcode.GetData(data)) {
       std::string out_string;
       std::string comment_string;
 
-      std::shared_ptr<DisassemblerLLVMC> disasm_sp(GetDisassembler());
-      if (disasm_sp) {
+      DisassemblerScope disasm(*this, exe_ctx);
+      if (disasm) {
         DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr;
 
         if (address_class == AddressClass::eCodeAlternateISA)
-          mc_disasm_ptr = disasm_sp->m_alternate_disasm_up.get();
+          mc_disasm_ptr = disasm->m_alternate_disasm_up.get();
         else
-          mc_disasm_ptr = disasm_sp->m_disasm_up.get();
+          mc_disasm_ptr = disasm->m_disasm_up.get();
 
         lldb::addr_t pc = m_address.GetFileAddress();
         m_using_file_addr = true;
 
-        const bool data_from_file = disasm_sp->m_data_from_file;
+        const bool data_from_file = disasm->m_data_from_file;
         bool use_hex_immediates = true;
         Disassembler::HexImmediateStyle hex_style = Disassembler::eHexStyleC;
 
         if (exe_ctx) {
           Target *target = exe_ctx->GetTargetPtr();
           if (target) {
             use_hex_immediates = target->GetUseHexImmediates();
             hex_style = target->GetHexImmediateStyle();
 
             if (!data_from_file) {
               const lldb::addr_t load_addr = m_address.GetLoadAddress(target);
               if (load_addr != LLDB_INVALID_ADDRESS) {
                 pc = load_addr;
                 m_using_file_addr = false;
               }
             }
           }
         }
 
-        disasm_sp->Lock(this, exe_ctx);
-
         const uint8_t *opcode_data = data.GetDataStart();
         const size_t opcode_data_len = data.GetByteSize();
         llvm::MCInst inst;
         size_t inst_size =
             mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
 
         if (inst_size > 0) {
           mc_disasm_ptr->SetStyle(use_hex_immediates, hex_style);
           mc_disasm_ptr->PrintMCInst(inst, out_string, comment_string);
 
           if (!comment_string.empty()) {
             AppendComment(comment_string);
           }
         }
 
-        disasm_sp->Unlock();
-
         if (inst_size == 0) {
           m_comment.assign("unknown opcode");
           inst_size = m_opcode.GetByteSize();
           StreamString mnemonic_strm;
           lldb::offset_t offset = 0;
           lldb::ByteOrder byte_order = data.GetByteOrder();
           switch (inst_size) {
           case 1: {
void CalculateMnemonicOperandsAndComment(
     }
   }
 
   bool IsValid() const { return m_is_valid; }
 
   bool UsingFileAddress() const { return m_using_file_addr; }
   size_t GetByteSize() const { return m_opcode.GetByteSize(); }
 
-  std::shared_ptr<DisassemblerLLVMC> GetDisassembler() {
-    return m_disasm_wp.lock();
+  /// Grants exclusive access to the disassembler and initializes it with the
+  /// given InstructionLLVMC and an optional ExecutionContext.
+  class DisassemblerScope {
+    std::shared_ptr<DisassemblerLLVMC> m_disasm;
+
+  public:
+    explicit DisassemblerScope(
+        InstructionLLVMC &i,
+        const lldb_private::ExecutionContext *exe_ctx = nullptr)
+        : m_disasm(i.m_disasm_wp.lock()) {
+      m_disasm->m_mutex.lock();
+      m_disasm->m_inst = &i;
+      m_disasm->m_exe_ctx = exe_ctx;
     }
+    ~DisassemblerScope() { m_disasm->m_mutex.unlock(); }
+
+    /// Evaluates to true if this scope contains a valid disassembler.
+    operator bool() const { return static_cast<bool>(m_disasm); }
+
+    std::shared_ptr<DisassemblerLLVMC> operator->() { return m_disasm; }
+  };
 
   static llvm::StringRef::const_iterator
   ConsumeWhitespace(llvm::StringRef::const_iterator osi,
                     llvm::StringRef::const_iterator ose) {
     while (osi != ose) {
       switch (*osi) {
       default:
         return osi;
if (Log *log =
       log->PutString(ss.GetString());
     }
 
     return true;
   }
 
   bool IsCall() override {
     if (m_is_call == eLazyBoolCalculate) {
-      std::shared_ptr<DisassemblerLLVMC> disasm_sp(GetDisassembler());
-      if (disasm_sp) {
-        disasm_sp->Lock(this, NULL);
+      DisassemblerScope disasm(*this);
+      if (disasm) {
         DataExtractor data;
         if (m_opcode.GetData(data)) {
           bool is_alternate_isa;
           lldb::addr_t pc = m_address.GetFileAddress();
 
           DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
-              GetDisasmToUse(is_alternate_isa);
+              GetDisasmToUse(is_alternate_isa, disasm);
           const uint8_t *opcode_data = data.GetDataStart();
           const size_t opcode_data_len = data.GetByteSize();
           llvm::MCInst inst;
           const size_t inst_size =
               mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
           if (inst_size == 0) {
             m_is_call = eLazyBoolNo;
           } else {
             if (mc_disasm_ptr->IsCall(inst))
               m_is_call = eLazyBoolYes;
             else
               m_is_call = eLazyBoolNo;
           }
         }
-        disasm_sp->Unlock();
       }
     }
     return m_is_call == eLazyBoolYes;
   }
 
 protected:
   std::weak_ptr<DisassemblerLLVMC> m_disasm_wp;
   LazyBool m_does_branch;
   LazyBool m_has_delay_slot;
   LazyBool m_is_call;
   bool m_is_valid;
   bool m_using_file_addr;
+
+private:
+  DisassemblerLLVMC::MCDisasmInstance *
+  GetDisasmToUse(bool &is_alternate_isa, DisassemblerScope &disasm) {
+    is_alternate_isa = false;
+    if (disasm) {
+      if (disasm->m_alternate_disasm_up) {
+        const AddressClass address_class = GetAddressClass();
+
+        if (address_class == AddressClass::eCodeAlternateISA) {
+          is_alternate_isa = true;
+          return disasm->m_alternate_disasm_up.get();
+        }
+      }
+      return disasm->m_disasm_up.get();
+    }
+    return nullptr;
+  }
 };
 
 std::unique_ptr<DisassemblerLLVMC::MCDisasmInstance>
 DisassemblerLLVMC::MCDisasmInstance::Create(const char *triple, const char *cpu,
                                             const char *features_str,
                                             unsigned flavor,
                                             DisassemblerLLVMC &owner) {
   using Instance = std::unique_ptr<DisassemblerLLVMC::MCDisasmInstance>;