Fix stepping over watchpoints in architectures that raise the exception before executing the instruction

[jingham]

Summary

On some architectures - e.g. aarch64 - the watchpoint exception is raised before the instruction that would have triggered it gets run. We want to report old/new values, however, so we need to have executed the instruction before reporting the stop. That means before reporting the stop, we need to do one stepi on the thread that got the watchpoint exception.

The original version of this code implemented this incorrectly. It did the step over synchronously in the StopInfoWatchpoint::ShouldStopSynchronous. You should NEVER run the target in ShouldStopSynchronous. If you do that, then any other threads that had interesting pending stop info actions won't be able to perform their action because the target will have moved on and invalidated their StopInfo's.

You should always do this kind of thing by pushing the action you want to perform onto the ThreadPlanStack for the thread in question. That way all the threads get to register what they need to do, and then the ThreadList runner can arbitrate among them and do the right thing. For instance, in the case of stepping over watchpoints, each thread stopped on the watchpoint pushes a "step instruction" thread plan that re-instates the original watchpoint StopInfo when complete. If there is more than one thread in this state, lldb notices each of them has a "stop others" plan, and one by one lets each thread step over the watchpoint. When all this work is done, we report all the threads stopped at their watchpoints.

I also fixed a bug in the same code I noticed while reworking this. We were not decrementing the HitCount for the watchpoint if the condition fails. We consider a failed condition to mean "the stop point wasn't hit". It's weird to see a hit count of 5 when the target only told you about one stop... But that wasn't being enforced in the StopInfoWatchpoint::PerformAction.

There was a test that was expecting the now-incorrect value of the hit count in this case, so I fixed that, and also all the tests that were failing because of concurrent access were turned on again and run successfully for me.

There's a bit of funniness for MIPS in the watchpoint support. I am pretty sure I got that right, but I don't have anything MIPS to test that on. The same concurrent watchpoint tests that were marked failing for arm64 Darwin are also marked failed for MIPS, but I am not sure if that's for the same reason. Anyway, if someone has access to a MIPS system, it would be good to try this out and see if it works there as well.

[jingham]

Ack, bad memory. It was actually originally done in PerformAction, but that has the same effect. You really shouldn't run the target in PerformAction if you can manage it. That's why, for instance, I added a --auto-continue flag to the breakpoints (probably should do the same for watchpoints) so that the thread list code gets a chance to reason about the state before continuing. We do run expressions there, but we should really only do that if (a) we know the expression will only run the current thread or (b) we're doing it for the user, where if they have an expression that requires all threads to run, they needed to do that and presumably were willing to trade off running the expression for changing the state of the other threads from what it was at the initial stop.

[jasonmolenda]

I read through the patch and I'm not sure I have much useful feedback because I haven't worked on the ThreadPlan code very much, but this does look like what we were discussing for this. I believe this patch will fix https://github.com/llvm/llvm-project/issues/48777 . Regarding MIPS suport, in reading through the patch, I was reviewing relevant calls and saw this bit in GDBRemoteCommunicationClient::GetWatchpointsTriggerAfterInstruction() - so I believe you're right this change should also fix multithreaded watchpoint hitting on those targets.

// On targets like MIPS and ppc64, watchpoint exceptions are always
// generated before the instruction is executed. The connected target may
// not support qHostInfo or qWatchpointSupportInfo packets.
after = !(triple.isMIPS() || triple.isPPC64());

[labath]

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

(It wouldn't surprise me if this was a bug in lldb-server, as I think it's related to some strange behavior with multiple threads stopped (in these situations, lldb will sometimes keep switching away from the debugged thread after every stop), but it was never clear to me whether this was a problem in lldb or lldb-server.)

lldb/source/Target/StopInfo.cpp

804

This creates a shared pointer loop, does it not?

IIUC, this m_step_over_wp_sp is used only to check whether the plan is complete. Maybe, instead of the pointer, we could have a flag that the thread plan sets when it finishes?

897–901

How does this relate to the thread plan part of the patch?

[jasonmolenda]

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

