Check existence of each required component during construction of LLVMCDisassembler.

[tatyana-krasnukha]

Summary

Actually, fix two issues:

1. remove repeat creation of reg_info, use m_reg_info_ap for createMCAsmInfo instead;
2. remove possibility to dereference nullptr during createMCAsmInfo invocation, that could lead to undefined behavior.

Placed checking of a component right after its creation to simplify the code and avoid same issues later.

[labath]

I think this makes the code hard to read. A much better approach would be to use Expected<T> + factory function pattern. So you would have something like:

Expected<unique_ptr<LLVMCDisassembler>> LLVMCDisassembler::Create(...) {
  do_first_thing_that_can_fail(...);
  if (first_thing_failed)
    return make_string_error("First thing failed");
  do_second_thing_that_can_fail();
  ...
  return make_unique<LLVMCDisassembler>(...); // can't fail
}

I think this is much more readable than the lambda approach, and it also allows you to get rid of the IsValid() function, as we only construct valid objects.

[tatyana-krasnukha]

Thank you, Pavel.
Would you mind if I move LLVMCDisassembler declaration in .cpp also? It looks like perfect candidate for pimpl.

And... doesn't DisassemblerLLVMC::LLVMCDisassembler confuse anyone but me?)

[labath]

In D41584#970945, @tatyana-krasnukha wrote:

Thank you, Pavel.
Would you mind if I move LLVMCDisassembler declaration in .cpp also? It looks like perfect candidate for pimpl.

Yes, that sounds like a good idea.

And... doesn't DisassemblerLLVMC::LLVMCDisassembler confuse anyone but me?)

Well... I haven't looked at this class in the past, but yes... it looks confusing. I wouldn't mind a name change.

Jason, any thoughts on this?

[tatyana-krasnukha]

Added function Create that creates an instance of LLVMCDisassembler only if pass all constraints.
Moved LLVMCDisassembler declaration to .cpp file, renamed to MCDisasmToolset (is this name ok?).
Added const qualifier to some functions of the class.
I also had the courage to remove ‘_ap’ suffixes from unique pointers.

[clayborg]

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

77–83

add "_up" suffix to all std::unique_ptr member variables.

989–1003

Do we need this? We are placing the MCDisasmToolset into std::unique_ptr<> member variables so this shouldn't be needed by anyone as the std::unique_ptr already has a move operator

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h

102

Maybe "MCDisasmInstance", "MCDisassembler" or just "Disassembler"?

103

add "_up" suffix to anything that is a std::unique_ptr

104

ditto

[clayborg]

I like the overall direction this patch is taking, just a few fixes.

[labath]

Looks good to me. Just make sure to respond to all of Greg's comments as well.

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

989–1003

The std::move is necessary in this context. The rvalue references can be dropped theoretically -- that will enforce that the constructor takes ownership of the passed-in arguments (as they will get implicitly deleted otherwise) -- right now the constructor can just do nothing and the caller will keep owning these objects.
However, this distinction seldom matters.

1002

this should be m_disasm and m_instr_printer (and make sure to test this with assertions enabled).

[tatyana-krasnukha]

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

1002

Thanks, I was not aware of this CMake option. Of course assertion failed.

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h

102

MCDisassembler already exists in llvm namespace (it is one of members of this class). Disassembler is base class of DisassemblerLLVMC. l thought about MCDisasmImpl also. Probably MCDisasmInstance is even better.

[tatyana-krasnukha]

Added "_up" suffix to each unique_ptr, renamed MCDisasmToolset to MCDisasmInstance.

[tatyana-krasnukha]

There is the function GetDisasmToUse in InstructionLLVMC class that can return nullptr. But this case is not handled in any usage. I suppose that caller functions cannot be invoked if !DisassemblerLLVMC::IsValid(). But it still looks dangerous for me. May be GetDisasmToUse should assert if neither m_disasm_up nor m_alternate_disasm_up exists? And may return a reference then.

