Pull GetSoftwareBreakpointPCOffset into base class

[labath]

Summary

This function encodes the knowledge of whether the PC points to the
breakpoint instruction of the one following it after the breakpoint is
"hit". This behavior mainly(*) depends on the architecture and not on the
OS, so it makes sense for it to be implemented in the base class, where
it can be shared between different implementations (Linux and NetBSD
atm).

(*) It is possible for an OS to expose a different API, perhaps by doing
some fixups in the kernel. In this case, the implementation can override
this function to implement custom behavior.

[krytarowski]

I was wondering whether we want to normalize this inside the kernel and always advance the Program Counter.. but it's easier to manage it in userland.

[labath]

In D52532#1246173, @krytarowski wrote:

I was wondering whether we want to normalize this inside the kernel and always advance the Program Counter.. but it's easier to manage it in userland.

I am generally in favour of keeping the kernel simple, particularly when ptrace is concerned. However, there is one difference in behaviour that annoys me. Right now, if an application itself inserts a trap into it's source code, on intel it will be easy to resume it from debugger simply by continuing. OTOH, on arm, the user will have to manually update the PC first. So I am not sure which is better here...

[krytarowski]

In D52532#1250404, @labath wrote:

In D52532#1246173, @krytarowski wrote:

I was wondering whether we want to normalize this inside the kernel and always advance the Program Counter.. but it's easier to manage it in userland.

I am generally in favour of keeping the kernel simple, particularly when ptrace is concerned. However, there is one difference in behaviour that annoys me. Right now, if an application itself inserts a trap into it's source code, on intel it will be easy to resume it from debugger simply by continuing. OTOH, on arm, the user will have to manually update the PC first. So I am not sure which is better here...

Right now I defer this into future. We still need to fixup PC in kernel debuggers... on the other hand in order to advance the register manually on an arbitrary ISA we might need to guess instruction size. It's better to guess it in userland.

lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h

protected:
   // ----------------------------------------------------------- Internal
   // interface for software breakpoints
   // -----------------------------------------------------------
   Status SetSoftwareBreakpoint(lldb::addr_t addr, uint32_t size_hint);
 
   virtual llvm::Expected<llvm::ArrayRef<uint8_t>>
   GetSoftwareBreakpointTrapOpcode(size_t size_hint);
 
+  /// Return the offset of the PC relative to the software breakpoint that was hit. If an
+  /// architecture (e.g. arm) reports breakpoint hits before incrementing the PC, this offset
+  /// will be 0. If an architecture (e.g. intel) reports breakpoints hits after incrementing the
+  /// PC, this offset will be the size of the breakpoint opcode.
+  virtual size_t GetSoftwareBreakpointPCOffset();
+
   // -----------------------------------------------------------
   /// Notify the delegate that an exec occurred.
   ///
   /// Provide a mechanism for a delegate to clear out any exec-
   /// sensitive data.
   // -----------------------------------------------------------
   void NotifyDidExec();
 
   NativeThreadProtocol *GetThreadByIDUnlocked(lldb::tid_t tid);
 
 private:
   void SynchronouslyNotifyProcessStateChanged(lldb::StateType state);
 };
 } // namespace lldb_private
 
 #endif // #ifndef liblldb_NativeProcessProtocol_h_

lldb/trunk/source/Host/common/NativeProcessProtocol.cpp

case llvm::Triple::ppc64le:
     return llvm::makeArrayRef(g_ppc64le_opcode);
 
   default:
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "CPU type not supported!");
   }
 }
 
+size_t NativeProcessProtocol::GetSoftwareBreakpointPCOffset() {
+  switch (GetArchitecture().GetMachine()) {
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+  case llvm::Triple::systemz:
+    // These architectures report increment the PC after breakpoint is hit.
+    return cantFail(GetSoftwareBreakpointTrapOpcode(0)).size();
+
+  case llvm::Triple::arm:
+  case llvm::Triple::aarch64:
+  case llvm::Triple::mips64:
+  case llvm::Triple::mips64el:
+  case llvm::Triple::mips:
+  case llvm::Triple::mipsel:
+  case llvm::Triple::ppc64le:
+    // On these architectures the PC doesn't get updated for breakpoint hits.
+    return 0;
+
+  default:
+    llvm_unreachable("CPU type not supported!");
+  }
+}
+
 Status NativeProcessProtocol::RemoveBreakpoint(lldb::addr_t addr,
                                                bool hardware) {
   if (hardware)
     return RemoveHardwareBreakpoint(addr);
   else
     return m_breakpoint_list.DecRef(addr);
 }
 

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h

private:
   Status SetupSoftwareSingleStepping(NativeThreadLinux &thread);
 
   bool HasThreadNoLock(lldb::tid_t thread_id);
 
   bool StopTrackingThread(lldb::tid_t thread_id);
 
   NativeThreadLinux &AddThread(lldb::tid_t thread_id);
 
-  Status GetSoftwareBreakpointPCOffset(uint32_t &actual_opcode_size);
-
   Status FixupBreakpointPCAsNeeded(NativeThreadLinux &thread);
 
   /// Writes a siginfo_t structure corresponding to the given thread ID to the
   /// memory region pointed to by @p siginfo.
   Status GetSignalInfo(lldb::tid_t tid, void *siginfo);
 
   /// Writes the raw event message code (vis-a-vis PTRACE_GETEVENTMSG)
   /// corresponding to the given thread ID to the memory pointed to by @p

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp

 
 size_t NativeProcessLinux::UpdateThreads() {
   // The NativeProcessLinux monitoring threads are always up to date with
   // respect to thread state and they keep the thread list populated properly.
   // All this method needs to do is return the thread count.
   return m_threads.size();
 }
 
