[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 marked an inline comment as done.
jingham added a comment.

In D129814#3658998 <https://reviews.llvm.org/D129814#3658998>, @labath wrote:

> In D129814#3655878 <https://reviews.llvm.org/D129814#3655878>, @jingham 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?
>>
>> Darwin has always worked in such a way that by the time it stops for an exception it will have given other threads enough chance to run that if they were likely to, other threads will hit the exception before the first exception is delivered to lldb.  In gdb we tried to hide that fact by only publishing one exception, but that was awkward since you when you tried to resume to do some piece of work, you'd immediately stop again without getting to run.  That just lead to confusing interactions (and was a little harder to deal with in the debugger as well).  I think it's clearer to stop and see 5 threads have hit your breakpoint, then decide what to do, than to have one breakpoint reported then when you "continue" immediately turn around and get another without anybody really seeming to make progress, rinse, repeat...
>>
>> Rereading this I wasn't sure what question you were asking.  I answered "what should you tell lldb if lldb-server sees 5 threads stopped for reasons at one stop point."  But the other question is "what to do with stop info's if you only run one thread".  If lldb knows it has stopped a thread from running when it resumes the target, it also preserves the StopInfo for that thread.  That seems to me the most useful behavior.
>
> Given your latest comment, I think you've understood my question, but to avoid confusion, the situation I was referring to is when lldb**-server** sees 5 threads stopped for some reason, and then lldb steps one of them. The question was whether lldb-server should still report the original stop reason of those threads (if asked -- theoretically lldb should know that the reason hasn't changed and might decide not to ask).
>
> I agree that preserving the stop reason is the most useful behavior for **lldb** (the threads haven't run, so why should their state change?), but then one could apply the same logic to the lldb-/debugserver component as well.

I think the useful part to lldb is that if IT knows that it was the one responsible for a thread not continuing (it told the stub to stop it) then it should preserve the stop info.  If the stub mirrors this, then that's okay but lldb shouldn't rely on it.  OTOH, if lldb told the stub to continue all threads, but the stub for some reason of it's own didn't continue one of the threads, and then it re-reported the stop info, that would be confusing to lldb.  I can't think of any reason why the stub would do this, but that's the only case where having the stub also do this work is anything but redundant.

> In D129814#3656658 <https://reviews.llvm.org/D129814#3656658>, @jingham wrote:
>
>> 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.
>
> What makes you so sure of that? As you've said before, the idea of having multiple stop reasons per stop is our own invention, so it's not like we have a precedent or a spec we compare ourselves to.

Interesting.  We introduced this many years ago, and I assumed the gdb version must have some way of returning independent stop info for all the threads, but it looks like it still doesn't.  OTOH, it might in the future and more generally it would be nice to rely on the simplest behavior on the part of the stub we can get away with and still preserve as much functionality as we can.  So to me that argues not having lldb rely on this behavior.

> The way I see it, one can fairly easily find justifications for both positions. In a world where we don't save the stop reasons, the reasons are not so much a property **of** a thread, as they are **about** a thread. They are a property of a stop, and once move on from that "stop", the stop reasons go away. If you wanted to preserve the stop reasons, you'd argue that the stop reasons are an intrinsic property of a thread, and since the thread hasn't run, its stop reason shouldn't change.
>
> I don't think either of them is wrong. Just, depending on how you look at things, one of them feels more natural. And if you're on a system where you have to actively suspend a thread to prevent it from running during a resume, you might incline towards a different interpretation than if you're able to control each thread individually (and "suspending a thread" means "doing nothing").

As I said above, the only time having the stub do this on it's own that wasn't purely redundant is when the stub stops something, but lldb didn't think that thread was stopped.  In that case, I think this would be more confusing than helpful.

>> 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 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".  We should certainly not make it worse.  So even if it hadn't caused this bug, that wasn't the right way to implement this bit of business.
>
> Yeah, I'm not arguing against this patch. The reason I brought this up is because it seems we have different behavior of our stubs in these situations. I think that is causing some (other) bugs, and I'd like to know where to fix them.

Unless lldb is getting into trouble because it lacks some piece of information needed to figure out what to do, I think we should be fixing this sort of problem on the lldb end if we can.  That's presumably the agent that knows everything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129814