[labath]

In D129814#3654276, @jasonmolenda wrote:

In 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?

[DavidSpickett]

lldb/source/Target/StopInfo.cpp

724

Blank line after this.

748

Depending how you read it this seems backwards. if m_reset_watchpoint is true don't reset it? m_did_reset_watchpoint maybe?

758

return m_step_over_wp_sp->IsPlanComplete(); ?

808

return error.Success() ?

[jasonmolenda]

In D129814#3654368, @labath wrote:

In D129814#3654276, @jasonmolenda wrote:

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

[jingham]

Address review comments.

1. Name changes and formatting
2. The ThreadPlan needs to keep the StopInfo alive but not vice versa. So I removed the sp to the StopInfo, and replaced it with a callback in the StopInfo that the thread plan can call to tell it it is done.
3. I removed the "ResetWatchpoint" call in the ThreadPlan destructor. I don't think on second thoughts that's right. By the time you've gotten to the ThreadPlan destructor, I don't think you know that you are the one in charge of that watchpoint anymore, so the plan shouldn't change it at that point.

[jingham]

lldb/source/Target/StopInfo.cpp

804

Ack, yes. I added a "I'm complete" callback to the StopInfoWatchpoint, and the ThreadPlan calls that.

808

Yeah, sometimes it's useful to break on one or other condition w/o having to add conditional breakpoints, but that doesn't seem to be the prevailing style.

897–901

This is a drive-by fix. The point of this effort was to make the hit counts correct for Watchpoints, and as I was reading through the code I noticed that we were actually failing to do that in two ways: mishandling the case of multiple simultaneous watchpoint hits, and mishandling failed condition hits.

I mentioned in the initial comments that I fixed them both in one patch, but I can separate them into two patches if that bugs you.

[jingham]

In D129814#3654368, @labath wrote:

In D129814#3654276, @jasonmolenda wrote:

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

[jingham]

In D129814#3655750, @jasonmolenda wrote:

In D129814#3654368, @labath wrote:

In D129814#3654276, @jasonmolenda wrote:

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

[labath]

In D129814#3655878, @jingham wrote:

In D129814#3654368, @labath wrote:

In D129814#3654276, @jasonmolenda wrote:

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

In D129814#3656658, @jingham wrote:

In D129814#3655750, @jasonmolenda wrote:

In D129814#3654368, @labath wrote:

In D129814#3654276, @jasonmolenda wrote:

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

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

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.

lldb/source/Target/StopInfo.cpp

730

Change type of m_stop_info_sp (and the relevant constructor argument) to shared_ptr<StopInfoWatchpoint> to avoid the cast here.

(btw, the right cast for upcasts is static_cast. reinterpret_cast can return wrong results in more complicated scenarios)

805

And use static_pointer_cast<StopInfoWatchpoint>(shared_from_this()) here. That way the casting remains confined within the origin class.

897–901

Sorry, I missed that part of the patch description. Well, the main question on my mind was whether I am missing something. Now that that's cleared up, it's not so much of a problem (I am assuming this is related to/tested by the change in TestWatchpointConditionCmd.py), though I would generally recommend splitting that off into a separate patch in the future (and I don't think you'd need to create a review for a simple change like this).

994

delete

lldb/test/API/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py

82–83

