labath updated this revision to Diff 106600.
labath marked an inline comment as done.
labath added a comment.

The version with a container object for ArchSpec+Plugin combo


https://reviews.llvm.org/D31172

Files:
  include/lldb/Core/ArchSpec.h
  include/lldb/Core/Architecture.h
  include/lldb/Core/PluginManager.h
  include/lldb/Target/Process.h
  include/lldb/Target/Target.h
  packages/Python/lldbsuite/test/arm/breakpoint-it/Makefile
  packages/Python/lldbsuite/test/arm/breakpoint-it/TestBreakpointIt.py
  packages/Python/lldbsuite/test/arm/breakpoint-it/main.c
  source/API/SystemInitializerFull.cpp
  source/Core/ArchSpec.cpp
  source/Core/PluginManager.cpp
  source/Plugins/Architecture/Arm/ArchitectureArm.cpp
  source/Plugins/Architecture/Arm/ArchitectureArm.h
  source/Plugins/Architecture/Arm/CMakeLists.txt
  source/Plugins/Architecture/CMakeLists.txt
  source/Plugins/CMakeLists.txt
  source/Target/Process.cpp
  source/Target/Target.cpp
  source/Target/Thread.cpp

Index: source/Target/Thread.cpp
===================================================================
--- source/Target/Thread.cpp
+++ source/Target/Thread.cpp
@@ -442,10 +442,9 @@
     if (m_stop_info_override_stop_id != process_stop_id) {
       m_stop_info_override_stop_id = process_stop_id;
       if (m_stop_info_sp) {
-        ArchSpec::StopInfoOverrideCallbackType callback =
-            GetProcess()->GetStopInfoOverrideCallback();
-        if (callback)
-          callback(*this);
+        if (Architecture *arch =
+                process_sp->GetTarget().GetArchitecturePlugin())
+          arch->OverrideStopInfo(*this);
       }
     }
   }
Index: source/Target/Target.cpp
===================================================================
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -26,6 +26,7 @@
 #include "lldb/Core/Event.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
+#include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/SourceManager.h"
 #include "lldb/Core/State.h"
@@ -65,6 +66,16 @@
 
 constexpr std::chrono::milliseconds EvaluateExpressionOptions::default_timeout;
 
