[Lldb-commits] [PATCH] D33283: RunThreadPlan: Fix halting logic in IgnoreBreakpoints = false

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Ok, I've missed the distinction between plan completing (aka being "done") and completing **sucessfully**. Things make a bit more sense after that.

With that in mind, let me try to explain how I understand it the code now, and then you can tell me if it's correct :)

For our purposes we can get a Stopped event due to one of these things happening:

- plan_success
- breakpoint_hit
- interrupt
- plan_failure (I'm not really sure when can that happen)
- other (this can include the process stopping due to a signal)

What the old code did in the non-halt case was:

  if(plan_success) handle_and_stop()
  else if(breakpoint_hit) handle_and_stop()
  else {
    // "interrupt" (which we can still get even though we haven't sent it) , "plan_failure" or "other"
    handle_and_stop()
  }

The old code in the halt case did:

  if (plan_failure || plan_success) handle_and_stop()
  else {
    // "interrupt", "breakpoint_hit" or "other"
    resume_with_all_threads()
  }

Here, the else part is wrong because we treat two other events the same way as the interrupt.

In my mind, the two versions of the code should behave the same way for all cases except for the "interrupt" case. In the first one, we should stop because the interrupt must have been initiated externally, while in the second one, we should resume because it was (probably) our halt event.

I'll upload a new version which attempts to to just that. The logic in the first case should remain unchanged, while the "halt" logic becomes:

  if(plan_success) handle_and_stop()
  else if(breakpoint_hit) handle_and_stop()
  else if(interrupt) resume_with_all_threads()
  else {
    // "plan_failure" or "other"
    handle_and_stop()
  }

Let me know what you think.

PS: I'm not in a hurry with this, so I can wait a couple of days if you're busy right now.


https://reviews.llvm.org/D33283