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

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

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

192

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

192

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

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);
 
+  struct Guard {
+    DisassemblerLLVMC *m_instance;
+    Guard(DisassemblerLLVMC *instance, InstructionLLVMC *inst,
+          const lldb_private::ExecutionContext *exe_ctx = nullptr)
+        : m_instance(instance) {
+      m_instance->Lock(inst, exe_ctx);
+    }
+
+    ~Guard() { m_instance->Unlock(); }
+  };
+
   void Lock(InstructionLLVMC *inst,
             const lldb_private::ExecutionContext *exe_ctx) {
     m_mutex.lock();
     m_inst = inst;
     m_exe_ctx = exe_ctx;
   }
 
   void Unlock() {

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

InstructionLLVMC(DisassemblerLLVMC &disasm,
         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);
+        DisassemblerLLVMC::Guard guard(disasm_sp.get(), this);
         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);
           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);
+        DisassemblerLLVMC::Guard guard(disasm_sp.get(), this);
         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);
           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());
public:
   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();
       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_sp) {
           // 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();
+          size_t inst_size;
+          {
+            DisassemblerLLVMC::Guard guard(disasm_sp.get(), this);
+            inst_size = mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len,
+                                                 pc, inst);
+          }
+
           if (inst_size == 0)
             m_opcode.Clear();
           else {
             m_opcode.SetOpcodeBytes(opcode_data, inst_size);
             m_is_valid = true;
           }
         }
       }
bool ParseOperands(
 
     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);
+        DisassemblerLLVMC::Guard guard(disasm_sp.get(), this);
         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);
           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;