[Lldb-commits] [PATCH] D129814: Fix stepping over watchpoints in architectures that raise the exception before executing the instruction

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D129814#3655750 <https://reviews.llvm.org/D129814#3655750>, @jasonmolenda wrote:

> In D129814#3654368 <https://reviews.llvm.org/D129814#3654368>, @labath wrote:
>
>> In D129814#3654276 <https://reviews.llvm.org/D129814#3654276>, @jasonmolenda wrote:
>>
>>> In D129814#3654230 <https://reviews.llvm.org/D129814#3654230>, @labath wrote:
>>>
>>>> Generally, this makes sense to me, but I do have one question (not necessarily for Jim). These tests work on (arm) linux, and I am wondering why is that the case. Could it be related to the fact that lldb-server preserves the stop reason for threads that are not running? I.e. if we have two threads hit a breakpoint (or whatever), and then step one of them, then the other thread will still report a stop_reason=breakpoint. Do you know if this is supposed to happen (e.g. does debugserver do that)?
>>>
>>> I'd have to go back and test it to be 100% sure but from the behavior I see on Darwin systems, when we receive a watchpoint hit on two threads at the same time, and instruction-step the first thread, when we ask debugserver for the current stop-reason on thread two, it says there is no reason.  The watchpoint exception on the second thread is lost.  I could imagine lldb-server behaving differently. (I think when we fetch the mach exceptions for the threads, they've now been delivered, and when we ask the kernel for any pending mach exceptions for the threads a second time -- even if those threads haven't been allowed to run, I think the state is lost, and debugserver didn't save that initial state it received.)
>>
>> Yes, that sounds plausible. It lldb-server (on linux) it happens differently because we store each stop reason inside the thread object, and since we can control each thread independently, there's no reason to touch it if we're not running the thread. Although it also wouldn't be hard to clear in on every resume.
>>
>> So which one of these behaviors would you say is the correct one?
>
> Yeah, as I was writing my explanation for why we see this problem on Darwin, I realized the obvious next question was, "why isn't this a debugserver fix" - I think that would be another valid way to approach this, although we still need to interop with older debugservers on iOS etc devices from the past five years.  (to be fair, it's relatively uncommon that this issue is hit in real-world programs, our API tests stress this area specifically to expose bugs)  I do wonder about the failure mgorny reported in https://github.com/llvm/llvm-project/issues/48777 which sound like exactly the same problems as we have on Darwin, but on a BSD or linux system running under AArch64 qemu?  (he doesn't say which OS he was running under qemu)
>
> That being said, compensating for this debugserver deficiency in lldb doesn't seem like a bad thing to me, given that the patch exists.

I'm pretty sure "saving suspended thread's stop reasons across a resume" is not a requirement of the gdb-remote protocol, so I don't think we should rely on that behavior.

Plus, this is the wrong way to use the execution control part of lldb.  It's much harder to reason about the execution control machinery if we're in the middle of one round of "How should I react to stop X" and someone just moves the target to Stop X+1 in the middle of that process...  We are in a bit of a tough spot with expressions that run all threads, but (a) that really only happens when user's explicitly ask for it and (b) we already make a distinction between "resumes for expression evaluation" and "natural resumes".  So even if it hadn't caused this bug, that wasn't the right way to implement this bit of business.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129814/new/

https://reviews.llvm.org/D129814