Re: [PATCH] Fix PR18360 - internal error when using "interrupt -a"

[Luis Machado <lgustavo at codesourcery dot com>]

On 11/08/2016 12:14 PM, Pedro Alves wrote:
> If you do "interrupt -a" just while some thread is stepping over a
> breakpoint, gdb trips on an internal error.
>
> The test added by this patch manages to trigger this consistently by
> spawning a few threads and that are constantly tripping on a
> conditional breakpoint whose conditional always evaluates to false.
> With current gdb, you get:
>
> ~~~
>  interrupt -a
>  .../src/gdb/inline-frame.c:343: internal-error: void skip_inline_frames(ptid_t): Assertion `find_inline_frame_state (ptid) == NULL' failed.
>  A problem internal to GDB has been detected,
>  further debugging may prove unreliable.
>  Quit this debugging session? (y or n) FAIL: gdb.threads/interrupt-while-step-over.exp: displaced-stepping=on: iter=0: interrupt -a (GDB internal error)
> [...]
>  .../src/gdb/inline-frame.c:343: internal-error: void skip_inline_frames(ptid_t): Assertion `find_inline_frame_state (ptid) == NULL' failed.
>  A problem internal to GDB has been detected,
>  further debugging may prove unreliable.
>  Quit this debugging session? (y or n) FAIL: gdb.threads/interrupt-while-step-over.exp: displaced-stepping=off: iter=0: wait for stops (GDB internal error)
> ~~~
>
> The assertion triggers because we're processing a stop for a thread
> that had already stopped before and thus had already its inline-frame
> state filled in.
>
> Calling handle_inferior_event_1 directly within a
> "thread_stop_requested" observer is something that I've wanted to get
> rid of before, for being fragile.  Nowadays, infrun is aware of
> threads with pending events, so we can use that instead, and let the
> normal fetch_inferior_event -> handle_inferior_event code path handle
> the forced stop.
>
> The change to finish_step_over is necessary before sometimes a thread
> that was told to PTRACE_SINGLESTEP reports back a SIGSTOP instead of a
> SIGTRAP (i.e., if tell to single-step, and then interrupt it quick
> enough that on the kernel side the thread dequeues the SIGTOP before
> ever having had a chance of executing the instruction to be stepped).
> SIGSTOP gets translated to a GDB_SIGNAL_0.  And then finish_step_over
> would miss calling clear_step_over_info, and thus miss restarting the
> other threads (which in this case of threads with pending events,
> means setting their "resumed" flag, so their pending events can be
> consumed).
>
> And now that we always restart threads in finish_step_over, we no
> longer need to do that in handle_signal_stop.
>
> Tested on x86_64 Fedora 20, native and gdbserver.
>
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
> 	PR gdb/18360
> 	* infrun.c (start_step_over, do_target_resume, resume)
> 	(restart_threads): Assert we're not resuming a thread that is
> 	meant to be stopped.
> 	(infrun_thread_stop_requested_callback): Delete.
> 	(infrun_thread_stop_requested): If the thread is internally
> 	stopped, queue a pending stop event and clear the thread's
> 	inline-frame state.
> 	(finish_step_over): Clear step over info unconditionally.
> 	(handle_signal_stop): If the user had interrupted the event
> 	thread, consider the stop a random signal.
> 	(handle_signal_stop)<signal arrived while stepping over
> 	breakpoint>: Don't restart threads here.
> 	(stop_waiting): Don't clear step-over info here.
>
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
> 	PR gdb/18360
> 	* gdb.threads/interrupt-while-step-over.c: New file.
> 	* gdb.threads/interrupt-while-step-over.exp: New file.
> ---
>  gdb/infrun.c                                       | 149 ++++++---------
>  .../gdb.threads/interrupt-while-step-over.c        |  75 ++++++++
>  .../gdb.threads/interrupt-while-step-over.exp      | 202 +++++++++++++++++++++
>  .../signal-while-stepping-over-bp-other-thread.exp |   1 -
>  4 files changed, 333 insertions(+), 94 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.threads/interrupt-while-step-over.c
>  create mode 100644 gdb/testsuite/gdb.threads/interrupt-while-step-over.exp

> @@ -6051,6 +6032,15 @@ handle_signal_stop (struct execution_control_state *ecs)
>    if (random_signal)
>      random_signal = !stopped_by_watchpoint;
>
> +  /* Always stop if the user explicitly requested this thread to
> +     remain stopped.  */
> +  if (ecs->event_thread->stop_requested)
> +    {
> +      random_signal = 1;
> +      if (debug_infrun)
> +	fprintf_unfiltered (gdb_stdlog, "infrun: user-requested stop\n");
> +    }
> +

Should we output information about the event thread here to make this 
message a bit more meaningful and make debugging problems easier?

> diff --git a/gdb/testsuite/gdb.threads/interrupt-while-step-over.c b/gdb/testsuite/gdb.threads/interrupt-while-step-over.c
> new file mode 100644
> index 0000000..eeb81d4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/interrupt-while-step-over.c
> @@ -0,0 +1,75 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2016 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <pthread.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +
> +#define NUM_THREADS 20
> +const int num_threads = NUM_THREADS;
> +
> +static pthread_t child_thread[NUM_THREADS];
> +
> +static pthread_barrier_t threads_started_barrier;
> +
> +volatile int always_zero;
> +volatile unsigned int dummy;
> +
> +static void
> +infinite_loop (void)
> +{
> +  while (1)
> +    {
> +      dummy++; /* set breakpoint here */
> +    }
> +}
> +

The test cookbook says we should avoid having infinite loops in case GDB 
crashes. Can we change this so we have something we can expect to finish?

Otherwise i have no other comments on this.