Re: [PATCH v2 1/6] gdb: add in_cond_eval field to thread_control_state struct.

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

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

> This patch adds a new field to thread_control_state struct, which
> is set to true when the thread is evaluating a breakpoint condition,
> and false otherwise.
> 
> gdb/ChangeLog:
> 2020-08-27  Natalia Saiapova  <natalia.saiapova@intel.com>
> 	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> 	* breakpoint.c (bpstat_check_breakpoint_conditions): Set/unset
> 	thread->control.in_cond_eval.
> 	* gdbthread.h (thread_control_state) <in_cond_eval>: New boolean field.
> 
> 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/breakpoint.c | 3 +++
>  gdb/gdbthread.h  | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 296b08c0afc..77f927dc645 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5209,10 +5209,13 @@ bpstat_check_breakpoint_conditions (bpstat bs, thread_info *thread)
>  	{
>  	  try
>  	    {
> +	      thread->control.in_cond_eval = true;
>  	      condition_result = breakpoint_cond_eval (cond);
> +	      thread->control.in_cond_eval = false;
>  	    }
>  	  catch (const gdb_exception &ex)
>  	    {
> +	      thread->control.in_cond_eval = false;
>  	      exception_fprintf (gdb_stderr, ex,
>  				 "Error in testing breakpoint condition:\n");
>  	    }

Instead of these three writes, would you consider:

	  try
	    {
	      scoped_restore save_in_cond_eval
		= make_scoped_restore (&thread->control.in_cond_eval, true);

	      condition_result = breakpoint_cond_eval (cond);
	    }
	  catch (const gdb_exception &ex)
	    {
	      exception_fprintf (gdb_stderr, ex,
				 "Error in testing breakpoint condition:\n");
	    }

This should be identical in functionality, but seems more inline with
GDB style, using RAII to handle restoring.

Thanks,
Andrew



> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index ab5771fdb47..9e93587f221 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -158,6 +158,9 @@ struct thread_control_state
>       command.  This is used to decide whether "set scheduler-locking
>       step" behaves like "on" or "off".  */
>    int stepping_command = 0;
> +
> +  /* True if the thread is evaluating a BP condition.  */
> +  bool in_cond_eval = false;
>  };
>  
>  /* Inferior thread specific part of `struct infcall_suspend_state'.  */
> -- 
> 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
>