This is an archive of the discontinued LLVM Phabricator instance.

lldb can know architecture-dependent watchpoint behaviors, instead of depending on the remote stub
ClosedPublic

Authored by jasonmolenda on Feb 2 2023, 1:46 PM.

Details

Summary

Some CPU architectures will give lldb a watchpoint exception before the instruction has executed (it is rolled back when the address matches), or after the instruction has executed. Currently, lldb learns which type of CPU this is from the gdb remote serial protocol stub though a key in the lldb-extension qHostInfo packet. This is fixed for most architectures, and lldb needs to interop with non-debugserver/non-lldb-server stubs, so moving this into lldb simplifies a lot. This patch still allows the remote stub to override the behavior if it needs to.

There's another lldb extended gdb RSP packet to retrieve the number of watchpoints that this target supports. This needs to be calculated on the target system to be correct, so the stub is the right place to retrieve this knowledge. But lldb needs to interoperate with remote stubs that do not implement this extended packet; we can optionally print the number to the user, but we should not prevent watchpoints from attempting to be set based on this number.

This patch changes lldb to expect these pieces of information -- whether the exceptions are received before/after, and the number of watchpoints -- to not be available from the remote stub, and behave correctly.

The bug driving this was when connected to a gdb RSP stub that did not implement one of these extended packets, and debugging on a target where watchpoint exceptions are received before the instruction executes. lldb would try to fetch whether this machine is a before/after machine, and also fetch the number of watchpoints. The latter packet was not implemented, and Process::GetWatchpointSupportInfo would return error and not provide whether watchpoint exceptions are before/after.

Diff Detail

Event Timeline

jasonmolenda created this revision.Feb 2 2023, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 1:46 PM
jasonmolenda requested review of this revision.Feb 2 2023, 1:46 PM

Ah, I wasn't paying close enough attention and got the logic for MIPS/PPC64 watchpoint exceptions backwards in my update to GDBRemoteCommunicationClient::GetWatchpointReportedAfter, update patch for that.

tl;dr: watchpoints don't work on things like arm targets with a JTAG gdb stub that doesn't support qWatchpointSupportInfo: or qHostInfo with the watchpoint_exceptions_received key.

Nearly all of this patch is mechanical, and IMO the question is whether I should have stuck with Process:GetWatchpointSupportInfo but added some way to indicate that we cannot fetch the number of watchpoints, but we can report whether the exceptions come before/after.

Also this seems a little weak to have it encoded down in GDBRemoteCommunicationClient; it seems like something we could ask the ArchSpec about tbh. I didn't go that far, but maybe that would be better to do while I'm here.

DavidSpickett added a subscriber: DavidSpickett.

I like removing a return by ref and being explicit about what info is wanted where.

So say the stub reports the "watchpoints received" but not the number of slots.

Does lldb just send the watchpoint set packets and rely on the stub to error if it can't handle it (presumably you get a less than helpful error)? As opposed to if you had a number of slots lldb could track how many it had used and give you a nice error instead.

lldb/include/lldb/Target/Process.h
1822–1825

This is a bit muddled.

I guess your point is that number of watchpoints the user can set may be less than the number that the hardware supports. And that this method returns the latter not the former. So perhaps:

This number may be less than the number of watchpoints a user can specify. This is because a single user watchpoint may require multiple watchpoint slots to implement. Due to the size and/or alignment of objects.
1830

I would wave my optional flag here but defining some error value is the way of the land so I will keep it under wraps :)

jasonmolenda updated this revision to Diff 495385.EditedFeb 6 2023, 11:01 PM
jasonmolenda edited the summary of this revision. (Show Details)

David's feedback was spot on, overhaul patch to address.

Change Process::GetWatchpointSlotCount to return an optional count, for when it is available, instead of depending on a magic number.

