zturner created this revision.

The only real consumer of this is the `Process` class, so it makes sense (both 
conceptually and from a layering standpoint) to hide this in the Process plugin.

This is intended to be NFC.


https://reviews.llvm.org/D31172

Files:
  lldb/include/lldb/Core/ArchSpec.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Core/ArchSpec.cpp
  lldb/source/Interpreter/CommandObject.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/Thread.cpp

Index: lldb/source/Target/Thread.cpp
===================================================================
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -441,10 +441,7 @@
     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);
+        GetProcess()->InvokeStopInfoOverrideCallback(*this);
       }
     }
   }
Index: lldb/source/Target/Process.cpp
===================================================================
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -17,7 +17,9 @@
 #include "llvm/Support/Threading.h"
 
 // Project includes
+#include "Plugins/Process/Utility/ARMDefines.h"
 #include "Plugins/Process/Utility/InferiorCallPOSIX.h"
+#include "Plugins/Process/Utility/InstructionUtils.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
@@ -2397,6 +2399,104 @@
   return bytes_written;
 }
 
+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();
+        Error 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());
+          }
+        }
+      }
+    }
+  }
+}
+
+void Process::SetStopInfoOverrideCallback(const ArchSpec &Arch) {
+  if (Arch.GetMachine() == llvm::Triple::arm)
+    m_stop_info_override_callback = StopInfoOverrideCallbackTypeARM;
+}
+
+void Process::InvokeStopInfoOverrideCallback(lldb_private::Thread &Thread) {
+  if (m_stop_info_override_callback)
+    m_stop_info_override_callback(Thread);
+}
+
 size_t Process::WriteMemory(addr_t addr, const void *buf, size_t size,
                             Error &error) {
 #if defined(ENABLE_MEMORY_CACHING)
@@ -2797,8 +2897,7 @@
             else
               StartPrivateStateThread();
 
-            m_stop_info_override_callback =
-                GetTarget().GetArchitecture().GetStopInfoOverrideCallback();
+            SetStopInfoOverrideCallback(GetTarget().GetArchitecture());
 
             // Target was stopped at entry as was intended. Need to notify the
             // listeners
@@ -3217,7 +3316,7 @@
     }
   }
 
-  m_stop_info_override_callback = process_arch.GetStopInfoOverrideCallback();
+  SetStopInfoOverrideCallback(process_arch);
 }
 
 Error Process::ConnectRemote(Stream *strm, llvm::StringRef remote_url) {
Index: lldb/source/Interpreter/CommandObject.cpp
===================================================================
--- lldb/source/Interpreter/CommandObject.cpp
+++ lldb/source/Interpreter/CommandObject.cpp
@@ -33,6 +33,9 @@
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -1017,15 +1020,19 @@
   return handled;
 }
 
+static std::string computeArchHelp() {
+  llvm::StringSet<> Archs;
+  ArchSpec::AutoComplete("", Archs);
+  std::string Result;
+  llvm::raw_string_ostream OS(Result);
+  OS << "These are the supported architecture names:\n";
+  OS << llvm::join_range(Archs.keys(), "\n");
+  return OS.str();
+}
+
 static llvm::StringRef arch_helper() {
-  static StreamString g_archs_help;
-  if (g_archs_help.Empty()) {
-    StringList archs;
-    ArchSpec::AutoComplete(llvm::StringRef(), archs);
-    g_archs_help.Printf("These are the supported architecture names:\n");
-    archs.Join("\n", g_archs_help);
-  }
-  return g_archs_help.GetString();
+  static std::string Help = computeArchHelp();
+  return Help;
 }
 
 CommandObject::ArgumentTableEntry CommandObject::g_arguments_data[] = {
Index: lldb/source/Core/ArchSpec.cpp
===================================================================
--- lldb/source/Core/ArchSpec.cpp
+++ lldb/source/Core/ArchSpec.cpp
@@ -22,14 +22,8 @@
 #include "llvm/Support/Host.h"
 
 // Project includes
-#include "Plugins/Process/Utility/ARMDefines.h"
-#include "Plugins/Process/Utility/InstructionUtils.h"
-#include "lldb/Core/StringList.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/Platform.h"
-#include "lldb/Target/Process.h"
-#include "lldb/Target/RegisterContext.h"
-#include "lldb/Target/Thread.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/NameMatches.h"
 #include "lldb/Utility/RegularExpression.h"
@@ -256,17 +250,18 @@
   const char *name;
 };
 
