RE: [PATCHv2 07/13] gdb: don't always print breakpoint location after failed condition check

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

On Wednesday, January 18, 2023 5:18 PM, Andrew Burgess wrote:
> Consider the following session:
> 
>   (gdb) list some_func
>   1	int
>   2	some_func ()
>   3	{
>   4	  int *p = 0;
>   5	  return *p;
>   6	}
>   7
>   8	void
>   9	foo ()
>   10	{
>   (gdb) break foo if (some_func ())
>   Breakpoint 1 at 0x40111e: file bpcond.c, line 11.
>   (gdb) r
>   Starting program: /tmp/bpcond
> 
>   Program received signal SIGSEGV, Segmentation fault.
>   0x0000000000401116 in some_func () at bpcond.c:5
>   5	  return *p;
>   Error in testing condition for breakpoint 1:
>   The program being debugged stopped while in a function called from GDB.
>   Evaluation of the expression containing the function
>   (some_func) will be abandoned.
>   When the function is done executing, GDB will silently stop.
> 
>   Breakpoint 1, 0x0000000000401116 in some_func () at bpcond.c:5
>   5	  return *p;
>   (gdb)
> 
> What happens here is the breakpoint condition includes a call to an
> inferior function, and the inferior function segfaults.  We can see
> that GDB reports the segfault, and then gives an error message that
> indicates that an inferior function call was interrupted.
> 
> After this GDB appears to report that it is stopped at Breakpoint 1,
> inside some_func.
> 
> I find this second stop report a little confusing.  Yes, GDB has
> stopped as a result of hitting breakpoint 1, but, I think the message
> as it currently is might give the impression that the thread is

Something is missing in this sentence, I think.  Did you mean
"I think the message it currently prints might give..."?

> actually stopped at a location of breakpoint 1, which is not the case.
> 
> Also, I find the second stop message draws attention away from
> the "Program received signal SIGSEGV, Segmentation fault" stop
> message, and this second stop might be thought of as replacing in
> someway the earlier message.
> 
> In short, I think that the in the situation above, I think things

Please consider revising this as 
"In short, in the situation above, I think things".

> would be clearer if the second stop message were not reported at all,
> so the output should (I think) look like this:
> 
>   (gdb) list some_func
>   1	int
>   2	some_func ()
>   3	{
>   4	  int *p = 0;
>   5	  return *p;
>   6	}
>   7
>   8	void
>   9	foo ()
>   10	{
>   (gdb) break foo if (some_func ())
>   Breakpoint 1 at 0x40111e: file bpcond.c, line 11.
>   (gdb) r
>   Starting program: /tmp/bpcond
> 
>   Program received signal SIGSEGV, Segmentation fault.
>   0x0000000000401116 in some_func () at bpcond.c:5
>   5	  return *p;
>   Error in testing condition for breakpoint 1:
>   The program being debugged stopped while in a function called from GDB.
>   Evaluation of the expression containing the function
>   (some_func) will be abandoned.
>   When the function is done executing, GDB will silently stop.
>   (gdb)
> 
> The user can still find the number of the breakpoint that triggered
> the initial stop in this line:
> 
>   Error in testing condition for breakpoint 1:
> 
> But there's now only one stop reason reported, the SIGSEGV, which I
> think is much clearer.

FWIW, I agree to this.

> To achieve this change I set the bpstat::print field when:
> 
>   (a) a breakpoint condition evaluation failed, and
> 
>   (b) the $pc of the thread changed during condition evaluation.
> 
> I've updated the existing tests that checked the error message printed
> when a breakpoint condition evaluation failed.
> ---
>  gdb/breakpoint.c                           | 12 ++++++++++++
>  gdb/testsuite/gdb.base/infcall-failure.exp | 18 ++++++++----------
>  2 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index eecaeefed3e..c4fec7e8e55 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5533,6 +5533,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>  	  else
>  	    within_current_scope = false;
>  	}
> +      CORE_ADDR pc_before_check = get_frame_pc (get_selected_frame (nullptr));
>        if (within_current_scope)
>  	{
>  	  try
> @@ -5544,6 +5545,17 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>  	      exception_fprintf (gdb_stderr, ex,
>  				 "Error in testing condition for breakpoint %d:\n",
>  				 b->number);
> +
> +	      /* If the pc value changed as a result of evaluating the
> +		 condition then we probably stopped within an inferior
> +		 function call due to some unexpected stop, e.g. the thread
> +		 hit another breakpoint, or the thread received an
> +		 unexpected signal.  In this case we don't want to also
> +		 print the information about this breakpoint.  */
> +	      CORE_ADDR pc_after_check
> +		= get_frame_pc (get_selected_frame (nullptr));
> +	      if (pc_before_check != pc_after_check)
> +		bs->print = 0;
>  	    }
>  	}
>        else
> diff --git a/gdb/testsuite/gdb.base/infcall-failure.exp b/gdb/testsuite/gdb.base/infcall-
> failure.exp
> index 2dcdda34b4d..8fa2c407fc5 100644
> --- a/gdb/testsuite/gdb.base/infcall-failure.exp
> +++ b/gdb/testsuite/gdb.base/infcall-failure.exp
> @@ -79,10 +79,7 @@ proc_with_prefix run_cond_hits_breakpoint_test { async_p non_stop_p } {
>  	     "The program being debugged stopped while in a function called from GDB\\." \
>  	     "Evaluation of the expression containing the function" \
>  	     "\\(func_bp\\) will be abandoned\\." \
> -	     "When the function is done executing, GDB will silently stop\\." \
> -	     "" \
> -	     "Breakpoint ${bp_1_num}, \[^\r\n\]+" \
> -	     "${::decimal}\\s+\[^\r\n\]+Second breakpoint\[^\r\n\]+"]
> +	     "When the function is done executing, GDB will silently stop\\."]
>  }
> 
>  # Start GDB according to ASYNC_P and NON_STOP_P, then call an inferior
> @@ -138,13 +135,12 @@ proc_with_prefix run_cond_hits_segfault_test { async_p non_stop_p } {
>  	     "${::hex} in func_segfault \\(\\) at \[^\r\n\]+:${::segv_line}" \
>  	     "${::decimal}\\s+\[^\r\n\]+Segfault here\[^\r\n\]+" \
>  	     "Error in testing condition for breakpoint ${bp_1_num}:" \
> -	     "The program being debugged stopped while in a function called from GDB\\." \
> +	     "The program being debugged was signaled while in a function called from GDB\\."
> \
> +	     "GDB remains in the frame where the signal was received\\." \
> +	     "To change this behavior use \"set unwindonsignal on\"\\." \
>  	     "Evaluation of the expression containing the function" \
>  	     "\\(func_segfault\\) will be abandoned\\." \
> -	     "When the function is done executing, GDB will silently stop\\." \
> -	     "" \
> -	     "Breakpoint ${bp_1_num}, \[^\r\n\]+" \
> -	     "${::decimal}\\s+\[^\r\n\]+Segfault here\[^\r\n\]+"]
> +	     "When the function is done executing, GDB will silently stop\\."]
>  }
> 
>  # Start GDB according to ASYNC_P and NON_STOP_P, then call an inferior
> @@ -167,7 +163,9 @@ proc_with_prefix run_call_hits_segfault_test { async_p non_stop_p } {
>  	     "Program received signal SIGSEGV, Segmentation fault\\." \
>  	     "${::hex} in func_segfault \\(\\) at \[^\r\n\]+:${::segv_line}" \
>  	     "${::decimal}\\s+\[^\r\n\]+Segfault here\[^\r\n\]+" \
> -	     "The program being debugged stopped while in a function called from GDB\\." \
> +	     "The program being debugged was signaled while in a function called from GDB\\."
> \
> +	     "GDB remains in the frame where the signal was received\\." \
> +	     "To change this behavior use \"set unwindonsignal on\"\\." \
>  	     "Evaluation of the expression containing the function" \
>  	     "\\(func_segfault\\) will be abandoned\\." \
>  	     "When the function is done executing, GDB will silently stop\\."]
> --
> 2.25.4

Regards
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928