Re: [PATCH v3 4/5] gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-01-12 12:14 p.m., Simon Marchi wrote:
> I think this patch introduces some regressions, when running
> 
> $ while make check TESTS="gdb.threads/interrupt-while-step-over.exp" RUNTESTFLAGS="--target_board=native-extended-gdbserver"; do done
> 
> I'll sometimes get:
> 
> /home/smarchi/src/binutils-gdb/gdb/inline-frame.c:383: internal-error: void skip_inline_frames(thread_info*, bpstat): Assertion `find_inline_frame_state (thread) == NULL' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) 


Ok, I think I found the reason.  We are missing a little something when
enqueuing phony stop replys (stop reply for a thread at was resumed but
not commit-resumed, still in the RESUMED_PENDING_VCONT state).

Imagine the following sequence:

1. Thread is resumed (goes into RESUMED_PENDING_VCONT)
2. Thread is stopped (phony stop reply is enqueued)
3. Core calls commit_resumed, sending a vCont;c for the thread

We are now in a state where we have a stop reply that we are going to
report as a stop to the core, but the thread is also running on the
target -> bad.

I think the state where the thread is in the RESUMED_PENDING_VCONT
resume state but there is a stop reply enqueued for it is wrong
and should be avoided.  When we enqueue a phony stop reply, we want
to pretend that the thread has executed on the remote target, so I
think we should change the thread's state to "resumed".

If I stick a `remote_thr->set_resumed ();` at the place we enqueue
the phony stop replies, I no longer get the failure.

I think this is sufficient, but if we ever want to distinguish
threads that are currently resumed on the remote target vs threads
that are stopped and have a stop reply waiting to be processed,
we could add a 4th resume state STOP_REPLY_PENDING.  When a enqueuing
a stop reply (real or phony), we would move the matching thread(s)
to that state.  And when processing the stop reply / reporting the
event to the core, we would move the state to NOT_RESUMED, just like
we do now.  But to be clear, I don't think we need this today.

Simon