-Status NativeProcessLinux::GetSoftwareBreakpointPCOffset(
-    uint32_t &actual_opcode_size) {
-  // FIXME put this behind a breakpoint protocol class that can be
-  // set per architecture.  Need ARM, MIPS support here.
-  static const uint8_t g_i386_opcode[] = {0xCC};
-  static const uint8_t g_s390x_opcode[] = {0x00, 0x01};
-
-  switch (m_arch.GetMachine()) {
-  case llvm::Triple::x86:
-  case llvm::Triple::x86_64:
-    actual_opcode_size = static_cast<uint32_t>(sizeof(g_i386_opcode));
-    return Status();
-
-  case llvm::Triple::systemz:
-    actual_opcode_size = static_cast<uint32_t>(sizeof(g_s390x_opcode));
-    return Status();
-
-  case llvm::Triple::arm:
-  case llvm::Triple::aarch64:
-  case llvm::Triple::mips64:
-  case llvm::Triple::mips64el:
-  case llvm::Triple::mips:
-  case llvm::Triple::mipsel:
-  case llvm::Triple::ppc64le:
-    // On these architectures the PC don't get updated for breakpoint hits
-    actual_opcode_size = 0;
-    return Status();
-
-  default:
-    assert(false && "CPU type not supported!");
-    return Status("CPU type not supported");
-  }
-}
-
 Status NativeProcessLinux::SetBreakpoint(lldb::addr_t addr, uint32_t size,
                                          bool hardware) {
   if (hardware)
     return SetHardwareBreakpoint(addr, size);
   else
     return SetSoftwareBreakpoint(addr, size);
 }
 
NativeProcessLinux::FixupBreakpointPCAsNeeded(NativeThreadLinux &thread) {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_BREAKPOINTS));
 
   Status error;
 
   // Find out the size of a breakpoint (might depend on where we are in the
   // code).
   NativeRegisterContext &context = thread.GetRegisterContext();
 
-  uint32_t breakpoint_size = 0;
-  error = GetSoftwareBreakpointPCOffset(breakpoint_size);
-  if (error.Fail()) {
-    LLDB_LOG(log, "GetBreakpointSize() failed: {0}", error);
-    return error;
-  } else
-    LLDB_LOG(log, "breakpoint size: {0}", breakpoint_size);
+  uint32_t breakpoint_size = GetSoftwareBreakpointPCOffset();
 
   // First try probing for a breakpoint at a software breakpoint location: PC -
   // breakpoint size.
   const lldb::addr_t initial_pc_addr = context.GetPCfromBreakpointLocation();
   lldb::addr_t breakpoint_addr = initial_pc_addr;
   if (breakpoint_size > 0) {
     // Do not allow breakpoint probe to wrap around.
     if (breakpoint_addr >= breakpoint_size)

lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp

Status NativeProcessNetBSD::PtraceWrapper(int req, lldb::pid_t pid, void *addr,
   LLDB_LOG(log, "ptrace({0}, {1}, {2}, {3})={4:x}", req, pid, addr, data, ret);
 
   if (error.Fail())
     LLDB_LOG(log, "ptrace() failed: {0}", error);
 
   return error;
 }
 
-Status NativeProcessNetBSD::GetSoftwareBreakpointPCOffset(
-    uint32_t &actual_opcode_size) {
-  // FIXME put this behind a breakpoint protocol class that can be
-  // set per architecture.  Need ARM, MIPS support here.
-  static const uint8_t g_i386_opcode[] = {0xCC};
-  switch (m_arch.GetMachine()) {
-  case llvm::Triple::x86_64:
-    actual_opcode_size = static_cast<uint32_t>(sizeof(g_i386_opcode));
-    return Status();
-  default:
-    assert(false && "CPU type not supported!");
-    return Status("CPU type not supported");
-  }
-}
-
 Status
 NativeProcessNetBSD::FixupBreakpointPCAsNeeded(NativeThreadNetBSD &thread) {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_BREAKPOINTS));
   Status error;
   // Find out the size of a breakpoint (might depend on where we are in the
   // code).
   NativeRegisterContext& context = thread.GetRegisterContext();
-  uint32_t breakpoint_size = 0;
-  error = GetSoftwareBreakpointPCOffset(breakpoint_size);
-  if (error.Fail()) {
-    LLDB_LOG(log, "GetBreakpointSize() failed: {0}", error);
-    return error;
-  } else
-    LLDB_LOG(log, "breakpoint size: {0}", breakpoint_size);
+  uint32_t breakpoint_size = GetSoftwareBreakpointPCOffset();
+
   // First try probing for a breakpoint at a software breakpoint location: PC -
   // breakpoint size.
   const lldb::addr_t initial_pc_addr =
       context.GetPCfromBreakpointLocation();
   lldb::addr_t breakpoint_addr = initial_pc_addr;
   if (breakpoint_size > 0) {
     // Do not allow breakpoint probe to wrap around.
     if (breakpoint_addr >= breakpoint_size)