+Target::Arch::Arch(const ArchSpec &spec)
+    : m_spec(spec),
+      m_plugin_up(PluginManager::CreateArchitectureInstance(spec)) {}
+
+const Target::Arch& Target::Arch::operator=(const ArchSpec &spec) {
+  m_spec = spec;
+  m_plugin_up = PluginManager::CreateArchitectureInstance(spec);
+  return *this;
+}
+
 ConstString &Target::GetStaticBroadcasterClass() {
   static ConstString class_name("lldb.target");
   return class_name;
@@ -76,12 +87,12 @@
       Broadcaster(debugger.GetBroadcasterManager(),
                   Target::GetStaticBroadcasterClass().AsCString()),
       ExecutionContextScope(), m_debugger(debugger), m_platform_sp(platform_sp),
-      m_mutex(), m_arch(target_arch), m_images(this), m_section_load_history(),
-      m_breakpoint_list(false), m_internal_breakpoint_list(true),
-      m_watchpoint_list(), m_process_sp(), m_search_filter_sp(),
-      m_image_search_paths(ImageSearchPathsChanged, this), m_ast_importer_sp(),
-      m_source_manager_ap(), m_stop_hooks(), m_stop_hook_next_id(0),
-      m_valid(true), m_suppress_stop_hooks(false),
+      m_mutex(), m_arch(target_arch),
+      m_images(this), m_section_load_history(), m_breakpoint_list(false),
+      m_internal_breakpoint_list(true), m_watchpoint_list(), m_process_sp(),
+      m_search_filter_sp(), m_image_search_paths(ImageSearchPathsChanged, this),
+      m_ast_importer_sp(), m_source_manager_ap(), m_stop_hooks(),
+      m_stop_hook_next_id(0), m_valid(true), m_suppress_stop_hooks(false),
       m_is_dummy_target(is_dummy_target)
 
 {
@@ -96,10 +107,11 @@
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
   if (log)
     log->Printf("%p Target::Target()", static_cast<void *>(this));
-  if (m_arch.IsValid()) {
-    LogIfAnyCategoriesSet(
-        LIBLLDB_LOG_TARGET, "Target::Target created with architecture %s (%s)",
-        m_arch.GetArchitectureName(), m_arch.GetTriple().getTriple().c_str());
+  if (target_arch.IsValid()) {
+    LogIfAnyCategoriesSet(LIBLLDB_LOG_TARGET,
+                          "Target::Target created with architecture %s (%s)",
+                          target_arch.GetArchitectureName(),
+                          target_arch.GetTriple().getTriple().c_str());
   }
 }
 
@@ -243,7 +255,7 @@
   m_valid = false;
   DeleteCurrentProcess();
   m_platform_sp.reset();
-  m_arch.Clear();
+  m_arch = ArchSpec();
   ClearModules(true);
   m_section_load_history.Clear();
   const bool notify = false;
@@ -1226,8 +1238,8 @@
 
 void Target::DidExec() {
   // When a process exec's we need to know about it so we can do some cleanup.
-  m_breakpoint_list.RemoveInvalidLocations(m_arch);
-  m_internal_breakpoint_list.RemoveInvalidLocations(m_arch);
+  m_breakpoint_list.RemoveInvalidLocations(m_arch.GetSpec());
+  m_internal_breakpoint_list.RemoveInvalidLocations(m_arch.GetSpec());
 }
 
 void Target::SetExecutableModule(ModuleSP &executable_sp,
@@ -1245,13 +1257,12 @@
 
     // If we haven't set an architecture yet, reset our architecture based on
     // what we found in the executable module.
-    if (!m_arch.IsValid()) {
+    if (!m_arch.GetSpec().IsValid()) {
       m_arch = executable_sp->GetArchitecture();
-      if (log)
-        log->Printf("Target::SetExecutableModule setting architecture to %s "
-                    "(%s) based on executable file",
-                    m_arch.GetArchitectureName(),
-                    m_arch.GetTriple().getTriple().c_str());
+      LLDB_LOG(log,
+               "setting architecture to {0} ({1}) based on executable file",
+               m_arch.GetSpec().GetArchitectureName(),
+               m_arch.GetSpec().GetTriple().getTriple());
     }
 
     FileSpecList dependent_files;
@@ -1269,7 +1280,7 @@
         else
           platform_dependent_file_spec = dependent_file_spec;
 
-        ModuleSpec module_spec(platform_dependent_file_spec, m_arch);
+        ModuleSpec module_spec(platform_dependent_file_spec, m_arch.GetSpec());
         ModuleSP image_module_sp(GetSharedModule(module_spec));
         if (image_module_sp) {
           ObjectFile *objfile = image_module_sp->GetObjectFile();
@@ -1283,21 +1294,21 @@
 
 bool Target::SetArchitecture(const ArchSpec &arch_spec) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_TARGET));
-  bool missing_local_arch = !m_arch.IsValid();
+  bool missing_local_arch = !m_arch.GetSpec().IsValid();
   bool replace_local_arch = true;
   bool compatible_local_arch = false;
   ArchSpec other(arch_spec);
 
   if (!missing_local_arch) {
-    if (m_arch.IsCompatibleMatch(arch_spec)) {
-      other.MergeFrom(m_arch);
+    if (m_arch.GetSpec().IsCompatibleMatch(arch_spec)) {
+      other.MergeFrom(m_arch.GetSpec());
 
-      if (m_arch.IsCompatibleMatch(other)) {
+      if (m_arch.GetSpec().IsCompatibleMatch(other)) {
         compatible_local_arch = true;
         bool arch_changed, vendor_changed, os_changed, os_ver_changed,
             env_changed;
 
-        m_arch.PiecewiseTripleCompare(other, arch_changed, vendor_changed,
+        m_arch.GetSpec().PiecewiseTripleCompare(other, arch_changed, vendor_changed,
                                       os_changed, os_ver_changed, env_changed);
 
         if (!arch_changed && !vendor_changed && !os_changed && !env_changed)
@@ -1311,10 +1322,9 @@
     // update the architecture, unless the one we already have is more specified
     if (replace_local_arch)
       m_arch = other;
-    if (log)
-      log->Printf("Target::SetArchitecture set architecture to %s (%s)",
-                  m_arch.GetArchitectureName(),
-                  m_arch.GetTriple().getTriple().c_str());
+    LLDB_LOG(log, "set architecture to {0} ({1})",
+             m_arch.GetSpec().GetArchitectureName(),
+             m_arch.GetSpec().GetTriple().getTriple());
     return true;
   }
 
@@ -1351,12 +1361,12 @@
 
 bool Target::MergeArchitecture(const ArchSpec &arch_spec) {
   if (arch_spec.IsValid()) {
-    if (m_arch.IsCompatibleMatch(arch_spec)) {
+    if (m_arch.GetSpec().IsCompatibleMatch(arch_spec)) {
       // The current target arch is compatible with "arch_spec", see if we
       // can improve our current architecture using bits from "arch_spec"
 
       // Merge bits from arch_spec into "merged_arch" and set our architecture
-      ArchSpec merged_arch(m_arch);
+      ArchSpec merged_arch(m_arch.GetSpec());
       merged_arch.MergeFrom(arch_spec);
       return SetArchitecture(merged_arch);
     } else {
@@ -1684,8 +1694,8 @@
     size_t bytes_read =
         ReadMemory(addr, prefer_file_cache, &uval, byte_size, error);
     if (bytes_read == byte_size) {
-      DataExtractor data(&uval, sizeof(uval), m_arch.GetByteOrder(),
-                         m_arch.GetAddressByteSize());
+      DataExtractor data(&uval, sizeof(uval), m_arch.GetSpec().GetByteOrder(),
+                         m_arch.GetSpec().GetAddressByteSize());
       lldb::offset_t offset = 0;
       if (byte_size <= 4)
         scalar = data.GetMaxU32(&offset, byte_size);
@@ -1719,7 +1729,7 @@
                                    Status &error, Address &pointer_addr) {
   Scalar scalar;
   if (ReadScalarIntegerFromMemory(addr, prefer_file_cache,
-                                  m_arch.GetAddressByteSize(), false, scalar,
+                                  m_arch.GetSpec().GetAddressByteSize(), false, scalar,
                                   error)) {
     addr_t pointer_vm_addr = scalar.ULongLong(LLDB_INVALID_ADDRESS);
     if (pointer_vm_addr != LLDB_INVALID_ADDRESS) {
@@ -2212,7 +2222,7 @@
 lldb::addr_t Target::GetCallableLoadAddress(lldb::addr_t load_addr,
                                             AddressClass addr_class) const {
   addr_t code_addr = load_addr;
-  switch (m_arch.GetMachine()) {
+  switch (m_arch.GetSpec().GetMachine()) {
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
@@ -2271,7 +2281,7 @@
 lldb::addr_t Target::GetOpcodeLoadAddress(lldb::addr_t load_addr,
                                           AddressClass addr_class) const {
   addr_t opcode_addr = load_addr;
-  switch (m_arch.GetMachine()) {
+  switch (m_arch.GetSpec().GetMachine()) {
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
@@ -2303,7 +2313,7 @@
   addr_t breakable_addr = addr;
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
 
-  switch (m_arch.GetMachine()) {
+  switch (m_arch.GetSpec().GetMachine()) {
   default:
     break;
   case llvm::Triple::mips:
@@ -2314,7 +2324,7 @@
     addr_t current_offset = 0;
     uint32_t loop_count = 0;
     Address resolved_addr;
-    uint32_t arch_flags = m_arch.GetFlags();
+    uint32_t arch_flags = m_arch.GetSpec().GetFlags();
     bool IsMips16 = arch_flags & ArchSpec::eMIPSAse_mips16;
     bool IsMicromips = arch_flags & ArchSpec::eMIPSAse_micromips;
     SectionLoadList &section_load_list = GetSectionLoadList();
@@ -2367,7 +2377,7 @@
 
     // Create Disassembler Instance
     lldb::DisassemblerSP disasm_sp(
-        Disassembler::FindPlugin(m_arch, nullptr, nullptr));
+        Disassembler::FindPlugin(m_arch.GetSpec(), nullptr, nullptr));
 
     ExecutionContext exe_ctx;
     CalculateExecutionContext(exe_ctx);
Index: source/Target/Process.cpp
===================================================================
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -743,8 +743,7 @@
       m_profile_data_comm_mutex(), m_profile_data(), m_iohandler_sync(0),
       m_memory_cache(*this), m_allocated_memory_cache(*this),
       m_should_detach(false), m_next_event_action_ap(), m_public_run_lock(),
-      m_private_run_lock(), m_stop_info_override_callback(nullptr),
-      m_finalizing(false), m_finalize_called(false),
+      m_private_run_lock(), m_finalizing(false), m_finalize_called(false),
       m_clear_thread_plans_on_stop(false), m_force_next_event_delivery(false),
       m_last_broadcast_state(eStateInvalid), m_destroy_in_process(false),
       m_can_interpret_function_calls(false), m_warnings_issued(),
@@ -871,7 +870,6 @@
   m_language_runtimes.clear();
   m_instrumentation_runtimes.clear();
   m_next_event_action_ap.reset();
-  m_stop_info_override_callback = nullptr;
   // Clear the last natural stop ID since it has a strong
   // reference to this process
   m_mod_id.SetStopEventForLastNaturalStopID(EventSP());
@@ -2712,7 +2710,6 @@
   m_system_runtime_ap.reset();
   m_os_ap.reset();
   m_process_input_reader.reset();
-  m_stop_info_override_callback = nullptr;
 
   Module *exe_module = GetTarget().GetExecutableModulePointer();
   if (exe_module) {
@@ -2800,9 +2797,6 @@
             else
               StartPrivateStateThread();
 
-            m_stop_info_override_callback =
-                GetTarget().GetArchitecture().GetStopInfoOverrideCallback();
-
             // Target was stopped at entry as was intended. Need to notify the
             // listeners
             // about it.
@@ -2986,7 +2980,6 @@
   m_jit_loaders_ap.reset();
   m_system_runtime_ap.reset();
   m_os_ap.reset();
-  m_stop_info_override_callback = nullptr;
 
   lldb::pid_t attach_pid = attach_info.GetProcessID();
   Status error;
@@ -3219,8 +3212,6 @@
                         : "<none>");
     }
   }
-
-  m_stop_info_override_callback = process_arch.GetStopInfoOverrideCallback();
 }
 
 Status Process::ConnectRemote(Stream *strm, llvm::StringRef remote_url) {
@@ -5849,7 +5840,6 @@
   m_instrumentation_runtimes.clear();
   m_thread_list.DiscardThreadPlans();
   m_memory_cache.Clear(true);
-  m_stop_info_override_callback = nullptr;
   DoDidExec();
   CompleteAttach();
   // Flush the process (threads and all stack frames) after running
Index: source/Plugins/CMakeLists.txt
===================================================================
--- source/Plugins/CMakeLists.txt
+++ source/Plugins/CMakeLists.txt
@@ -1,4 +1,5 @@
 add_subdirectory(ABI)
+add_subdirectory(Architecture)
 add_subdirectory(Disassembler)
 add_subdirectory(DynamicLoader)
 add_subdirectory(ExpressionParser)
Index: source/Plugins/Architecture/CMakeLists.txt
===================================================================
--- /dev/null
+++ source/Plugins/Architecture/CMakeLists.txt
@@ -0,0 +1 @@
+add_subdirectory(Arm)
Index: source/Plugins/Architecture/Arm/CMakeLists.txt
===================================================================
--- /dev/null
+++ source/Plugins/Architecture/Arm/CMakeLists.txt
@@ -0,0 +1,10 @@
+add_lldb_library(lldbPluginArchitectureArm PLUGIN
+  ArchitectureArm.cpp
+
+  LINK_LIBS
+    lldbPluginProcessUtility
+    lldbCore
+    lldbTarget
+  LINK_COMPONENTS
+    Support
+  )
Index: source/Plugins/Architecture/Arm/ArchitectureArm.h
===================================================================
--- /dev/null
+++ source/Plugins/Architecture/Arm/ArchitectureArm.h
@@ -0,0 +1,35 @@
+//===-- ArchitectureArm.h ---------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_PLUGIN_ARCHITECTURE_ARM_H
+#define LLDB_PLUGIN_ARCHITECTURE_ARM_H
+
+#include "lldb/Core/Architecture.h"
+
+namespace lldb_private {
+
+class ArchitectureArm : public Architecture {
+public:
+  static ConstString GetPluginNameStatic();
+  static void Initialize();
+  static void Terminate();
+
+  ConstString GetPluginName() override;
+  uint32_t GetPluginVersion() override;
+
+  void OverrideStopInfo(Thread &thread) override;
+
+private:
+  static std::unique_ptr<Architecture> Create(const ArchSpec &arch);
+  ArchitectureArm() = default;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_PLUGIN_ARCHITECTURE_ARM_H
Index: source/Plugins/Architecture/Arm/ArchitectureArm.cpp
===================================================================
--- /dev/null
+++ source/Plugins/Architecture/Arm/ArchitectureArm.cpp
@@ -0,0 +1,131 @@
+//===-- ArchitectureArm.cpp -------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/Architecture/Arm/ArchitectureArm.h"
+#include "Plugins/Process/Utility/ARMDefines.h"
+#include "Plugins/Process/Utility/InstructionUtils.h"
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/PluginManager.h"
+#include "lldb/Target/RegisterContext.h"
+#include "lldb/Target/Thread.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+ConstString ArchitectureArm::GetPluginNameStatic() {
+  return ConstString("arm");
+}
+
+void ArchitectureArm::Initialize() {
+  PluginManager::RegisterPlugin(GetPluginNameStatic(),
+                                "Arm-specific algorithms",
+                                &ArchitectureArm::Create);
+}
+
+void ArchitectureArm::Terminate() {
+  PluginManager::UnregisterPlugin(&ArchitectureArm::Create);
+}
+
+std::unique_ptr<Architecture> ArchitectureArm::Create(const ArchSpec &arch) {
+  if (arch.GetMachine() != llvm::Triple::arm)
+    return nullptr;
+  return std::unique_ptr<Architecture>(new ArchitectureArm());
+}
+
+ConstString ArchitectureArm::GetPluginName() { return GetPluginNameStatic(); }
+uint32_t ArchitectureArm::GetPluginVersion() { return 1; }
+
+void ArchitectureArm::OverrideStopInfo(Thread &thread) {
+  // We need to check if we are stopped in Thumb mode in a IT instruction
+  // and detect if the condition doesn't pass. If this is the case it means
+  // we won't actually execute this instruction. If this happens we need to
+  // clear the stop reason to no thread plans think we are stopped for a
+  // reason and the plans should keep going.
+  //
+  // We do this because when single stepping many ARM processes, debuggers
+  // often use the BVR/BCR registers that says "stop when the PC is not
+  // equal to its current value". This method of stepping means we can end
+  // up stopping on instructions inside an if/then block that wouldn't get
+  // executed. By fixing this we can stop the debugger from seeming like
+  // you stepped through both the "if" _and_ the "else" clause when source
+  // level stepping because the debugger stops regardless due to the BVR/BCR
+  // triggering a stop.
+  //
+  // It also means we can set breakpoints on instructions inside an an
+  // if/then block and correctly skip them if we use the BKPT instruction.
+  // The ARM and Thumb BKPT instructions are unconditional even when executed
+  // in a Thumb IT block.
+  //
+  // If your debugger inserts software traps in ARM/Thumb code, it will
+  // need to use 16 and 32 bit instruction for 16 and 32 bit thumb
+  // instructions respectively. If your debugger inserts a 16 bit thumb
+  // trap on top of a 32 bit thumb instruction for an opcode that is inside
+  // an if/then, it will change the it/then to conditionally execute your
+  // 16 bit trap and then cause your program to crash if it executes the
+  // trailing 16 bits (the second half of the 32 bit thumb instruction you
+  // partially overwrote).
+
+  RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
+  if (!reg_ctx_sp)
+    return;
+
+  const uint32_t cpsr = reg_ctx_sp->GetFlags(0);
+  if (cpsr == 0)
+    return;
+
+  // Read the J and T bits to get the ISETSTATE
+  const uint32_t J = Bit32(cpsr, 24);
+  const uint32_t T = Bit32(cpsr, 5);
+  const uint32_t ISETSTATE = J << 1 | T;
+  if (ISETSTATE == 0) {
+// NOTE: I am pretty sure we want to enable the code below
+// that detects when we stop on an instruction in ARM mode
+// that is conditional and the condition doesn't pass. This
+// can happen if you set a breakpoint on an instruction that
+// is conditional. We currently will _always_ stop on the
+// instruction which is bad. You can also run into this while
+// single stepping and you could appear to run code in the "if"
+// and in the "else" clause because it would stop at all of the
+// conditional instructions in both.
+// In such cases, we really don't want to stop at this location.
+// I will check with the lldb-dev list first before I enable this.
+#if 0
+    // ARM mode: check for condition on intsruction
+    const addr_t pc = reg_ctx_sp->GetPC();
+    Status error;
+    // If we fail to read the opcode we will get UINT64_MAX as the
+    // result in "opcode" which we can use to detect if we read a
+    // valid opcode.
+    const uint64_t opcode = thread.GetProcess()->ReadUnsignedIntegerFromMemory(pc, 4, UINT64_MAX, error);
+    if (opcode <= UINT32_MAX)
+    {
+        const uint32_t condition = Bits32((uint32_t)opcode, 31, 28);
+        if (!ARMConditionPassed(condition, cpsr))
+        {
+            // We ARE stopped on an ARM instruction whose condition doesn't
+            // pass so this instruction won't get executed.
+            // Regardless of why it stopped, we need to clear the stop info
+            thread.SetStopInfo (StopInfoSP());
+        }
+    }
+#endif
+  } else if (ISETSTATE == 1) {
+    // Thumb mode
+    const uint32_t ITSTATE = Bits32(cpsr, 15, 10) << 2 | Bits32(cpsr, 26, 25);
+    if (ITSTATE != 0) {
+      const uint32_t condition = Bits32(ITSTATE, 7, 4);
+      if (!ARMConditionPassed(condition, cpsr)) {
+        // We ARE stopped in a Thumb IT instruction on an instruction whose
+        // condition doesn't pass so this instruction won't get executed.
+        // Regardless of why it stopped, we need to clear the stop info
+        thread.SetStopInfo(StopInfoSP());
+      }
+    }
+  }
+}
Index: source/Core/PluginManager.cpp
===================================================================
--- source/Core/PluginManager.cpp
+++ source/Core/PluginManager.cpp
@@ -275,6 +275,54 @@
   return nullptr;
 }
 
+#pragma mark Architecture
+
+struct ArchitectureInstance {
+  ConstString name;
+  std::string description;
+  PluginManager::ArchitectureCreateInstance create_callback;
+};
+
+typedef std::vector<ArchitectureInstance> ArchitectureInstances;
+
+static std::mutex g_architecture_mutex;
+
+static ArchitectureInstances &GetArchitectureInstances() {
+  static ArchitectureInstances g_instances;
+  return g_instances;
+}
+
+void PluginManager::RegisterPlugin(const ConstString &name,
+                                   llvm::StringRef description,
+                                   ArchitectureCreateInstance create_callback) {
+  std::lock_guard<std::mutex> guard(g_architecture_mutex);
+  GetArchitectureInstances().push_back({name, description, create_callback});
+}
+
+void PluginManager::UnregisterPlugin(
+    ArchitectureCreateInstance create_callback) {
+  std::lock_guard<std::mutex> guard(g_architecture_mutex);
+  auto &instances = GetArchitectureInstances();
+
+  for (auto pos = instances.begin(), end = instances.end(); pos != end; ++pos) {
+    if (pos->create_callback == create_callback) {
+      instances.erase(pos);
+      return;
+    }
+  }
+  llvm_unreachable("Plugin not found");
+}
+
+std::unique_ptr<Architecture>
+PluginManager::CreateArchitectureInstance(const ArchSpec &arch) {
+  std::lock_guard<std::mutex> guard(g_architecture_mutex);
+  for (const auto &instances : GetArchitectureInstances()) {
+    if (auto plugin_up = instances.create_callback(arch))
+      return plugin_up;
+  }
+  return nullptr;
+}
+
 #pragma mark Disassembler
 
 struct DisassemblerInstance {
Index: source/Core/ArchSpec.cpp
===================================================================
--- source/Core/ArchSpec.cpp
+++ source/Core/ArchSpec.cpp
@@ -11,17 +11,12 @@
 
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/Platform.h"
-#include "lldb/Target/RegisterContext.h"
-#include "lldb/Target/Thread.h"
 #include "lldb/Utility/NameMatches.h"
 #include "lldb/Utility/Stream.h" // for Stream
 #include "lldb/Utility/StringList.h"
 #include "lldb/lldb-defines.h" // for LLDB_INVALID_C...
 #include "lldb/lldb-forward.h" // for RegisterContextSP
 
-#include "Plugins/Process/Utility/ARMDefines.h"
-#include "Plugins/Process/Utility/InstructionUtils.h"
-
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Twine.h" // for Twine
 #include "llvm/BinaryFormat/COFF.h"
@@ -1502,102 +1497,6 @@
   return lhs_core < rhs_core;
 }
 
-static void StopInfoOverrideCallbackTypeARM(lldb_private::Thread &thread) {
-  // We need to check if we are stopped in Thumb mode in a IT instruction
-  // and detect if the condition doesn't pass. If this is the case it means
-  // we won't actually execute this instruction. If this happens we need to
-  // clear the stop reason to no thread plans think we are stopped for a
-  // reason and the plans should keep going.
-  //
-  // We do this because when single stepping many ARM processes, debuggers
-  // often use the BVR/BCR registers that says "stop when the PC is not
-  // equal to its current value". This method of stepping means we can end
-  // up stopping on instructions inside an if/then block that wouldn't get
-  // executed. By fixing this we can stop the debugger from seeming like
-  // you stepped through both the "if" _and_ the "else" clause when source
-  // level stepping because the debugger stops regardless due to the BVR/BCR
-  // triggering a stop.
-  //
-  // It also means we can set breakpoints on instructions inside an an
-  // if/then block and correctly skip them if we use the BKPT instruction.
-  // The ARM and Thumb BKPT instructions are unconditional even when executed
-  // in a Thumb IT block.
-  //
-  // If your debugger inserts software traps in ARM/Thumb code, it will
-  // need to use 16 and 32 bit instruction for 16 and 32 bit thumb
-  // instructions respectively. If your debugger inserts a 16 bit thumb
-  // trap on top of a 32 bit thumb instruction for an opcode that is inside
-  // an if/then, it will change the it/then to conditionally execute your
-  // 16 bit trap and then cause your program to crash if it executes the
-  // trailing 16 bits (the second half of the 32 bit thumb instruction you
-  // partially overwrote).
-
-  RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
-  if (reg_ctx_sp) {
-    const uint32_t cpsr = reg_ctx_sp->GetFlags(0);
-    if (cpsr != 0) {
-      // Read the J and T bits to get the ISETSTATE
-      const uint32_t J = Bit32(cpsr, 24);
-      const uint32_t T = Bit32(cpsr, 5);
-      const uint32_t ISETSTATE = J << 1 | T;
-      if (ISETSTATE == 0) {
-// NOTE: I am pretty sure we want to enable the code below
-// that detects when we stop on an instruction in ARM mode
-// that is conditional and the condition doesn't pass. This
-// can happen if you set a breakpoint on an instruction that
-// is conditional. We currently will _always_ stop on the
-// instruction which is bad. You can also run into this while
-// single stepping and you could appear to run code in the "if"
-// and in the "else" clause because it would stop at all of the
-// conditional instructions in both.
-// In such cases, we really don't want to stop at this location.
-// I will check with the lldb-dev list first before I enable this.
-#if 0
-                // ARM mode: check for condition on intsruction
-                const addr_t pc = reg_ctx_sp->GetPC();
-                Status error;
-                // If we fail to read the opcode we will get UINT64_MAX as the
-                // result in "opcode" which we can use to detect if we read a
-                // valid opcode.
-                const uint64_t opcode = thread.GetProcess()->ReadUnsignedIntegerFromMemory(pc, 4, UINT64_MAX, error);
-                if (opcode <= UINT32_MAX)
-                {
-                    const uint32_t condition = Bits32((uint32_t)opcode, 31, 28);
-                    if (!ARMConditionPassed(condition, cpsr))
-                    {
-                        // We ARE stopped on an ARM instruction whose condition doesn't
-                        // pass so this instruction won't get executed.
-                        // Regardless of why it stopped, we need to clear the stop info
-                        thread.SetStopInfo (StopInfoSP());
-                    }
-                }
-#endif
-      } else if (ISETSTATE == 1) {
-        // Thumb mode
-        const uint32_t ITSTATE =
-            Bits32(cpsr, 15, 10) << 2 | Bits32(cpsr, 26, 25);
-        if (ITSTATE != 0) {
-          const uint32_t condition = Bits32(ITSTATE, 7, 4);
-          if (!ARMConditionPassed(condition, cpsr)) {
-            // We ARE stopped in a Thumb IT instruction on an instruction whose
-            // condition doesn't pass so this instruction won't get executed.
-            // Regardless of why it stopped, we need to clear the stop info
-            thread.SetStopInfo(StopInfoSP());
-          }
-        }
-      }
-    }
-  }
-}
-
-ArchSpec::StopInfoOverrideCallbackType
-ArchSpec::GetStopInfoOverrideCallback() const {
-  const llvm::Triple::ArchType machine = GetMachine();
-  if (machine == llvm::Triple::arm)
-    return StopInfoOverrideCallbackTypeARM;
-  return nullptr;
-}
-
 bool ArchSpec::IsFullySpecifiedTriple() const {
   const auto &user_specified_triple = GetTriple();
 
@@ -1619,7 +1518,7 @@
 
 void ArchSpec::PiecewiseTripleCompare(
     const ArchSpec &other, bool &arch_different, bool &vendor_different,
-    bool &os_different, bool &os_version_different, bool &env_different) {
+    bool &os_different, bool &os_version_different, bool &env_different) const {
   const llvm::Triple &me(GetTriple());
   const llvm::Triple &them(other.GetTriple());
 
Index: source/API/SystemInitializerFull.cpp
===================================================================
--- source/API/SystemInitializerFull.cpp
+++ source/API/SystemInitializerFull.cpp
@@ -42,17 +42,18 @@
 #include "Plugins/ABI/SysV-ppc64/ABISysV_ppc64.h"
 #include "Plugins/ABI/SysV-s390x/ABISysV_s390x.h"
 #include "Plugins/ABI/SysV-x86_64/ABISysV_x86_64.h"
+#include "Plugins/Architecture/Arm/ArchitectureArm.h"
 #include "Plugins/Disassembler/llvm/DisassemblerLLVMC.h"
 #include "Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h"
 #include "Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.h"
 #include "Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h"
 #include "Plugins/DynamicLoader/Static/DynamicLoaderStatic.h"
 #include "Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h"
 #include "Plugins/Instruction/ARM64/EmulateInstructionARM64.h"
 #include "Plugins/InstrumentationRuntime/ASan/ASanRuntime.h"
+#include "Plugins/InstrumentationRuntime/MainThreadChecker/MainThreadCheckerRuntime.h"
 #include "Plugins/InstrumentationRuntime/TSan/TSanRuntime.h"
 #include "Plugins/InstrumentationRuntime/UBSan/UBSanRuntime.h"
-#include "Plugins/InstrumentationRuntime/MainThreadChecker/MainThreadCheckerRuntime.h"
 #include "Plugins/JITLoader/GDB/JITLoaderGDB.h"
 #include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
 #include "Plugins/Language/Go/GoLanguage.h"
@@ -304,6 +305,9 @@
   ABISysV_mips::Initialize();
   ABISysV_mips64::Initialize();
   ABISysV_s390x::Initialize();
+
+  ArchitectureArm::Initialize();
+
   DisassemblerLLVMC::Initialize();
 
   JITLoaderGDB::Initialize();
Index: packages/Python/lldbsuite/test/arm/breakpoint-it/main.c
===================================================================
--- /dev/null
+++ packages/Python/lldbsuite/test/arm/breakpoint-it/main.c
@@ -0,0 +1,14 @@
+int main() {
+  int value;
+  asm (
+      "cmp %1, %2\n\t"
+      "ite ne\n\t"
+      ".thumb_func\n\t"
+      "bkpt_true:\n\t"
+      "movne %0, %1\n\t"
+      ".thumb_func\n\t"
+      "bkpt_false:\n\t"
+      "moveq %0, %2\n\t"
+      : "=r" (value) : "r"(42), "r"(47));
+  return value;
+}
Index: packages/Python/lldbsuite/test/arm/breakpoint-it/TestBreakpointIt.py
===================================================================
--- /dev/null
+++ packages/Python/lldbsuite/test/arm/breakpoint-it/TestBreakpointIt.py
@@ -0,0 +1,45 @@
+"""
+Test that breakpoints in an IT instruction don't fire if their condition is
+false.
+"""
+from __future__ import print_function
+
+
+import lldb
+import os
+import time
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestBreakpointIt(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
+
+    @skipIf(archs=no_match(["arm"]))
+    def test_false(self):
+        self.build()
+        exe = os.path.join(os.getcwd(), "a.out")
+
+        self.runCmd("target create %s" % exe)
+        lldbutil.run_break_set_by_symbol(self, "bkpt_false",
+                extra_options="--skip-prologue 0")
+
+        self.runCmd("run")
+        self.assertEqual(self.process().GetState(), lldb.eStateExited,
+                "Breakpoint does not get hit")
+
+    @skipIf(archs=no_match(["arm"]))
+    def test_true(self):
+        self.build()
+        exe = os.path.join(os.getcwd(), "a.out")
+
+        self.runCmd("target create %s" % exe)
+        bpid = lldbutil.run_break_set_by_symbol(self, "bkpt_true",
+                extra_options="--skip-prologue 0")
+
+        self.runCmd("run")
+        self.assertIsNotNone(lldbutil.get_one_thread_stopped_at_breakpoint_id(
+            self.process(), bpid))
Index: packages/Python/lldbsuite/test/arm/breakpoint-it/Makefile
===================================================================
--- /dev/null
+++ packages/Python/lldbsuite/test/arm/breakpoint-it/Makefile
@@ -0,0 +1,6 @@
+LEVEL = ../../make
+
+C_SOURCES := main.c
+CFLAGS_EXTRAS = -mthumb
+
+include $(LEVEL)/Makefile.rules
Index: include/lldb/Target/Target.h
===================================================================
--- include/lldb/Target/Target.h
+++ include/lldb/Target/Target.h
@@ -23,6 +23,7 @@
 #include "lldb/Breakpoint/BreakpointList.h"
 #include "lldb/Breakpoint/WatchpointList.h"
 #include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/Architecture.h"
 #include "lldb/Core/Broadcaster.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/ModuleList.h"
@@ -881,7 +882,7 @@
   bool
   ModuleIsExcludedForUnconstrainedSearches(const lldb::ModuleSP &module_sp);
 
-  const ArchSpec &GetArchitecture() const { return m_arch; }
+  const ArchSpec &GetArchitecture() const { return m_arch.GetSpec(); }
 
   //------------------------------------------------------------------
   /// Set the architecture for this target.
@@ -912,6 +913,8 @@
 
   bool MergeArchitecture(const ArchSpec &arch_spec);
 
+  Architecture *GetArchitecturePlugin() { return m_arch.GetPlugin(); }
+
   Debugger &GetDebugger() { return m_debugger; }
 
   size_t ReadMemoryFromFileCache(const Address &addr, void *dst, size_t dst_len,
@@ -1205,14 +1208,26 @@
                      const lldb::ModuleSP &new_module_sp) override;
   void WillClearList(const ModuleList &module_list) override;
 
+  class Arch {
+  public:
+    explicit Arch(const ArchSpec &spec);
+    const Arch &operator=(const ArchSpec &spec);
+
+    const ArchSpec &GetSpec() const { return m_spec; }
+    Architecture *GetPlugin() const { return m_plugin_up.get(); }
+
+  private:
+    ArchSpec m_spec;
+    std::unique_ptr<Architecture> m_plugin_up;
+  };
   //------------------------------------------------------------------
   // Member variables.
   //------------------------------------------------------------------
   Debugger &m_debugger;
   lldb::PlatformSP m_platform_sp; ///< The platform for this target.
   std::recursive_mutex m_mutex; ///< An API mutex that is used by the lldb::SB*
                                 /// classes make the SB interface thread safe
-  ArchSpec m_arch;
+  Arch m_arch;
   ModuleList m_images; ///< The list of images for this process (shared
                        /// libraries and anything dynamically loaded).
   SectionLoadHistory m_section_load_history;
Index: include/lldb/Target/Process.h
===================================================================
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -2514,10 +2514,6 @@
 
   OperatingSystem *GetOperatingSystem() { return m_os_ap.get(); }
 
-  ArchSpec::StopInfoOverrideCallbackType GetStopInfoOverrideCallback() const {
-    return m_stop_info_override_callback;
-  }
-
   virtual LanguageRuntime *GetLanguageRuntime(lldb::LanguageType language,
                                               bool retry_if_null = true);
 
@@ -3106,7 +3102,6 @@
   std::vector<PreResumeCallbackAndBaton> m_pre_resume_actions;
   ProcessRunLock m_public_run_lock;
   ProcessRunLock m_private_run_lock;
-  ArchSpec::StopInfoOverrideCallbackType m_stop_info_override_callback;
   bool m_currently_handling_do_on_removals;
   bool m_resume_requested; // If m_currently_handling_event or
                            // m_currently_handling_do_on_removals are true,
Index: include/lldb/Core/PluginManager.h
===================================================================
--- include/lldb/Core/PluginManager.h
+++ include/lldb/Core/PluginManager.h
@@ -10,6 +10,7 @@
 #ifndef liblldb_PluginManager_h_
 #define liblldb_PluginManager_h_
 
+#include "lldb/Core/Architecture.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Status.h"          // for Status
 #include "lldb/lldb-enumerations.h"       // for ScriptLanguage
@@ -53,6 +54,21 @@
   static ABICreateInstance
   GetABICreateCallbackForPluginName(const ConstString &name);
 
+  //------------------------------------------------------------------
+  // Architecture
+  //------------------------------------------------------------------
+  using ArchitectureCreateInstance =
+      std::unique_ptr<Architecture> (*)(const ArchSpec &);
+
+  static void RegisterPlugin(const ConstString &name,
+                             llvm::StringRef description,
+                             ArchitectureCreateInstance create_callback);
+
+  static void UnregisterPlugin(ArchitectureCreateInstance create_callback);
+
+  static std::unique_ptr<Architecture>
+  CreateArchitectureInstance(const ArchSpec &arch);
+
   //------------------------------------------------------------------
   // Disassembler
   //------------------------------------------------------------------
Index: include/lldb/Core/Architecture.h
===================================================================
--- /dev/null
+++ include/lldb/Core/Architecture.h
@@ -0,0 +1,43 @@
+//===-- Architecture.h ------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_CORE_ARCHITECTURE_H
+#define LLDB_CORE_ARCHITECTURE_H
+
+#include "lldb/Core/PluginInterface.h"
+
+namespace lldb_private {
+
+class Architecture : public PluginInterface {
+public:
+  Architecture() = default;
+  virtual ~Architecture() = default;
+
+  //------------------------------------------------------------------
+  /// This is currently intended to handle cases where a
+  /// program stops at an instruction that won't get executed and it
+  /// allows the stop reasonm, like "breakpoint hit", to be replaced
+  /// with a different stop reason like "no stop reason".
+  ///
+  /// This is specifically used for ARM in Thumb code when we stop in
+  /// an IT instruction (if/then/else) where the instruction won't get
+  /// executed and therefore it wouldn't be correct to show the program
+  /// stopped at the current PC. The code is generic and applies to all
+  /// ARM CPUs.
+  //------------------------------------------------------------------
+  virtual void OverrideStopInfo(Thread &thread) = 0;
+
+private:
+  Architecture(const Architecture &) = delete;
+  void operator=(const Architecture &) = delete;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_CORE_ARCHITECTURE_H
Index: include/lldb/Core/ArchSpec.h
===================================================================
--- include/lldb/Core/ArchSpec.h
+++ include/lldb/Core/ArchSpec.h
@@ -15,27 +15,15 @@
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-private-enumerations.h"
+#include "lldb/lldb-forward.h"
 #include "llvm/ADT/StringRef.h" // for StringRef
 #include "llvm/ADT/Triple.h"
 
 #include <string> // for string
 
 #include <stddef.h> // for size_t
 #include <stdint.h> // for uint32_t
 
-namespace lldb_private {
-class Platform;
-}
-namespace lldb_private {
-class Stream;
-}
-namespace lldb_private {
-class StringList;
-}
-namespace lldb_private {
-class Thread;
-}
-
 namespace lldb_private {
 
 //----------------------------------------------------------------------
@@ -257,8 +245,6 @@
 
   };
 
-  typedef void (*StopInfoOverrideCallbackType)(lldb_private::Thread &thread);
-
   //------------------------------------------------------------------
   /// Default constructor.
   ///
@@ -573,34 +559,11 @@
   //------------------------------------------------------------------
   bool IsCompatibleMatch(const ArchSpec &rhs) const;
 
-  //------------------------------------------------------------------
-  /// Get a stop info override callback for the current architecture.
-  ///
-  /// Most platform specific code should go in lldb_private::Platform,
-  /// but there are cases where no matter which platform you are on
-  /// certain things hold true.
-  ///
-  /// This callback is currently intended to handle cases where a
-  /// program stops at an instruction that won't get executed and it
-  /// allows the stop reasonm, like "breakpoint hit", to be replaced
-  /// with a different stop reason like "no stop reason".
-  ///
-  /// This is specifically used for ARM in Thumb code when we stop in
-  /// an IT instruction (if/then/else) where the instruction won't get
-  /// executed and therefore it wouldn't be correct to show the program
-  /// stopped at the current PC. The code is generic and applies to all
-  /// ARM CPUs.
-  ///
-  /// @return NULL or a valid stop info override callback for the
-  ///     current architecture.
-  //------------------------------------------------------------------
-  StopInfoOverrideCallbackType GetStopInfoOverrideCallback() const;
-
   bool IsFullySpecifiedTriple() const;
 
   void PiecewiseTripleCompare(const ArchSpec &other, bool &arch_different,
                               bool &vendor_different, bool &os_different,
-                              bool &os_version_different, bool &env_different);
+                              bool &os_version_different, bool &env_different) const;
 
   //------------------------------------------------------------------
   /// Detect whether this architecture uses thumb code exclusively
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to