Re: [PATCH v2 2/6] gdb/infrun: in condition evaluation resume only current thread.

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org> [2020-10-09 13:27:14 +0200]:

> When stopped at a conditional BP, GDB evaluates the condition for the
> event thread in order to decide whether the thread should be stopped or
> resumed.  Thus, if the condition includes an inferior call, during
> the call not all threads have finalized state, some of them might
> still be shown as running.  While executing the proceed for the inferior
> call, GDB tries to resume all non-exited threads.  This leads to
> the following assertion, when GDB tries to resume a running thread:
> 
>     (gdb) b 43 if foo()
>     Breakpoint 1 at 0x119b: file main.c, line 43.
>     (gdb) run
>     Starting program: main
>     [Thread debugging using libthread_db enabled]
>     Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>     [New Thread 0x7ffff77c4700 (LWP 21421)]
>     [New Thread 0x7ffff6fc3700 (LWP 21422)]
>     gdb/nat/x86-linux-dregs.c:146: internal-error: void x86_linux_update_debug_registers(lwp_info*): Assertion `lwp_is_stopped (lwp)' failed.
>     A problem internal to GDB has been detected,
>     further debugging may prove unreliable.
> 
>     This is a bug, please report it.  For instructions, see:
>     <https://www.gnu.org/software/gdb/bugs/>.
> 
> This patch changes the behavior, so if the current thread is in the
> middle of a condition evaluation, only the current thread is resumed.
> 
> gdb/ChangeLog:
> 2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
> 	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> 	* infrun.c (user_visible_resume_ptid): In BP condition
> 	evaluation, resume only the current thread

This patch is definitely going to need a test.

Maybe some of the tests you add in later patch(es) cover this case,
but ideally tests should be included in the patch that fixes the
issue.

The reason why bundling tests with the patch is good thing is that if
in the future I'm run into a problem with this patch then I can easily
(git blame) find this patch.  If the tests are included in the same
commit then I know that if I modify this code then these specific
tests must still work.

But, I hear you say, surely I run all tests when changing GDB?  Yes,
but as GDB changes it might be that the test you add now no longer
exercises this code, thus, removing, or changing, this code might no
longer cause the tests you add to fail.  At that point I'm stuck, this
code is basically untested.  If I have the tests included in this
commit then I can start to dig in to figure out why this code is no
longer needed for those tests to pass.

> 
> Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
> ---
>  gdb/infrun.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index d552fb3adb2..91271c2ddbe 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2156,6 +2156,14 @@ user_visible_resume_ptid (int step)
>  	 mode.  */
>        resume_ptid = inferior_ptid;
>      }
> +  else if (inferior_ptid != null_ptid
> +	   && inferior_thread ()->control.in_cond_eval)
> +    {
> +      /* The inferior thread is evaluating a BP condition.  Other threads
> +	 might be stopped or running and we do not want to change their
> +	 state, thus, resume only the current thread.  */
> +      resume_ptid = inferior_ptid;
> +    }

I'm not sure about this.  We set threads going during an inferior call
so that any cross-thread interaction will not block.  At a minimum
this probably requires additional documentation.

I'd be interested to understand more about the different states that a
thread might be in here, and what conditions might put them into that
state.

If we assume a process with threads A, B, and C, then what happens is:

  1. Thread A bits conditional breakpoint, threads B & C are still
  running,

  2. GDB spots A has stopped and needs to execute the condition.

In this case we should only resume A as B & C are still running.  But,
we might also have:

  1. Thread A hits condition breakpoint, threads B & C are still
  running,

  2. Thread B hits condition breakpoint, thread C is still running,

  3. GDB spots A has stopped and needs to execute the condition.

In this case it's not clear what you would do... if you resume B to
evaluate A then you'll miss B's breakpoint hit.  But neither can you
evaluate B as that would require setting A running again.

Hmm, I think I've probably convinced myself that your approach might
be the only safe solution.

I think that I'd like to see the commit message expanded to give more
depth rather than just saying: "...some of them might still be shown
as running", I think you should say, what states a thread could be in
and why.  Also I think this probably is worth documenting in the
manual (that b/p conditions might be evaluated with some threads
stopped, even when running in all-stop mode).

Thanks,
Andrew

>    else if (!sched_multi && target_supports_multi_process ())
>      {
>        /* Resume all threads of the current process (and none of other
> -- 
> 2.17.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>