[clayborg]

Fix comment spacing as mentioned in inline comments and this is good to go.

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h

98–101

Fix spacing on these comments since they are new.

[tatyana-krasnukha]

Fixed comment spacing. Changed the comment slightly (words “I added this class to…” sound now like I did this, so, replaced it with passive voice).

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h

 #include <mutex>
 #include <string>
 
-// Other libraries and framework includes
-#include "llvm-c/Disassembler.h"
-
 // Project includes
 #include "lldb/Core/Address.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/PluginManager.h"
 
-// Opaque references to C++ Objects in LLVM's MC.
-namespace llvm {
-class MCContext;
-class MCInst;
-class MCInstrInfo;
-class MCRegisterInfo;
-class MCDisassembler;
-class MCInstPrinter;
-class MCAsmInfo;
-class MCSubtargetInfo;
-} // namespace llvm
-
 class InstructionLLVMC;
 
 class DisassemblerLLVMC : public lldb_private::Disassembler {
-  // 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, I added this class to manage the actual
-  // disassemblers.
-  class LLVMCDisassembler {
-  public:
-    LLVMCDisassembler(const char *triple, const char *cpu,
-                      const char *features_str, unsigned flavor,
-                      DisassemblerLLVMC &owner);
-
-    ~LLVMCDisassembler();
-
-    uint64_t GetMCInst(const uint8_t *opcode_data, size_t opcode_data_len,
-                       lldb::addr_t pc, llvm::MCInst &mc_inst);
-    void PrintMCInst(llvm::MCInst &mc_inst, std::string &inst_string,
-                     std::string &comments_string);
-    void SetStyle(bool use_hex_immed, HexImmediateStyle hex_style);
-    bool CanBranch(llvm::MCInst &mc_inst);
-    bool HasDelaySlot(llvm::MCInst &mc_inst);
-    bool IsCall(llvm::MCInst &mc_inst);
-    bool IsValid() { return m_is_valid; }
-
-  private:
-    bool m_is_valid;
-    std::unique_ptr<llvm::MCContext> m_context_ap;
-    std::unique_ptr<llvm::MCAsmInfo> m_asm_info_ap;
-    std::unique_ptr<llvm::MCSubtargetInfo> m_subtarget_info_ap;
-    std::unique_ptr<llvm::MCInstrInfo> m_instr_info_ap;
-    std::unique_ptr<llvm::MCRegisterInfo> m_reg_info_ap;
-    std::unique_ptr<llvm::MCInstPrinter> m_instr_printer_ap;
-    std::unique_ptr<llvm::MCDisassembler> m_disasm_ap;
-  };
-
 public:
   DisassemblerLLVMC(const lldb_private::ArchSpec &arch,
                     const char *flavor /* = NULL */);

   bool FlavorValidForArchSpec(const lldb_private::ArchSpec &arch,
                               const char *flavor) override;
 
-  bool IsValid() {
-    return (m_disasm_ap.get() != NULL && m_disasm_ap->IsValid());
-  }
+  bool IsValid() const;
 
   int OpInfo(uint64_t PC, uint64_t Offset, uint64_t Size, int TagType,
              void *TagBug);

   std::mutex m_mutex;
   bool m_data_from_file;
 
-  std::unique_ptr<LLVMCDisassembler> m_disasm_ap;
-  std::unique_ptr<LLVMCDisassembler> m_alternate_disasm_ap;
+  // 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, I added this class 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_

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

 // C Includes
 // C++ Includes
 // Project includes
+#include "DisassemblerLLVMC.h"
+
+// Other libraries and framework includes
 #include "llvm-c/Disassembler.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/MC/MCAsmInfo.h"

 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 
-// Other libraries and framework includes
-#include "DisassemblerLLVMC.h"
-
 #include "lldb/Core/Address.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Symbol/SymbolContext.h"

 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Log.h"
+#include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 
-#include "lldb/Utility/RegularExpression.h"
-
 using namespace lldb;
 using namespace lldb_private;
 
+class DisassemblerLLVMC::MCDisasmInstance {
+public:
+  static std::unique_ptr<MCDisasmInstance>
+  Create(const char *triple, const char *cpu, const char *features_str,
+         unsigned flavor, DisassemblerLLVMC &owner);
+
+  ~MCDisasmInstance() = default;
+
+  uint64_t GetMCInst(const uint8_t *opcode_data, size_t opcode_data_len,
+                     lldb::addr_t pc, llvm::MCInst &mc_inst) const;
+  void PrintMCInst(llvm::MCInst &mc_inst, std::string &inst_string,
+                   std::string &comments_string);
+  void SetStyle(bool use_hex_immed, HexImmediateStyle hex_style);
+  bool CanBranch(llvm::MCInst &mc_inst) const;
+  bool HasDelaySlot(llvm::MCInst &mc_inst) const;
+  bool IsCall(llvm::MCInst &mc_inst) const;
+
+private:
+  MCDisasmInstance(std::unique_ptr<llvm::MCInstrInfo> &&instr_info_up,
+                   std::unique_ptr<llvm::MCRegisterInfo> &&reg_info_up,
+                   std::unique_ptr<llvm::MCSubtargetInfo> &&subtarget_info_up,
+                   std::unique_ptr<llvm::MCAsmInfo> &&asm_info_up,
+                   std::unique_ptr<llvm::MCContext> &&context_up,
+                   std::unique_ptr<llvm::MCDisassembler> &&disasm_up,
+                   std::unique_ptr<llvm::MCInstPrinter> &&instr_printer_up);
+
+  std::unique_ptr<llvm::MCInstrInfo> m_instr_info_up;
+  std::unique_ptr<llvm::MCRegisterInfo> m_reg_info_up;
+  std::unique_ptr<llvm::MCSubtargetInfo> m_subtarget_info_up;
+  std::unique_ptr<llvm::MCAsmInfo> m_asm_info_up;
+  std::unique_ptr<llvm::MCContext> m_context_up;
+  std::unique_ptr<llvm::MCDisassembler> m_disasm_up;
+  std::unique_ptr<llvm::MCInstPrinter> m_instr_printer_up;
+};
+
 class InstructionLLVMC : public lldb_private::Instruction {
 public:
   InstructionLLVMC(DisassemblerLLVMC &disasm,

           bool is_alternate_isa;
           lldb::addr_t pc = m_address.GetFileAddress();
 
-          DisassemblerLLVMC::LLVMCDisassembler *mc_disasm_ptr =
+          DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
               GetDisasmToUse(is_alternate_isa);
           const uint8_t *opcode_data = data.GetDataStart();
           const size_t opcode_data_len = data.GetByteSize();

           bool is_alternate_isa;
           lldb::addr_t pc = m_address.GetFileAddress();
 
-          DisassemblerLLVMC::LLVMCDisassembler *mc_disasm_ptr =
+          DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
               GetDisasmToUse(is_alternate_isa);
           const uint8_t *opcode_data = data.GetDataStart();
           const size_t opcode_data_len = data.GetByteSize();

     return m_has_delay_slot == eLazyBoolYes;
   }
 
-  DisassemblerLLVMC::LLVMCDisassembler *GetDisasmToUse(bool &is_alternate_isa) {
+  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_ap.get() != NULL) {
+      if (disasm_sp->m_alternate_disasm_up) {
         const AddressClass address_class = GetAddressClass();
 
         if (address_class == eAddressClassCodeAlternateISA) {
           is_alternate_isa = true;
-          return disasm_sp->m_alternate_disasm_ap.get();
+          return disasm_sp->m_alternate_disasm_up.get();
         }
       }
-      return disasm_sp->m_disasm_ap.get();
+      return disasm_sp->m_disasm_up.get();
     }
     return nullptr;
   }

       }
       if (!got_op) {
         bool is_alternate_isa = false;
-        DisassemblerLLVMC::LLVMCDisassembler *mc_disasm_ptr =
+        DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
             GetDisasmToUse(is_alternate_isa);
 
         const llvm::Triple::ArchType machine = arch.GetMachine();

 
       std::shared_ptr<DisassemblerLLVMC> disasm_sp(GetDisassembler());
       if (disasm_sp) {
-        DisassemblerLLVMC::LLVMCDisassembler *mc_disasm_ptr;
+        DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr;
 
         if (address_class == eAddressClassCodeAlternateISA)
-          mc_disasm_ptr = disasm_sp->m_alternate_disasm_ap.get();
+          mc_disasm_ptr = disasm_sp->m_alternate_disasm_up.get();
         else
-          mc_disasm_ptr = disasm_sp->m_disasm_ap.get();
+          mc_disasm_ptr = disasm_sp->m_disasm_up.get();
 
         lldb::addr_t pc = m_address.GetFileAddress();
         m_using_file_addr = true;

           bool is_alternate_isa;
           lldb::addr_t pc = m_address.GetFileAddress();
 
-          DisassemblerLLVMC::LLVMCDisassembler *mc_disasm_ptr =
+          DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
               GetDisasmToUse(is_alternate_isa);
           const uint8_t *opcode_data = data.GetDataStart();
           const size_t opcode_data_len = data.GetByteSize();

   bool m_using_file_addr;
 };
 
-DisassemblerLLVMC::LLVMCDisassembler::LLVMCDisassembler(
-    const char *triple, const char *cpu, const char *features_str,
-    unsigned flavor, DisassemblerLLVMC &owner)
-    : m_is_valid(true) {
+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>;
+
   std::string Status;
   const llvm::Target *curr_target =
       llvm::TargetRegistry::lookupTarget(triple, Status);
-  if (!curr_target) {
-    m_is_valid = false;
-    return;
-  }
+  if (!curr_target)
+    return Instance();
 
-  m_instr_info_ap.reset(curr_target->createMCInstrInfo());
-  m_reg_info_ap.reset(curr_target->createMCRegInfo(triple));
+  std::unique_ptr<llvm::MCInstrInfo> instr_info_up(
+      curr_target->createMCInstrInfo());
+  if (!instr_info_up)
+    return Instance();
 
-  m_subtarget_info_ap.reset(
+  std::unique_ptr<llvm::MCRegisterInfo> reg_info_up(
+      curr_target->createMCRegInfo(triple));
+  if (!reg_info_up)
+    return Instance();
+
+  std::unique_ptr<llvm::MCSubtargetInfo> subtarget_info_up(
       curr_target->createMCSubtargetInfo(triple, cpu, features_str));
+  if (!subtarget_info_up)
+    return Instance();
 
-  std::unique_ptr<llvm::MCRegisterInfo> reg_info(
-      curr_target->createMCRegInfo(triple));
-  m_asm_info_ap.reset(curr_target->createMCAsmInfo(*reg_info, triple));
+  std::unique_ptr<llvm::MCAsmInfo> asm_info_up(
+      curr_target->createMCAsmInfo(*reg_info_up, triple));
+  if (!asm_info_up)
+    return Instance();
 
-  if (m_instr_info_ap.get() == NULL || m_reg_info_ap.get() == NULL ||
-      m_subtarget_info_ap.get() == NULL || m_asm_info_ap.get() == NULL) {
-    m_is_valid = false;
-    return;
-  }
+  std::unique_ptr<llvm::MCContext> context_up(
+      new llvm::MCContext(asm_info_up.get(), reg_info_up.get(), 0));
+  if (!context_up)
+    return Instance();
 
-  m_context_ap.reset(
-      new llvm::MCContext(m_asm_info_ap.get(), m_reg_info_ap.get(), 0));
+  std::unique_ptr<llvm::MCDisassembler> disasm_up(
+      curr_target->createMCDisassembler(*subtarget_info_up, *context_up));
+  if (!disasm_up)
+    return Instance();
 
-  m_disasm_ap.reset(curr_target->createMCDisassembler(
-      *m_subtarget_info_ap.get(), *m_context_ap.get()));
-  if (m_disasm_ap.get() && m_context_ap.get()) {
-    std::unique_ptr<llvm::MCRelocationInfo> RelInfo(
-        curr_target->createMCRelocationInfo(triple, *m_context_ap.get()));
-    if (!RelInfo) {
-      m_is_valid = false;
-      return;
-    }
-    std::unique_ptr<llvm::MCSymbolizer> symbolizer_up(
-        curr_target->createMCSymbolizer(
-            triple, NULL, DisassemblerLLVMC::SymbolLookupCallback,
-            (void *)&owner, m_context_ap.get(), std::move(RelInfo)));
-    m_disasm_ap->setSymbolizer(std::move(symbolizer_up));
+  std::unique_ptr<llvm::MCRelocationInfo> rel_info_up(
+      curr_target->createMCRelocationInfo(triple, *context_up));
+  if (!rel_info_up)
+    return Instance();
 
-    unsigned asm_printer_variant;
-    if (flavor == ~0U)
-      asm_printer_variant = m_asm_info_ap->getAssemblerDialect();
-    else {
-      asm_printer_variant = flavor;
-    }
+  std::unique_ptr<llvm::MCSymbolizer> symbolizer_up(
+      curr_target->createMCSymbolizer(
+          triple, nullptr, DisassemblerLLVMC::SymbolLookupCallback, &owner,
+          context_up.get(), std::move(rel_info_up)));
+  disasm_up->setSymbolizer(std::move(symbolizer_up));
 
-    m_instr_printer_ap.reset(curr_target->createMCInstPrinter(
-        llvm::Triple{triple}, asm_printer_variant, *m_asm_info_ap.get(),
-        *m_instr_info_ap.get(), *m_reg_info_ap.get()));
-    if (m_instr_printer_ap.get() == NULL) {
-      m_disasm_ap.reset();
-      m_is_valid = false;
-    }
-  } else
-    m_is_valid = false;
+  unsigned asm_printer_variant =
+      flavor == ~0U ? asm_info_up->getAssemblerDialect() : flavor;
+
+  std::unique_ptr<llvm::MCInstPrinter> instr_printer_up(
+      curr_target->createMCInstPrinter(llvm::Triple{triple},
+                                       asm_printer_variant, *asm_info_up,
+                                       *instr_info_up, *reg_info_up));
+  if (!instr_printer_up)
+    return Instance();
+
+  return Instance(
+      new MCDisasmInstance(std::move(instr_info_up), std::move(reg_info_up),
+                           std::move(subtarget_info_up), std::move(asm_info_up),
+                           std::move(context_up), std::move(disasm_up),
+                           std::move(instr_printer_up)));
 }
 
-DisassemblerLLVMC::LLVMCDisassembler::~LLVMCDisassembler() = default;
+DisassemblerLLVMC::MCDisasmInstance::MCDisasmInstance(
+    std::unique_ptr<llvm::MCInstrInfo> &&instr_info_up,
+    std::unique_ptr<llvm::MCRegisterInfo> &&reg_info_up,
+    std::unique_ptr<llvm::MCSubtargetInfo> &&subtarget_info_up,
+    std::unique_ptr<llvm::MCAsmInfo> &&asm_info_up,
+    std::unique_ptr<llvm::MCContext> &&context_up,
+    std::unique_ptr<llvm::MCDisassembler> &&disasm_up,
+    std::unique_ptr<llvm::MCInstPrinter> &&instr_printer_up)
+    : m_instr_info_up(std::move(instr_info_up)),
+      m_reg_info_up(std::move(reg_info_up)),
+      m_subtarget_info_up(std::move(subtarget_info_up)),
+      m_asm_info_up(std::move(asm_info_up)),
+      m_context_up(std::move(context_up)), m_disasm_up(std::move(disasm_up)),
+      m_instr_printer_up(std::move(instr_printer_up)) {
+  assert(m_instr_info_up && m_reg_info_up && m_subtarget_info_up &&
+         m_asm_info_up && m_context_up && m_disasm_up && m_instr_printer_up);
+}
 
-uint64_t DisassemblerLLVMC::LLVMCDisassembler::GetMCInst(
+uint64_t DisassemblerLLVMC::MCDisasmInstance::GetMCInst(
     const uint8_t *opcode_data, size_t opcode_data_len, lldb::addr_t pc,
-    llvm::MCInst &mc_inst) {
+    llvm::MCInst &mc_inst) const {
   llvm::ArrayRef<uint8_t> data(opcode_data, opcode_data_len);
   llvm::MCDisassembler::DecodeStatus status;
 
   uint64_t new_inst_size;
-  status = m_disasm_ap->getInstruction(mc_inst, new_inst_size, data, pc,
+  status = m_disasm_up->getInstruction(mc_inst, new_inst_size, data, pc,
                                        llvm::nulls(), llvm::nulls());
   if (status == llvm::MCDisassembler::Success)
     return new_inst_size;

     return 0;
 }
 
-void DisassemblerLLVMC::LLVMCDisassembler::PrintMCInst(
+void DisassemblerLLVMC::MCDisasmInstance::PrintMCInst(
     llvm::MCInst &mc_inst, std::string &inst_string,
     std::string &comments_string) {
   llvm::raw_string_ostream inst_stream(inst_string);
   llvm::raw_string_ostream comments_stream(comments_string);
 
-  m_instr_printer_ap->setCommentStream(comments_stream);
-  m_instr_printer_ap->printInst(&mc_inst, inst_stream, llvm::StringRef(),
-                                *m_subtarget_info_ap);
-  m_instr_printer_ap->setCommentStream(llvm::nulls());
+  m_instr_printer_up->setCommentStream(comments_stream);
+  m_instr_printer_up->printInst(&mc_inst, inst_stream, llvm::StringRef(),
+                                *m_subtarget_info_up);
+  m_instr_printer_up->setCommentStream(llvm::nulls());
   comments_stream.flush();
 
   static std::string g_newlines("\r\n");

   }
 }
 
-void DisassemblerLLVMC::LLVMCDisassembler::SetStyle(
+void DisassemblerLLVMC::MCDisasmInstance::SetStyle(
     bool use_hex_immed, HexImmediateStyle hex_style) {
-  m_instr_printer_ap->setPrintImmHex(use_hex_immed);
+  m_instr_printer_up->setPrintImmHex(use_hex_immed);
   switch (hex_style) {
   case eHexStyleC:
-    m_instr_printer_ap->setPrintHexStyle(llvm::HexStyle::C);
+    m_instr_printer_up->setPrintHexStyle(llvm::HexStyle::C);
     break;
   case eHexStyleAsm:
-    m_instr_printer_ap->setPrintHexStyle(llvm::HexStyle::Asm);
+    m_instr_printer_up->setPrintHexStyle(llvm::HexStyle::Asm);
     break;
   }
 }
 
-bool DisassemblerLLVMC::LLVMCDisassembler::CanBranch(llvm::MCInst &mc_inst) {
-  return m_instr_info_ap->get(mc_inst.getOpcode())
-      .mayAffectControlFlow(mc_inst, *m_reg_info_ap.get());
+bool DisassemblerLLVMC::MCDisasmInstance::CanBranch(
+    llvm::MCInst &mc_inst) const {
+  return m_instr_info_up->get(mc_inst.getOpcode())
+      .mayAffectControlFlow(mc_inst, *m_reg_info_up);
 }
 
-bool DisassemblerLLVMC::LLVMCDisassembler::HasDelaySlot(llvm::MCInst &mc_inst) {
-  return m_instr_info_ap->get(mc_inst.getOpcode()).hasDelaySlot();
+bool DisassemblerLLVMC::MCDisasmInstance::HasDelaySlot(
+    llvm::MCInst &mc_inst) const {
+  return m_instr_info_up->get(mc_inst.getOpcode()).hasDelaySlot();
 }
 
-bool DisassemblerLLVMC::LLVMCDisassembler::IsCall(llvm::MCInst &mc_inst) {
-  return m_instr_info_ap->get(mc_inst.getOpcode()).isCall();
+bool DisassemblerLLVMC::MCDisasmInstance::IsCall(llvm::MCInst &mc_inst) const {
+  return m_instr_info_up->get(mc_inst.getOpcode()).isCall();
 }
 
 DisassemblerLLVMC::DisassemblerLLVMC(const ArchSpec &arch,

   if (triple.getArch() == llvm::Triple::aarch64)
     features_str += "+v8.2a";
 
-  m_disasm_ap.reset(new LLVMCDisassembler(triple_str, cpu, features_str.c_str(),
-                                          flavor, *this));
-  if (!m_disasm_ap->IsValid()) {
-    // We use m_disasm_ap.get() to tell whether we are valid or not, so if this
-    // isn't good for some reason,
-    // we reset it, and then we won't be valid and FindPlugin will fail and we
-    // won't get used.
-    m_disasm_ap.reset();
-  }
+  // We use m_disasm_ap.get() to tell whether we are valid or not,
+  // so if this isn't good for some reason,
+  // we won't be valid and FindPlugin will fail and we won't get used.
+  m_disasm_up = MCDisasmInstance::Create(triple_str, cpu, features_str.c_str(),
+                                         flavor, *this);
 
   llvm::Triple::ArchType llvm_arch = triple.getArch();
 

   // thumb instruction disassembler.
   if (llvm_arch == llvm::Triple::arm) {
     std::string thumb_triple(thumb_arch.GetTriple().getTriple());
-    m_alternate_disasm_ap.reset(
-        new LLVMCDisassembler(thumb_triple.c_str(), "", "", flavor, *this));
-    if (!m_alternate_disasm_ap->IsValid()) {
-      m_disasm_ap.reset();
-      m_alternate_disasm_ap.reset();
-    }
+    m_alternate_disasm_up =
+        MCDisasmInstance::Create(thumb_triple.c_str(), "", "", flavor, *this);
+    if (!m_alternate_disasm_up)
+      m_disasm_up.reset();
+
   } else if (llvm_arch == llvm::Triple::mips ||
              llvm_arch == llvm::Triple::mipsel ||
              llvm_arch == llvm::Triple::mips64 ||

     else if (arch_flags & ArchSpec::eMIPSAse_micromips)
       features_str += "+micromips,";
 
-    m_alternate_disasm_ap.reset(new LLVMCDisassembler(
-        triple_str, cpu, features_str.c_str(), flavor, *this));
-    if (!m_alternate_disasm_ap->IsValid()) {
-      m_disasm_ap.reset();
-      m_alternate_disasm_ap.reset();
-    }
+    m_alternate_disasm_up = MCDisasmInstance::Create(
+        triple_str, cpu, features_str.c_str(), flavor, *this);
+    if (!m_alternate_disasm_up)
+      m_disasm_up.reset();
   }
 }
 

 
     AddressClass address_class = eAddressClassCode;
 
-    if (m_alternate_disasm_ap.get() != NULL)
+    if (m_alternate_disasm_up)
       address_class = inst_addr.GetAddressClass();
 
     InstructionSP inst_sp(

     return false;
 }
 
+bool DisassemblerLLVMC::IsValid() const { return m_disasm_up.operator bool(); }
+
 int DisassemblerLLVMC::OpInfo(uint64_t PC, uint64_t Offset, uint64_t Size,
                               int tag_type, void *tag_bug) {
   switch (tag_type) {