-size_t ArchSpec::AutoComplete(llvm::StringRef name, StringList &matches) {
+size_t ArchSpec::AutoComplete(llvm::StringRef name,
+                              llvm::StringSet<> &matches) {
   if (!name.empty()) {
     for (uint32_t i = 0; i < llvm::array_lengthof(g_core_definitions); ++i) {
       if (NameMatches(g_core_definitions[i].name, NameMatch::StartsWith, name))
-        matches.AppendString(g_core_definitions[i].name);
+        matches.insert(g_core_definitions[i].name);
     }
   } else {
     for (uint32_t i = 0; i < llvm::array_lengthof(g_core_definitions); ++i)
-      matches.AppendString(g_core_definitions[i].name);
+      matches.insert(g_core_definitions[i].name);
   }
-  return matches.GetSize();
+  return matches.size();
 }
 
 #define CPU_ANY (UINT32_MAX)
@@ -1499,102 +1494,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();
-                Error 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();
 
Index: lldb/include/lldb/Target/Process.h
===================================================================
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -2512,9 +2512,29 @@
 
   OperatingSystem *GetOperatingSystem() { return m_os_ap.get(); }
 
-  ArchSpec::StopInfoOverrideCallbackType GetStopInfoOverrideCallback() const {
-    return m_stop_info_override_callback;
-  }
+  //------------------------------------------------------------------
+  /// 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.
+  //------------------------------------------------------------------
+  typedef void (*StopInfoOverrideCallbackType)(lldb_private::Thread &);
+  void InvokeStopInfoOverrideCallback(lldb_private::Thread &Thread);
 
   virtual LanguageRuntime *GetLanguageRuntime(lldb::LanguageType language,
                                               bool retry_if_null = true);
@@ -3036,7 +3056,7 @@
   std::vector<PreResumeCallbackAndBaton> m_pre_resume_actions;
   ProcessRunLock m_public_run_lock;
   ProcessRunLock m_private_run_lock;
-  ArchSpec::StopInfoOverrideCallbackType m_stop_info_override_callback;
+  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,
@@ -3081,6 +3101,8 @@
   void ResumePrivateStateThread();
 
 private:
+  void SetStopInfoOverrideCallback(const ArchSpec &Arch);
+
   struct PrivateStateThreadArgs {
     PrivateStateThreadArgs(Process *p, bool s)
         : process(p), is_secondary_thread(s){};
Index: lldb/include/lldb/Core/ArchSpec.h
===================================================================
--- lldb/include/lldb/Core/ArchSpec.h
+++ lldb/include/lldb/Core/ArchSpec.h
@@ -14,6 +14,7 @@
 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-forward.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Triple.h"
 
 namespace lldb_private {
@@ -239,8 +240,6 @@
 
   };
 
-  typedef void (*StopInfoOverrideCallbackType)(lldb_private::Thread &thread);
-
   //------------------------------------------------------------------
   /// Default constructor.
   ///
@@ -283,7 +282,7 @@
   //------------------------------------------------------------------
   const ArchSpec &operator=(const ArchSpec &rhs);
 
-  static size_t AutoComplete(llvm::StringRef name, StringList &matches);
+  static size_t AutoComplete(llvm::StringRef name, llvm::StringSet<> &matches);
 
   //------------------------------------------------------------------
   /// Returns a static string representing the current architecture.
@@ -555,29 +554,6 @@
   //------------------------------------------------------------------
   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,
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to