Page MenuHomePhabricator

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

Authored by mstorsjo on Sep 14 2021, 12:17 PM.

Diff Detail

Event Timeline

mstorsjo created this revision.Sep 14 2021, 12:17 PM
mstorsjo requested review of this revision.Sep 14 2021, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2021, 12:17 PM
DavidSpickett added a subscriber: 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)

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.

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.

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.

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.

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 added inline comments.Sep 17 2021, 5:07 AM
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 updated this revision to Diff 373195.Sep 17 2021, 5:09 AM

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.

@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 accepted this revision.Sep 22 2021, 2:04 AM

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).

This revision is now accepted and ready to land.Sep 22 2021, 2:04 AM

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.

This revision was landed with ongoing or failed builds.Sep 22 2021, 4:11 AM
This revision was automatically updated to reflect the committed changes.