I guess this comment was wrong before, but better update it now. (or delete it -- it's kind of self-evident)

[jingham]

Address review comments

[jingham]

lldb/source/Target/StopInfo.cpp

730

Change type of m_stop_info_sp (and the relevant constructor argument) to shared_ptr<StopInfoWatchpoint> to avoid the cast here.

Cute, I didn't know the voodoo to change the type of the pointer inside a shared pointer while retaining its reference counts.

(btw, the right cast for upcasts is static_cast. reinterpret_cast can return wrong results in more complicated scenarios)

Huh, interesting. The names seem backwards to me, static_cast sounds like it takes the pointer you gave it and casts it to the target type, whereas reinterpret sounds like the one that figures out what the class hierarchy actually is and does the right thing, but it looks like it's the other way round.

805

I didn't know that voodoo, looks like we use it in other places, however, so I switched to doing it that way.

[jingham]

In D129814#3658998, @labath wrote:

In D129814#3655878, @jingham wrote:

In D129814#3654368, @labath wrote:

In D129814#3654276, @jasonmolenda wrote:

In 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, @jingham wrote:

In D129814#3655750, @jasonmolenda wrote:

In D129814#3654368, @labath wrote:

In D129814#3654276, @jasonmolenda wrote:

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

lldb/include/lldb/Breakpoint/Watchpoint.h

public:
 
   bool IsDisabledDuringEphemeralMode();
 
   const CompilerType &GetCompilerType() { return m_type; }
 
 private:
   friend class Target;
   friend class WatchpointList;
+  friend class StopInfoWatchpoint; // This needs to call UndoHitCount()
 
   void ResetHistoricValues() {
     m_old_value_sp.reset();
     m_new_value_sp.reset();
   }
 
+  void UndoHitCount() { m_hit_counter.Decrement(); }
+
   Target &m_target;
   bool m_enabled;           // Is this watchpoint enabled
   bool m_is_hardware;       // Is this a hardware watchpoint
   bool m_is_watch_variable; // True if set via 'watchpoint set variable'.
   bool m_is_ephemeral;      // True if the watchpoint is in the ephemeral mode,
                             // meaning that it is
   // undergoing a pair of temporary disable/enable actions to avoid recursively
   // triggering further watchpoint events.

lldb/include/lldb/Target/StopInfo.h

 #include <string>
 
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-public.h"
 
 namespace lldb_private {
 
-class StopInfo {
+class StopInfo : public std::enable_shared_from_this<StopInfo> {
   friend class Process::ProcessEventData;
   friend class ThreadPlanBase;
 
 public:
   // Constructors and Destructors
   StopInfo(Thread &thread, uint64_t value);
 
   virtual ~StopInfo() = default;

lldb/source/Target/StopInfo.cpp

 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StopInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadPlan.h"
+#include "lldb/Target/ThreadPlanStepInstruction.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
if (m_description.empty()) {
       StreamString strm;
       strm.Printf("watchpoint %" PRIi64, m_value);
       m_description = std::string(strm.GetString());
     }
     return m_description.c_str();
   }
 
 protected:
+  using StopInfoWatchpointSP = std::shared_ptr<StopInfoWatchpoint>;
+  // This plan is used to orchestrate stepping over the watchpoint for
+  // architectures (e.g. ARM) that report the watch before running the watched
+  // access.  This is the sort of job you have to defer to the thread plans,
+  // if you try to do it directly in the stop info and there are other threads
+  // that needed to process this stop you will have yanked control away from
+  // them and they won't behave correctly.
+  class ThreadPlanStepOverWatchpoint : public ThreadPlanStepInstruction {
+  public:
+    ThreadPlanStepOverWatchpoint(Thread &thread, 
+                                 StopInfoWatchpointSP stop_info_sp,
+                                 WatchpointSP watch_sp)
+        : ThreadPlanStepInstruction(thread, false, true, eVoteNoOpinion,
+                                    eVoteNoOpinion),
+          m_stop_info_sp(stop_info_sp), m_watch_sp(watch_sp) {
+      assert(watch_sp);
+      m_watch_index = watch_sp->GetHardwareIndex();
+    }
+
+    bool DoWillResume(lldb::StateType resume_state,
+                      bool current_plan) override {
+      if (m_did_reset_watchpoint) {
+        GetThread().GetProcess()->DisableWatchpoint(m_watch_sp.get(), false);
+        m_did_reset_watchpoint = false;
+      }
+      return true;
+    }
+
+    void DidPop() override {
+      // Don't artifically keep the watchpoint alive.
+      m_watch_sp.reset();
+    }
+    
+    bool ShouldStop(Event *event_ptr) override {
+      bool should_stop = ThreadPlanStepInstruction::ShouldStop(event_ptr);
+      bool plan_done = MischiefManaged();
+      if (plan_done) {
+        m_stop_info_sp->SetStepOverPlanComplete();
+        GetThread().SetStopInfo(m_stop_info_sp);
+        ResetWatchpoint();
+      }
+      return should_stop;
+    }
+
+  protected:
+    void ResetWatchpoint() {
+      if (m_did_reset_watchpoint)
+        return;
+      m_did_reset_watchpoint = true;
+      GetThread().GetProcess()->EnableWatchpoint(m_watch_sp.get(), false);
+      m_watch_sp->SetHardwareIndex(m_watch_index);
+    }
+
+  private:
+    StopInfoWatchpointSP m_stop_info_sp;
+    WatchpointSP m_watch_sp;
+    uint32_t m_watch_index = LLDB_INVALID_INDEX32;
+    bool m_did_reset_watchpoint = false;
+  };
+
   bool ShouldStopSynchronous(Event *event_ptr) override {
-    // ShouldStop() method is idempotent and should not affect hit count. See
-    // Process::RunPrivateStateThread()->Process()->HandlePrivateEvent()
-    // -->Process()::ShouldBroadcastEvent()->ThreadList::ShouldStop()->
-    // Thread::ShouldStop()->ThreadPlanBase::ShouldStop()->
-    // StopInfoWatchpoint::ShouldStop() and
-    // Event::DoOnRemoval()->Process::ProcessEventData::DoOnRemoval()->
-    // StopInfoWatchpoint::PerformAction().
-    if (m_should_stop_is_valid)
-      return m_should_stop;
+    // If we are running our step-over the watchpoint plan, stop if it's done
+    // and continue if it's not:
+    if (m_using_step_over_plan)
+      return m_step_over_plan_complete;
 
     ThreadSP thread_sp(m_thread_wp.lock());
-    if (thread_sp) {
-      WatchpointSP wp_sp(
-          thread_sp->CalculateTarget()->GetWatchpointList().FindByID(
-              GetValue()));
-      if (wp_sp) {
-        // Check if we should stop at a watchpoint.
-        ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
-        StoppointCallbackContext context(event_ptr, exe_ctx, true);
-        m_should_stop = wp_sp->ShouldStop(&context);
-      } else {
-        Log *log = GetLog(LLDBLog::Process);
+    assert(thread_sp);
+        thread_sp->CalculateTarget()->GetWatchpointList().FindByID(GetValue()));
+    if (!wp_sp) {
 
-        LLDB_LOGF(log,
-                  "Process::%s could not find watchpoint location id: %" PRId64
-                  "...",
-                  __FUNCTION__, GetValue());
 
-        m_should_stop = true;
-      }
-    }
+      m_should_stop_is_valid = true;
+      return true;
+
+    ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
+    StoppointCallbackContext context(event_ptr, exe_ctx, true);
+    m_should_stop = wp_sp->ShouldStop(&context);
+    if (!m_should_stop) {
+      // This won't happen at present because we only allow one watchpoint per
+      // watched range.  So we won't stop at a watched address with a disabled
+      // watchpoint.  If we start allowing overlapping watchpoints, then we
+      // will have to make watchpoints be real "WatchpointSite" and delegate to
+      // all the watchpoints sharing the site.  In that case, the code below
+      // would be the right thing to do.
       m_should_stop_is_valid = true;
       return m_should_stop;
     }
+    // If this is a system where we need to execute the watchpoint by hand
+    // after the hit, queue a thread plan to do that, and then say not to stop.
+    // Otherwise, let the async action figure out whether the watchpoint should
+    // stop
+
+    ProcessSP process_sp = exe_ctx.GetProcessSP();
+    uint32_t num;
+    bool wp_triggers_after;
+
+    if (process_sp->GetWatchpointSupportInfo(num, wp_triggers_after)
+            .Success()) {
+      if (wp_triggers_after)
+        return true;
+      
+      StopInfoWatchpointSP me_as_siwp_sp 
+          = std::static_pointer_cast<StopInfoWatchpoint>(shared_from_this());
+      ThreadPlanSP step_over_wp_sp(new ThreadPlanStepOverWatchpoint(
+          *(thread_sp.get()), me_as_siwp_sp, wp_sp));
+      Status error;
+      error = thread_sp->QueueThreadPlan(step_over_wp_sp, false);
+      m_using_step_over_plan = true;
+      return !error.Success();
+    }
+    // If we don't have to step over the watchpoint, just let the PerformAction
+    // determine what we should do.
+    return true;
+  }
 
   bool ShouldStop(Event *event_ptr) override {
     // This just reports the work done by PerformAction or the synchronous
     // stop. It should only ever get called after they have had a chance to
     // run.
     assert(m_should_stop_is_valid);
     return m_should_stop;
   }
 
   void PerformAction(Event *event_ptr) override {
     Log *log = GetLog(LLDBLog::Watchpoints);
     // We're going to calculate if we should stop or not in some way during the
     // course of this code.  Also by default we're going to stop, so set that
     // here.
     m_should_stop = true;
 
 
     ThreadSP thread_sp(m_thread_wp.lock());
     if (thread_sp) {
 
       WatchpointSP wp_sp(
           thread_sp->CalculateTarget()->GetWatchpointList().FindByID(
               GetValue()));
       if (wp_sp) {
-        ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
-        ProcessSP process_sp = exe_ctx.GetProcessSP();
-
-        {
-          // check if this process is running on an architecture where
-          // watchpoints trigger before the associated instruction runs. if so,
-          // disable the WP, single-step and then re-enable the watchpoint
-          if (process_sp) {
-            uint32_t num;
-            bool wp_triggers_after;
-
-            if (process_sp->GetWatchpointSupportInfo(num, wp_triggers_after)
-                    .Success()) {
-              if (!wp_triggers_after) {
-                // We need to preserve the watch_index before watchpoint  is
-                // disable. Since Watchpoint::SetEnabled will clear the watch
-                // index. This will fix TestWatchpointIter failure
-                Watchpoint *wp = wp_sp.get();
-                uint32_t watch_index = wp->GetHardwareIndex();
-                process_sp->DisableWatchpoint(wp, false);
-                StopInfoSP stored_stop_info_sp = thread_sp->GetStopInfo();
-                assert(stored_stop_info_sp.get() == this);
-
-                Status new_plan_status;
-                ThreadPlanSP new_plan_sp(
-                    thread_sp->QueueThreadPlanForStepSingleInstruction(
-                        false, // step-over
-                        false, // abort_other_plans
-                        true,  // stop_other_threads
-                        new_plan_status));
-                if (new_plan_sp && new_plan_status.Success()) {
-                  new_plan_sp->SetIsControllingPlan(true);
-                  new_plan_sp->SetOkayToDiscard(false);
-                  new_plan_sp->SetPrivate(true);
-                }
-                process_sp->GetThreadList().SetSelectedThreadByID(
-                    thread_sp->GetID());
-                process_sp->ResumeSynchronous(nullptr);
-                process_sp->GetThreadList().SetSelectedThreadByID(
-                    thread_sp->GetID());
-                thread_sp->SetStopInfo(stored_stop_info_sp);
-                process_sp->EnableWatchpoint(wp, false);
-                wp->SetHardwareIndex(watch_index);
-              }
-            }
-          }
-        }
-
         // This sentry object makes sure the current watchpoint is disabled
         // while performing watchpoint actions, and it is then enabled after we
         // are finished.
+        ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
+        ProcessSP process_sp = exe_ctx.GetProcessSP();
+
         WatchpointSentry sentry(process_sp, wp_sp);
 
         /*
          * MIPS: Last 3bits of the watchpoint address are masked by the kernel.
          * For example:
          * 'n' is at 0x120010d00 and 'm' is 0x120010d04. When a watchpoint is
          * set at 'm', then
          * watch exception is generated even when 'n' is read/written. To handle
if (thread_sp) {
               thread_sp->CalculateTarget()->GetWatchpointList().FindByAddress(
                   m_watch_hit_addr);
           if (!wp_hit_sp) {
             m_should_stop = false;
             wp_sp->IncrementFalseAlarmsAndReviseHitCount();
           }
         }
 
-        // TODO: This condition should be checked in the synchronous part of the
-        // watchpoint code
-        // (Watchpoint::ShouldStop), so that we avoid pulling an event even if
-        // the watchpoint fails the ignore count condition. It is moved here
-        // temporarily, because for archs with
-        // watchpoint_exceptions_received=before, the code in the previous
-        // lines takes care of moving the inferior to next PC. We have to check
-        // the ignore count condition after this is done, otherwise we will hit
-        // same watchpoint multiple times until we pass ignore condition, but
-        // we won't actually be ignoring them.
-        if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount())
+        if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount()) {
           m_should_stop = false;
+          m_should_stop_is_valid = true;
+        }
 
         Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();
 
         if (m_should_stop && wp_sp->GetConditionText() != nullptr) {
           // We need to make sure the user sees any parse errors in their
           // condition, so we'll hook the constructor errors up to the
           // debugger's Async I/O.
           ExpressionResults result_code;
           EvaluateExpressionOptions expr_options;
           expr_options.SetUnwindOnError(true);
           expr_options.SetIgnoreBreakpoints(true);
           ValueObjectSP result_value_sp;
           Status error;
           result_code = UserExpression::Evaluate(
               exe_ctx, expr_options, wp_sp->GetConditionText(),
               llvm::StringRef(), result_value_sp, error);
 
           if (result_code == eExpressionCompleted) {
             if (result_value_sp) {
               Scalar scalar_value;
               if (result_value_sp->ResolveValue(scalar_value)) {
                 if (scalar_value.ULongLong(1) == 0) {
-                  // We have been vetoed.  This takes precedence over querying
-                  // the watchpoint whether it should stop (aka ignore count
-                  // and friends).  See also StopInfoWatchpoint::ShouldStop()
-                  // as well as Process::ProcessEventData::DoOnRemoval().
+                  // The condition failed, which we consider "not having hit
+                  // the watchpoint" so undo the hit count here.
+                  wp_sp->UndoHitCount();
                   m_should_stop = false;
                 } else
                   m_should_stop = true;
                 LLDB_LOGF(log,
                           "Condition successfully evaluated, result is %s.\n",
                           m_should_stop ? "true" : "false");
               } else {
                 m_should_stop = true;
                 LLDB_LOGF(
if (thread_sp) {
                 "Process::%s returning from action with m_should_stop: %d.",
                 __FUNCTION__, m_should_stop);
 
       m_should_stop_is_valid = true;
     }
   }
 
 private:
+  void SetStepOverPlanComplete() {
+    assert(m_using_step_over_plan);
+    m_step_over_plan_complete = true;
+  }
+  
   bool m_should_stop = false;
   bool m_should_stop_is_valid = false;
   lldb::addr_t m_watch_hit_addr;
+  bool m_step_over_plan_complete = false;
+  bool m_using_step_over_plan = false;
 };
 
 // StopInfoUnixSignal
 
 class StopInfoUnixSignal : public StopInfo {
 public:
   StopInfoUnixSignal(Thread &thread, int signo, const char *description)
       : StopInfo(thread, signo) {
     SetDescription(description);

lldb/test/API/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py

def test_watchpoint_cond(self):
 
         # We should be stopped again due to the watchpoint (write type).
         # The stop reason of the thread should be watchpoint.
         self.expect("thread backtrace", STOPPED_DUE_TO_WATCHPOINT,
                     substrs=['stop reason = watchpoint'])
         self.expect("frame variable --show-globals global",
                     substrs=['(int32_t)', 'global = 5'])
 
         # Use the '-v' option to do verbose listing of the watchpoint.
         # The hit count should now be 2.
         self.expect("watchpoint list -v",
-                    substrs=['hit_count = 5'])
+                    substrs=['hit_count = 1'])

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayWatchBreak.py

 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentDelayWatchBreak(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @skipIf(
-        oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-        archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-        bugnumber="rdar://81811539")
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test (1-second delay) watchpoint and a breakpoint in multiple threads."""
         self.build()
         self.do_thread_actions(
             num_breakpoint_threads=1,
             num_delay_watchpoint_threads=1)

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py

 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentManyWatchpoints(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @skipIf(
-        oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-        archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-        bugnumber="rdar://93863107")
     @add_test_categories(["watchpoint"])
     @skipIfOutOfTreeDebugserver
     def test(self):
         """Test 100 watchpoints from 100 threads."""
         self.build()
         self.do_thread_actions(num_watchpoint_threads=100)

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py

 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentNWatchNBreak(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
     @expectedFailureAll(archs=["aarch64"], oslist=["freebsd"],
                         bugnumber="llvm.org/pr49433")
-    @skipIf(
-        oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-        archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-        bugnumber="rdar://93863107")
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test with 5 watchpoint and breakpoint threads."""
         self.build()
         self.do_thread_actions(num_watchpoint_threads=5,
                                num_breakpoint_threads=5)

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalNWatchNBreak.py

 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentSignalNWatchNBreak(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
     @expectedFailureNetBSD
     @expectedFailureAll(archs=["aarch64"], oslist=["freebsd"],
                         bugnumber="llvm.org/pr49433")
-    @skipIf(
-        oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-        archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-        bugnumber="rdar://93863107")
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test one signal thread with 5 watchpoint and breakpoint threads."""
         self.build()
         self.do_thread_actions(num_signal_threads=1,
                                num_watchpoint_threads=5,
                                num_breakpoint_threads=5)

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatch.py

 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentSignalWatch(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @skipIf(
-        oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-        archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-        bugnumber="rdar://93863107")
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test a watchpoint and a signal in multiple threads."""
         self.build()
         self.do_thread_actions(num_signal_threads=1, num_watchpoint_threads=1)

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatchBreak.py

 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentSignalWatchBreak(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
     @expectedFailureNetBSD
-    @skipIf(
-        oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-        archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-        bugnumber="rdar://93863107")
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test a signal/watchpoint/breakpoint in multiple threads."""
         self.build()
         self.do_thread_actions(num_signal_threads=1,
                                num_watchpoint_threads=1,
                                num_breakpoint_threads=1)

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py

 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentTwoWatchpointThreads(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @skipIf(
-        oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-        archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-        bugnumber="rdar://93863107")
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test two threads that trigger a watchpoint. """
         self.build()
         self.do_thread_actions(num_watchpoint_threads=2)

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py

 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentTwoWatchpointsOneBreakpoint(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @skipIf(
-        oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-        archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-        bugnumber="rdar://93863107")
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test two threads that trigger a watchpoint and one breakpoint thread. """
         self.build()
         self.do_thread_actions(
             num_watchpoint_threads=2,
             num_breakpoint_threads=1)

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py

 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentTwoWatchpointsOneDelayBreakpoint(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @skipIf(
-        oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-        archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-        bugnumber="rdar://93863107")
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test two threads that trigger a watchpoint and one (1 second delay) breakpoint thread. """
         self.build()
         self.do_thread_actions(
             num_watchpoint_threads=2,
             num_delay_breakpoint_threads=1)

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py

 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentTwoWatchpointsOneSignal(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
     @expectedFailureNetBSD
-    @skipIf(
-        oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-        archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-        bugnumber="rdar://93863107")
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test two threads that trigger a watchpoint and one signal thread. """
         self.build()
         self.do_thread_actions(num_watchpoint_threads=2, num_signal_threads=1)

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py

 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentWatchBreak(ConcurrentEventsBase):
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
     @add_test_categories(["watchpoint"])
-    @skipIf(
-    oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-    archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-    bugnumber="rdar://93863107")
 
     def test(self):
         """Test watchpoint and a breakpoint in multiple threads."""
         self.build()
         self.do_thread_actions(
             num_breakpoint_threads=1,
             num_watchpoint_threads=1)