RE: [PATCHv2 06/13] gdb: avoid repeated signal reporting during failed conditional breakpoint

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

On Wednesday, January 18, 2023 5:18 PM, Andrew Burgess wrote:
> Consider the following case:
> 
>   (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 breakpoint condition:
>   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
>   (some_func) will be abandoned.
>   When the function is done executing, GDB will silently stop.
> 
>   Program received signal SIGSEGV, Segmentation fault.
> 
>   Breakpoint 1, 0x0000000000401116 in some_func () at bpcond.c:5
>   5	  return *p;
>   (gdb)
> 
> Notice that this line:
> 
>   Program received signal SIGSEGV, Segmentation fault.
> 
> Appears twice in the output.  The first time is followed by the
> current location.  The second time is a little odd, why do we print
> that?
> 
> Printing that line is controlled, in part, by a global variable,
> stopped_by_random_signal.  This variable is reset to zero in
> handle_signal_stop, and is set if/when GDB figures out that the
> inferior stopped due to some random signal.
> 
> The problem is, in our case, GDB first stops at the breakpoint for
> foo, and enters handle_signal_stop and the stopped_by_random_signal
> global is reset to 0.
> 
> Later within handle_signal_stop GDB calls bpstat_stop_status, it is
> within this function (via bpstat_check_breakpoint_conditions) that the
> breakpoint condition is checked, and, we end up calling the inferior
> function (some_func in our example above).
> 
> In our case above the thread performing the inferior function call
> segfaults in some_func.  GDB catches the SIGSEGV and handles the stop,
> this causes us to reenter handle_signal_stop.  The global variable
> stopped_by_random_signal is updated, this time it is set to true
> because the thread stopped due to SIGSEGV.  As a result of this we
> print the first instance of the line (as seen above in the example).
> 
> Finally we unwind GDB's call stack, the inferior function call is
> complete, and we return to the original handle_signal_stop.  However,
> the stopped_by_random_signal global is still carrying the value as
> computed for the inferior function call's stop, which is why we now
> print a second instance of the line, as seen in the example.
> 
> To prevent this, I propose adding a scoped_restore before we start an
> inferior function call, this will save and restore the global
> stopped_by_random_signal value.

Nit: It'd read better, I think, if this read
"... function call. This will save ..."
 
> With this done, the output from our example is now 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.
> 
>   Breakpoint 1, 0x0000000000401116 in some_func () at bpcond.c:5
>   5	  return *p;
>   (gdb)
> 
> We now only see the 'Program received signal SIGSEGV, ...' line once,
> which I think makes more sense.
> 
> Finally, I'm aware that the last few lines, that report the stop as
> being at 'Breakpoint 1', when this is not where the thread is actually
> located anymore, is not great.  I'll address that in the next commit.
> ---
>  gdb/infcall.c                              |   9 +
>  gdb/testsuite/gdb.base/infcall-failure.c   |  48 ++++++
>  gdb/testsuite/gdb.base/infcall-failure.exp | 184 +++++++++++++++++++++
>  3 files changed, 241 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/infcall-failure.c
>  create mode 100644 gdb/testsuite/gdb.base/infcall-failure.exp
> 
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index e09904f9a35..e1b785e437b 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -1296,6 +1296,15 @@ call_function_by_hand_dummy (struct value *function,
>    /* Register a clean-up for unwind_on_terminating_exception_breakpoint.  */
>    SCOPE_EXIT { delete_std_terminate_breakpoint (); };
> 
> +  /* The stopped_by_random_signal variable is global.  If we are here
> +     as part of a breakpoint condition check then the global will have
> +     already been setup as part of the original breakpoint stop.  By
> +     making the inferior call the global will be changed when GDB
> +     handles the stop after the inferior call.  Avoid confusion by
> +     restoring the current value after the inferior call.  */
> +  scoped_restore restore_stopped_by_random_signal
> +    = make_scoped_restore (&stopped_by_random_signal, 0);
> +
>    /* - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP -
>       If you're looking to implement asynchronous dummy-frames, then
>       just below is the place to chop this function in two..  */
> diff --git a/gdb/testsuite/gdb.base/infcall-failure.c b/gdb/testsuite/gdb.base/infcall-
> failure.c
> new file mode 100644
> index 00000000000..00f4369e164
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/infcall-failure.c
> @@ -0,0 +1,48 @@
> +/* Copyright 2022 Free Software Foundation, Inc.

2022-2023?

> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +/* A function that segfaults (assuming that reads of address zero are
> +   prohibited), this is used from within a breakpoint condition.  */
> +int
> +func_segfault ()
> +{
> +  volatile int *p = 0;
> +  return *p;	/* Segfault here.  */
> +}
> +
> +/* A function in which we will place a breakpoint.  This function is itself
> +   then used from within a breakpoint condition.  */
> +int
> +func_bp ()
> +{
> +  int res = 0;	/* Second breakpoint.  */
> +  return res;
> +}
> +
> +int
> +foo ()
> +{
> +  return 0;	/* First breakpoint.  */
> +}
> +
> +int
> +main ()
> +{
> +  int res = foo ();
> +
> +  return res;
> +}
> diff --git a/gdb/testsuite/gdb.base/infcall-failure.exp b/gdb/testsuite/gdb.base/infcall-
> failure.exp
> new file mode 100644
> index 00000000000..2dcdda34b4d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/infcall-failure.exp
> @@ -0,0 +1,184 @@
> +# Copyright 2022 Free Software Foundation, Inc.

2022-2023?

> +
> +# 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/>.
> +
> +# Some simple tests of inferior function calls from breakpoint
> +# conditions, in a single-threaded inferior.
> +#
> +# Test what happens when the inferior function (from a breakpoint
> +# condition) either hits a nested breakpoint, or segfaults.
> +
> +standard_testfile
> +
> +if { [build_executable "failed to prepare" ${binfile} "${srcfile}" \
> +	  {debug}] == -1 } {
> +    return
> +}
> +
> +set bp_1_line [gdb_get_line_number "First breakpoint"]
> +set bp_2_line [gdb_get_line_number "Second breakpoint"]
> +set segv_line [gdb_get_line_number "Segfault here"]
> +
> +# Start GDB based on TARGET_ASYNC and TARGET_NON_STOP, and then runto
> +# main.
> +proc start_gdb_and_runto_main { target_async target_non_stop } {
> +    save_vars { ::GDBFLAGS } {
> +	append ::GDBFLAGS \
> +	    " -ex \"maint set target-non-stop $target_non_stop\""
> +	append ::GDBFLAGS \
> +	    " -ex \"maintenance set target-async ${target_async}\""
> +
> +	clean_restart ${::binfile}
> +    }
> +
> +    if {![runto_main]} {

Other if-statements in this file put spaces around the condition.

> +	fail "run to main"

Commit 4dfef5be6812dd7abfbc8f18e9c0384f2522b511 ("gdb/testsuite: make
runto_main not pass no-message to runto") eliminated the need to emit a 
fail explicitly. 

> +	return -1
> +    }
> +
> +    return 0
> +}
> +
> +# Start GDB according to ASYNC_P and NON_STOP_P, then setup a
> +# conditional breakpoint.  The breakpoint condition includes an
> +# inferior function call that will itself hit a breakpoint.  Check how
> +# GDB reports this to the user.
> +proc_with_prefix run_cond_hits_breakpoint_test { async_p non_stop_p } {
> +    if { [start_gdb_and_runto_main $async_p $non_stop_p] == -1 } {
> +	return
> +    }
> +
> +    # Setup the conditional breakpoint and record its number.
> +    gdb_breakpoint "${::srcfile}:${::bp_1_line} if (func_bp ())"
> +    set bp_1_num [get_integer_valueof "\$bpnum" "*UNKNOWN*" \
> +		     "get number of first breakpoint"]
> +
> +    # Setup a breakpoint inside func_bp.
> +    gdb_breakpoint "${::srcfile}:${::bp_2_line}"
> +    set bp_2_num [get_integer_valueof "\$bpnum" "*UNKNOWN*" \
> +		     "get number of second breakpoint"]
> +
> +    gdb_test "continue" \
> +	[multi_line \
> +	     "Continuing\\." \
> +	     "" \
> +	     "Breakpoint ${bp_2_num}, func_bp \\(\\) at \[^\r\n\]+:${::bp_2_line}" \
> +	     "${::decimal}\\s+\[^\r\n\]+Second breakpoint\[^\r\n\]+" \
> +	     "Error in testing condition for breakpoint ${bp_1_num}:" \
> +	     "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\]+"]
> +}
> +
> +# Start GDB according to ASYNC_P and NON_STOP_P, then call an inferior
> +# function.  The inferior function being called will itself have a
> +# breakpoint within it.  Check how GDB reports this to the user.
> +proc_with_prefix run_call_hits_breakpoint_test { async_p non_stop_p } {
> +    if { [start_gdb_and_runto_main $async_p $non_stop_p] == -1 } {
> +	return
> +    }
> +
> +    # Setup a breakpoint inside func_bp.
> +    gdb_breakpoint "${::srcfile}:${::bp_2_line}"
> +    set bp_2_num [get_integer_valueof "\$bpnum" "*UNKNOWN*" \
> +		      "get number of second breakpoint"]
> +
> +
> +    gdb_test "call func_bp ()" \
> +	[multi_line \
> +	     "" \
> +	     "Breakpoint ${bp_2_num}, func_bp \\(\\) at \[^\r\n\]+:${::bp_2_line}" \
> +	     "${::decimal}\\s+\[^\r\n\]+Second breakpoint\[^\r\n\]+" \
> +	     "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\\."]
> +}
> +
> +# Start GDB according to ASYNC_P and NON_STOP_P, then setup a
> +# conditional breakpoint.  The breakpoint condition includes an
> +# inferior function call that segfaults.  Check how GDB reports this
> +# to the user.
> +proc_with_prefix run_cond_hits_segfault_test { async_p non_stop_p } {
> +    if { [start_gdb_and_runto_main $async_p $non_stop_p] == -1 } {
> +	return
> +    }
> +
> +    # This test relies on the inferior segfaulting when trying to
> +    # access address zero.
> +    if { [is_address_zero_readable] } {

Maybe emit an UNTESTED in this case?

> +	return
> +    }
> +
> +    # Setup the conditional breakpoint and record its number.
> +    gdb_breakpoint "${::srcfile}:${::bp_1_line} if (func_segfault ())"
> +    set bp_1_num [get_integer_valueof "\$bpnum" "*UNKNOWN*" \
> +		     "get number of first breakpoint"]
> +
> +    gdb_test "continue" \
> +	[multi_line \
> +	     "Continuing\\." \
> +	     "" \
> +	     "Program received signal SIGSEGV, Segmentation fault\\." \
> +	     "${::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\\." \
> +	     "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\]+"]
> +}
> +
> +# Start GDB according to ASYNC_P and NON_STOP_P, then call an inferior
> +# function.  The inferior function will segfault.  Check how GDB
> +# reports this to the user.
> +proc_with_prefix run_call_hits_segfault_test { async_p non_stop_p } {
> +    if { [start_gdb_and_runto_main $async_p $non_stop_p] == -1 } {
> +	return
> +    }
> +
> +    # This test relies on the inferior segfaulting when trying to
> +    # access address zero.
> +    if { [is_address_zero_readable] } {

Here, too.

> +	return
> +    }
> +
> +    gdb_test "call func_segfault ()" \
> +	[multi_line \
> +	     "" \
> +	     "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\\." \
> +	     "Evaluation of the expression containing the function" \
> +	     "\\(func_segfault\\) will be abandoned\\." \
> +	     "When the function is done executing, GDB will silently stop\\."]
> +}
> +
> +foreach_with_prefix target_async { "on" "off" } {
> +    foreach_with_prefix target_non_stop { "on" "off" } {
> +	run_cond_hits_breakpoint_test $target_async $target_non_stop
> +	run_call_hits_breakpoint_test $target_async $target_non_stop
> +
> +	run_cond_hits_segfault_test $target_async $target_non_stop
> +	run_call_hits_segfault_test $target_async $target_non_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