I've been unhappy with how we calculate if a target reports watchpoint exceptions before or after the instruction has executed. I believe it is target arch specific, I don't think we have any processors that change behavior at runtime. We originally homed this bit of information in debugserver and had it report it in qHostInfo, but it was one of those early decisions that was not correct IMO, and I'm not in favor of perpetuating it. This update to the patch makes a concrete Process::GetWatchpointReportedAfter method that uses the Target architecture to determine it. It also has a call to a DoGetWatchpointReportedAfter method that subclasses can override if they are an environment where the target arch determination is not sufficient - who knows.

This patch separates these into two different Process methods, and sets a default of "before" for arm targets if the qHostInfo packet didn't provide any hints (e.g. watchpoint_exceptions_received:after;), otherwise it returns nullopt.

I removed the before/after calculation from the ProcessWindows plugin because it always returned after; the concrete method in Process already behaves that way.

I need to review & test this patch - it's closer to a WIP at this instant, but I wanted to checkpoint the updates I made so far, and where I'm heading with it.

I phabracated wrong and the end of the last msg was truncated. I meant to end it with this:

If the number of watchpoints could not be fetched with an lldb-extension packet, Process:GetWatchpointSupportInfo would not report whether breakpoints are received before or after the instruction has executed. This latter is important to know to handle watchpoints; the former doesn't really matter. So when we connect to a non-lldb-server/debugserver stub, and we are targetting an arm cpu (where watchpoint exceptions are received before the instruction executes), lldb would not correctly disable the watchpoint/insn-step/re-enable the watchpoint (v. StopInfoWatchpoint::ShouldStopSynchronous in StopInfo.cpp), you'd hit the watchpoint exception over and over without advancing.

This patch separates these into two different Process methods, and sets a default of "before" for arm targets if the qHostInfo packet didn't provide any hints (e.g. watchpoint_exceptions_received:after;), otherwise it returns nullopt.

I removed the before/after calculation from the ProcessWindows plugin because it always returned after; the concrete method in Process already behaves that way for intel.

I need to review & test this patch - it's closer to a WIP at this instant, but I wanted to checkpoint the updates I made so far, and where I'm heading with it.

Went over the rewrite one more time, fixing up comments, fixing a couple of logic bugs I wrote, and having tested it with a debugserver I hacked to remove the watchpoint_exceptions_received:before; key in the qHostInfo reply packet, so lldb had to use its knowledge of the target system to determine this.

I have changed ProcessWindows and can't build that to ensure it's correct, but the changes are tiny and I'll keep an eye on the bots when I land it.

I think this is ready for review now.

If I was going to describe this patch briefly at this point, I would say: "Change lldb from depending on the remote gdb stub to tell it whether watchpoints are received before or after the instruction, to knowing the architectures where watchpoint exceptions are received before the instruction executes, and allow the remote gdb stub to override this behavior in its qHostInfo response". The rest is mechanical change.

Change lldb from depending on the remote gdb stub to tell it whether watchpoints are received before or after the instruction, to knowing the architectures where watchpoint exceptions are received before the instruction executes, and allow the remote gdb stub to override this behavior in its qHostInfo response.

Sounds good to me. Can you simplify the commit message some now that you've got the changes you want?

Seems like the main things are:

  • Split up querying watchpoint reporting style, and number of watchpoint slots.
  • Have process determine the reporting style by target, with the option for the remote to override. Meaning this query never fails.
  • Only conclude that watchpoints are unsupported if the remote tells us that it has 0 slots.

(the last one I think we always did but good for context)

