[lldb] [Windows] Fix continuing from breakpoints and singlestepping on ARM/AArch64

[mstorsjo]

Summary

Based on suggestions by Eric Youngdale.

This fixes https://llvm.org/PR51673.

[DavidSpickett]

https://docs.microsoft.com/en-us/cpp/intrinsics/debugbreak?view=msvc-160 confirms the AArch64 breakpoint instruction, were you able to find a source for the Arm/Thumb one?

Otherwise the logic seems fine. It would be good to dedupe these switches, in a file in Plugins/Process/Windows/Common/?
Or move some of this into the register context? Assuming you have access to a register context in all of these places. It's a bit weird to put it there since the only register related thing is the single step bit but that's the only per architecture class at the moment.

(also all the Windows code being in Windows/Common is odd but ok not a thing for now)

[mstorsjo]

In D109777#3001429, @DavidSpickett wrote:

https://docs.microsoft.com/en-us/cpp/intrinsics/debugbreak?view=msvc-160 confirms the AArch64 breakpoint instruction, were you able to find a source for the Arm/Thumb one?

I don't have a formal source for that one, but compiling a __debugbreak() with MSVC produces that opcode. As Windows is thumb-only, we can always do a breakpoint with that single opcode (even if the targeted instruction happens to be a wide thumb2 instruction) without needing to detect betwen arm/thumb mode.

Otherwise the logic seems fine. It would be good to dedupe these switches, in a file in Plugins/Process/Windows/Common/?
Or move some of this into the register context? Assuming you have access to a register context in all of these places. It's a bit weird to put it there since the only register related thing is the single step bit but that's the only per architecture class at the moment.

You mean the duplication between the "native" and regular versions of the classes - or the arch specific switches in otherwise seemingly arch-independent code?

(In case it's not familiar to all - the former duplication is because lldb can operate in two modes - either with lldb directly controlling the debugged process (which is the case without the "native" prefix) or with lldb spawning lldb-server which then owns the debugged process (the classes with the "native" prefix).)

For the former, I'm not quite familiar enough the how the architecture of these things work in practice, but I think those two class hiearchies/families don't have much in common, so there's no immediate common place to put shared code between the two.

For the latter - I guess it'd maybe be possible to add some extra locally defined (not overriding a virtual method from the base classes) methods in the register contexts that return the breakpoint opcodes, their sizes and the right flag bits...

(also all the Windows code being in Windows/Common is odd but ok not a thing for now)

Yeah that's a bit odd indeed. There's also a few other oddities in the lldb tree, like some directiories being capitalized while others aren't, I guess it's one of the many cases where it's just not worth the churn to change it.

[DavidSpickett]

I don't have a formal source for that one, but compiling a __debugbreak() with MSVC produces that opcode. As Windows is thumb-only, we can always do a breakpoint with that single opcode (even if the targeted instruction happens to be a wide thumb2 instruction) without needing to detect betwen arm/thumb mode.

Great. (I had forgotten about being Thumb only in fact)

You mean the duplication between the "native" and regular versions of the classes - or the arch specific switches in otherwise seemingly arch-independent code?

The first one. The switches follow the style of the generic methods so I'm cool with that.

For the former, I'm not quite familiar enough the how the architecture of these things work in practice, but I think those two class hiearchies/families don't have much in common, so there's no immediate common place to put shared code between the two.

Looking at the cmake it builds lldbPluginProcessWindowsCommon which includes native and process files so I think that means you can add a file there and both modes can use it, I think. BreakpointInfo.cpp ?

For the latter - I guess it'd maybe be possible to add some extra locally defined (not overriding a virtual method from the base classes) methods in the register contexts that return the breakpoint opcodes, their sizes and the right flag bits...

Now you mention the two modes, because each mode has its own register context class if you put the breakpoint info there it'll also be duplicated. Unless you add an intermediate RegisterContextWindowsBaseArm in the middle but that's blurring the register context's purpose further.

If the breakpoint info can go in a file that both modes can access that would be great, otherwise it's fine as is. Not likely to change this in a hurry anyway.

[labath]

The real issue here is that there are two implementations in the first place. The lldb-server method is supposed to be the new thing, and the old should go away as soon as the new one is good enough. Therefore I wouldn't spend too much time trying to unify these. Rather, if anyone has the time, I'd recommend seeing what features does the lldb-server thing lack so it can be made the default one.

lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp

501–504

This should be the job of GetSoftwareBreakpointPCOffset, although the default implementation of that function returns zero for arm. If that is not desired (does windows automatically increment the PC or something?), then I guess you should override that function.

[DavidSpickett]

One thing that I forgot to ask, are there already tests that cover the scenario the bug reported?

Good chance there are but they only run on x86 due to the available bots. We (Linaro) are working to get more tests running on WoA bots so this would be covered eventually.

[mstorsjo]

In D109777#3003312, @DavidSpickett wrote:

One thing that I forgot to ask, are there already tests that cover the scenario the bug reported?

No idea - this fixes a sequence of b <somesymbol>, run, step, i.e. fairly trivial/basic breakpoint functionality. (It fixes it entirely on ARM64; on ARM there's other issues due to the thumb bit offset that I haven't had time to fix, but the aspects that this patch touches should be correct for ARM at least.)

Good chance there are but they only run on x86 due to the available bots. We (Linaro) are working to get more tests running on WoA bots so this would be covered eventually.

That'd be great. I haven't ever run the LLDB testsuite properly on Windows on ARM - my only WoA hardware is in the absolute lowest end (Snapdragon 835) so I only cross compile things and test on the target device manually - doing cross testing of LLDB is tricky to say the least, and I wouldn't want to try to actually build LLDB and run the testsuite natively on that hardware...

[mstorsjo]

lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp

501–504

Thanks for the pointer - I checked and it does indeed seem to be the case that Windows stops with the PC register incremented when hitting breakpoints, so it's straightforward to override GetSoftwareBreakpointPCOffset here and use that.

Unfortunately, for the case in ProcessWindows.cpp, it doesn't seem to be as straightforward to use that (or anything else based on GetSoftwareBreakpointTrapOpcode) as the Process/Platform versions of GetSoftwareBreakpointTrapOpcode also need a BreakpointSite where the opcode actually is set.

[mstorsjo]

Refactored to override and use GetSoftwareBreakpointPCOffset in NativeProcessWindows.cpp. Not factorized the implementations to share code, if one of the implementations is bound to be removed at some point.

[mstorsjo]

@labath Does this seem ok to you now, and/or do you have any refactoring suggestion regarding getting the breakpoint opcode size in ProcessWindows.cpp?

[labath]

Sorry about the delay. I think this is fine, though I would like to encourage (*wink, wing, nudge, nudge*) someone to finish the lldb-server migration. We're in a pretty bad situation, when it comes to windows right now, as we have two process plugin implementations *and* two pdb readers, and both (all four) of them are an active source of pain (see D109834 for the other one).

[mstorsjo]

In D109777#3014644, @labath wrote:

Sorry about the delay. I think this is fine, though I would like to encourage (*wink, wing, nudge, nudge*) someone to finish the lldb-server migration. We're in a pretty bad situation, when it comes to windows right now, as we have two process plugin implementations *and* two pdb readers, and both (all four) of them are an active source of pain (see D109834 for the other one).

Yup, I clearly agree on that. Unfortunately I don't think I can put up the overall architecting work on that right now, to look through the big picture and see what's missing and what needs to be taken care of, to move forward with dropping the old one.

lldb/source/Plugins/Platform/Windows/PlatformWindows.h

public:
   void GetStatus(lldb_private::Stream &strm) override;
 
   bool CanDebugProcess() override;
 
   // FIXME not sure what the _sigtramp equivalent would be on this platform
   void CalculateTrapHandlerSymbolNames() override {}
 
   ConstString GetFullNameForDylib(ConstString basename) override;
+
+  size_t GetSoftwareBreakpointTrapOpcode(Target &target,
+                                         BreakpointSite *bp_site) override;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PLATFORM_WINDOWS_PLATFORMWINDOWS_H

lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp

 ConstString PlatformWindows::GetFullNameForDylib(ConstString basename) {
   if (basename.IsEmpty())
     return basename;
 
   StreamString stream;
   stream.Printf("%s.dll", basename.GetCString());
   return ConstString(stream.GetString());
 }
+
+size_t
+PlatformWindows::GetSoftwareBreakpointTrapOpcode(Target &target,
+                                                 BreakpointSite *bp_site) {
+  ArchSpec arch = target.GetArchitecture();
+  assert(arch.IsValid());
+  const uint8_t *trap_opcode = nullptr;
+  size_t trap_opcode_size = 0;
+
+  switch (arch.GetMachine()) {
+  case llvm::Triple::aarch64: {
+    static const uint8_t g_aarch64_opcode[] = {0x00, 0x00, 0x3e, 0xd4}; // brk #0xf000
+    trap_opcode = g_aarch64_opcode;
+    trap_opcode_size = sizeof(g_aarch64_opcode);
+
+    if (bp_site->SetTrapOpcode(trap_opcode, trap_opcode_size))
+      return trap_opcode_size;
+    return 0;
+  } break;
+
+  case llvm::Triple::arm:
+  case llvm::Triple::thumb: {
+    static const uint8_t g_thumb_opcode[] = {0xfe, 0xde}; // udf #0xfe
+    trap_opcode = g_thumb_opcode;
+    trap_opcode_size = sizeof(g_thumb_opcode);
+
+    if (bp_site->SetTrapOpcode(trap_opcode, trap_opcode_size))
+      return trap_opcode_size;
+    return 0;
+  } break;
+
+  default:
+    return Platform::GetSoftwareBreakpointTrapOpcode(target, bp_site);
+  }
+}

lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h

public:
   void OnExitThread(lldb::tid_t thread_id, uint32_t exit_code) override;
   void OnLoadDll(const ModuleSpec &module_spec,
                  lldb::addr_t module_addr) override;
   void OnUnloadDll(lldb::addr_t module_addr) override;
 
 protected:
   NativeThreadWindows *GetThreadByID(lldb::tid_t thread_id);
 
+  llvm::Expected<llvm::ArrayRef<uint8_t>>
+  GetSoftwareBreakpointTrapOpcode(size_t size_hint) override;
+
+  size_t GetSoftwareBreakpointPCOffset() override;
+
   bool FindSoftwareBreakpoint(lldb::addr_t addr);
 
   void StopThread(lldb::tid_t thread_id, lldb::StopReason reason,
                   std::string description = "");
 
   void SetStopReasonForThread(NativeThreadWindows &thread,
                               lldb::StopReason reason,
                               std::string description = "");

lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp

 size_t NativeProcessWindows::UpdateThreads() { return m_threads.size(); }
 
 llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
 NativeProcessWindows::GetAuxvData() const {
   // Not available on this target.
   return llvm::errc::not_supported;
 }
 
+llvm::Expected<llvm::ArrayRef<uint8_t>>
+NativeProcessWindows::GetSoftwareBreakpointTrapOpcode(size_t size_hint) {
+  static const uint8_t g_aarch64_opcode[] = {0x00, 0x00, 0x3e, 0xd4}; // brk #0xf000
+  static const uint8_t g_thumb_opcode[] = {0xfe, 0xde}; // udf #0xfe
+
+  switch (GetArchitecture().GetMachine()) {
+  case llvm::Triple::aarch64:
+    return llvm::makeArrayRef(g_aarch64_opcode);
+
+  case llvm::Triple::arm:
+  case llvm::Triple::thumb:
+    return llvm::makeArrayRef(g_thumb_opcode);
+
+  default:
+    return NativeProcessProtocol::GetSoftwareBreakpointTrapOpcode(size_hint);
+  }
+}
+
+size_t NativeProcessWindows::GetSoftwareBreakpointPCOffset() {
+    // Windows always reports an incremented PC after a breakpoint is hit,
+    // even on ARM.
+    return cantFail(GetSoftwareBreakpointTrapOpcode(0)).size();
+}
+
 bool NativeProcessWindows::FindSoftwareBreakpoint(lldb::addr_t addr) {
   auto it = m_software_breakpoints.find(addr);
   if (it == m_software_breakpoints.end())
     return false;
   return true;
 }
 
 Status NativeProcessWindows::SetBreakpoint(lldb::addr_t addr, uint32_t size,
if (FindSoftwareBreakpoint(record.GetExceptionAddress())) {
       LLDB_LOG(log, "Hit non-loader breakpoint at address {0:x}.",
                record.GetExceptionAddress());
 
       StopThread(record.GetThreadID(), StopReason::eStopReasonBreakpoint);
 
       if (NativeThreadWindows *stop_thread =
               GetThreadByID(record.GetThreadID())) {
         auto &register_context = stop_thread->GetRegisterContext();
-        // The current EIP is AFTER the BP opcode, which is one byte '0xCC'
-        uint64_t pc = register_context.GetPC() - 1;
+        uint32_t breakpoint_size = GetSoftwareBreakpointPCOffset();
+        // The current PC is AFTER the BP opcode, on all architectures.
+        uint64_t pc = register_context.GetPC() - breakpoint_size;
         register_context.SetPC(pc);
       }
 
       SetState(eStateStopped, true);
       return ExceptionResult::MaskException;
     }
 
     if (!initial_stop) {
       initial_stop = true;

lldb/source/Plugins/Process/Windows/Common/NativeThreadWindows.cpp

 }
 
 Status NativeThreadWindows::DoResume(lldb::StateType resume_state) {
   StateType current_state = GetState();
   if (resume_state == current_state)
     return Status();
 
   if (resume_state == eStateStepping) {
+    Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
+
     uint32_t flags_index =
         GetRegisterContext().ConvertRegisterKindToRegisterNumber(
             eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FLAGS);
     uint64_t flags_value =
         GetRegisterContext().ReadRegisterAsUnsigned(flags_index, 0);
+    NativeProcessProtocol &process = GetProcess();
+    const ArchSpec &arch = process.GetArchitecture();
+    switch (arch.GetMachine()) {
+    case llvm::Triple::x86:
+    case llvm::Triple::x86_64:
       flags_value |= 0x100; // Set the trap flag on the CPU
+      break;
+    case llvm::Triple::aarch64:
+    case llvm::Triple::arm:
+    case llvm::Triple::thumb:
+      flags_value |= 0x200000; // The SS bit in PState
+      break;
+    default:
+      LLDB_LOG(log, "single stepping unsupported on this architecture");
+      break;
+    }
     GetRegisterContext().WriteRegisterFromUnsigned(flags_index, flags_value);
   }
 
   if (resume_state == eStateStepping || resume_state == eStateRunning) {
     DWORD previous_suspend_count = 0;
     HANDLE thread_handle = m_host_thread.GetNativeThread().GetSystemHandle();
     do {
       // ResumeThread returns -1 on error, or the thread's *previous* suspend

lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp

case EXCEPTION_SINGLE_STEP: {
     stop_thread->SetStopInfo(stop_info);
 
     return;
   }
 
   case EXCEPTION_BREAKPOINT: {
     RegisterContextSP register_context = stop_thread->GetRegisterContext();
 
-    // The current EIP is AFTER the BP opcode, which is one byte.
-    uint64_t pc = register_context->GetPC() - 1;
+    int breakpoint_size = 1;
+    switch (GetTarget().GetArchitecture().GetMachine()) {
+    case llvm::Triple::aarch64:
+      breakpoint_size = 4;
+      break;
+
+    case llvm::Triple::arm:
+    case llvm::Triple::thumb:
+      breakpoint_size = 2;
+      break;
+
+    case llvm::Triple::x86:
+    case llvm::Triple::x86_64:
+      breakpoint_size = 1;
+      break;
+
+    default:
+      LLDB_LOG(log, "Unknown breakpoint size for architecture");
+      break;
+    }
+
+    // The current PC is AFTER the BP opcode, on all architectures.
+    uint64_t pc = register_context->GetPC() - breakpoint_size;
 
     BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
     if (site) {
       LLDB_LOG(log,
                "detected breakpoint in process {0} at address {1:x} with "
                "breakpoint site {2}",
                m_session_data->m_debugger->GetProcess().GetProcessId(), pc,
                site->GetID());

lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp

 
 Status TargetThreadWindows::DoResume() {
   StateType resume_state = GetTemporaryResumeState();
   StateType current_state = GetState();
   if (resume_state == current_state)
     return Status();
 
   if (resume_state == eStateStepping) {
+    Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
+
     uint32_t flags_index =
         GetRegisterContext()->ConvertRegisterKindToRegisterNumber(
             eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FLAGS);
     uint64_t flags_value =
         GetRegisterContext()->ReadRegisterAsUnsigned(flags_index, 0);
+    ProcessSP process = GetProcess();
+    const ArchSpec &arch = process->GetTarget().GetArchitecture();
+    switch (arch.GetMachine()) {
+    case llvm::Triple::x86:
+    case llvm::Triple::x86_64:
       flags_value |= 0x100; // Set the trap flag on the CPU
+      break;
+    case llvm::Triple::aarch64:
+    case llvm::Triple::arm:
+    case llvm::Triple::thumb:
+      flags_value |= 0x200000; // The SS bit in PState
+      break;
+    default:
+      LLDB_LOG(log, "single stepping unsupported on this architecture");
+      break;
+    }
     GetRegisterContext()->WriteRegisterFromUnsigned(flags_index, flags_value);
   }
 
   if (resume_state == eStateStepping || resume_state == eStateRunning) {
     DWORD previous_suspend_count = 0;
     HANDLE thread_handle = m_host_thread.GetNativeThread().GetSystemHandle();
     do {
       // ResumeThread returns -1 on error, or the thread's *previous* suspend