lldb/source/Target/Process.cpp
2360 ↗(On Diff #495686)

Can do if (std::optional<bool> subclass....) {... here.

Also no {} for single line if.

2367 ↗(On Diff #495686)

No {} needed here too.

2373 ↗(On Diff #495686)

Would this be any clearer with multiple returns? Or one giant return, but the logic is a bit cryptic then.

const ArchSpec &arch = GetTarget().GetArchitecture();
if (!arch.IsValid())
  return true;

llvm::Triple triple = arch.GetTriple();
return !(triple.isMIPS() || triple.isPPC64() || triple.isAArch64() || triple.isArmMClass() || triple.isARM());

Better as:

const ArchSpec &arch = GetTarget().GetArchitecture();
if (!arch.IsValid())
  return false;

llvm::Triple triple = arch.GetTriple();
if (triple.isMIPS() || triple.isPPC64() || triple.isAArch64() || triple.isArmMClass() || triple.isARM())
  return false;

return true;

Also, do we know what RISC-V does?

lldb/source/Target/StopInfo.cpp
833

This is no longer needed because GetWatchpointReportedAfter will fall back to the process' defaults if the GDB layer does not/can not provide a value, correct?

lldb/source/Target/Target.cpp
804

This should just append "Target supports 0 hardware...".

In theory a compiler could figure that out but also it's a bit confusing because it makes the reader wonder what value other than 0 is supposed to end up here.

DavidSpickett added inline comments.
lldb/source/Target/Process.cpp
2373 ↗(On Diff #495686)

@Emmmer any idea?

Emmmer added inline comments.Feb 8 2023, 7:41 AM
lldb/source/Target/Process.cpp
2373 ↗(On Diff #495686)

It seems standard RISC-V uses the before mode, that is

The action for this trigger will be taken just before the instruction that triggered it is committed, but after all preceding instructions are committed.

If I understand this code correctly (correct me if I was wrong), we should return false for RISC-V.

But in the debug-spec [1], RISC-V seems to allow both before and after modes, which can be detected through CSRs. When debug-spec lands, we may change the code accordingly.

[1]: https://github.com/riscv/riscv-debug-spec/blob/6309972901417a0fa72d812d2ffe89e432f00dff/xml/hwbp_registers.xml#L365

jasonmolenda added inline comments.Feb 8 2023, 3:18 PM
lldb/source/Target/Process.cpp
2373 ↗(On Diff #495686)

Thanks! I'll add isRISCV() to the architectures we default to "watchpoints received before instruction executes". This can be overridden by a gdb remote stub which adds the lldb-extension key to qHostInfo response. although it sounds like it could technically be per *process*??? in which case the key should really be sent in qProcessInfo. The remote stub should be able to send qHostInfo before it has attached to any specific process.

lldb/source/Target/StopInfo.cpp
833

Previously, this would fetch the number of watchpoints available (if the lldb-extended packet was provided) and it would fetch whether watchpoints are received before or after (if the qHostInfo includes the lldb-extended key), and you would get an error if the former was unavailable. (I think the latter would default to "watchpoints are after"). This m_should_stop = true is the behavior when watchpoints are received after the instruction has executed; this is the crux of the bug I was trying to fix, where lldb would not instruction step over the instruction and re-add it, when the packet declaring the number of watchpoints was unimplemented.

lldb/source/Target/Target.cpp
804

It is a bit confusing. The layers that return the optional<uint32_t> will return the actual number of hardware watchpoints available, including 0 if that is the correct value. Or a nullopt to indicate that it could not be retrieved. So in this method, we got back the number of watchpoints available, and it was said to be 0. lldb won't actually check against this number if you try to set one, so even if we got a bogus value here, you could try to set a watchpoint and see if it works. SBProcess::GetNumSupportedHardwareWatchpoints() takes an SBError out arg & returns a uint32_t. It can return 0 to mean "could not get number of watchpoints" or "there are zero watchpoints" but the SBError tells you whether it was the failure case or not.

jasonmolenda retitled this revision from Separate Process::GetWatchpointSupportInfo into two methods to get the two separate pieces of information to lldb can know architecture-dependent watchpoint behaviors, instead of depending on the remote stub.
jasonmolenda edited the summary of this revision. (Show Details)

Update to fix David's second round of comments, rewrite the Title/Summary to more accurately reflect what I'm doing in this patch. I think we're about done here, but open to any comments.

DavidSpickett accepted this revision.Feb 9 2023, 1:22 AM

One minor comment otherwise LGTM.

[LLDB] tag in the commit title please :)

lldb/source/Target/Process.cpp
2371 ↗(On Diff #495976)

Combine these into one and if you feel like it, return false instead.

This revision is now accepted and ready to land.Feb 9 2023, 1:22 AM