Re: [PATCHv5 05/11] gdb: don't always print breakpoint location after failed condition check

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> Hi!
>
> Overall this looks good.  Some nits below.
>
> On 2023-07-14 13:17, Andrew Burgess wrote:
>
>> commit bd3309a50423114c2cb31241500820efa21d0452
>> Author: Andrew Burgess <aburgess@redhat.com>
>> Date:   Wed Jul 12 21:56:50 2023 +0100
>> 
>>     gdb: avoid double stop after failed breakpoint condition check
>>     
>>     This commit replaces this earlier commit:
>>     
>>       commit 2e411b8c68eb2b035b31d5b00d940d4be1a0928b
>>       Date:   Fri Oct 14 14:53:15 2022 +0100
>>     
>>           gdb: don't always print breakpoint location after failed condition check
>>     
>>     and is a result of feedback received here[1].
>>     
>>     The original commit addressed a problem where, if a breakpoint
>>     condition included an inferior function call, and if the inferior
>>     function call fail, then GDB would announce the stop twice.  Here's an
>
> "fail" -> "failed" or "fails", I guess.
>
>>     example of GDB's output before the above commit that shows the problem
>>     being addressed:
>>     
>>       (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)
>>     
>>     The original commit addressed this issue in breakpoint.c, by spotting
>>     that the $pc had changed while evaluating the breakpoint condition,
>>     and inferring from this that GDB must have stopped elsewhere.
>>     
>>     However, the way in which the original commit suppressed the second
>>     stop announcement was to set bpstat::print to true -- this tells GDB
>>     not to print the frame during the stop announcement, and for the CLI
>>     this is fine, however, it was pointed out that for the MI this still
>>     isn't really enough.  Below is an example from an MI session after the
>>     above commit was applied, this shows the problem with the above
>>     commit:
>>     
>>       -break-insert -c "cond_fail()" foo
>>       ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x000000000040111e",func="foo",file="/tmp/mi-condbreak-fail.c",line="30",thread-groups=["i1"],cond="cond_fail()",times="0",original-location="foo"}
>>       (gdb)
>>       -exec-run
>>       =thread-group-started,id="i1",pid="2636270"
>>       =thread-created,id="1",group-id="i1"
>>       =library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-group="i1",ranges=[{from="0x00007ffff7fd3110",to="0x00007ffff7ff2bb4"}]
>>       ^running
>>       *running,thread-id="all"
>>       (gdb)
>>       =library-loaded,id="/lib64/libm.so.6",target-name="/lib64/libm.so.6",host-name="/lib64/libm.so.6",symbols-loaded="0",thread-group="i1",ranges=[{from="0x00007ffff7e59390",to="0x00007ffff7ef4f98"}]
>>       =library-loaded,id="/lib64/libc.so.6",target-name="/lib64/libc.so.6",host-name="/lib64/libc.so.6",symbols-loaded="0",thread-group="i1",ranges=[{from="0x00007ffff7ca66b0",to="0x00007ffff7df3c5f"}]
>>       ~"\nProgram"
>>       ~" received signal SIGSEGV, Segmentation fault.\n"
>>       ~"0x0000000000401116 in cond_fail () at /tmp/mi-condbreak-fail.c:24\n"
>>       ~"24\t  return *p;\t\t\t/* Crash here.  */\n"
>>       *stopped,reason="signal-received",signal-name="SIGSEGV",signal-meaning="Segmentation fault",frame={addr="0x0000000000401116",func="cond_fail",args=[],file="/tmp/mi-condbreak-fail.c",fullname="/tmp/mi-condbreak-fail.c",line="24",arch="i386:x86-64"},thread-id="1",stopped-threads="all",core="9"
>>       &"Error in testing condition for breakpoint 1:\n"
>>       &"The program being debugged was signaled while in a function called from GDB.\n"
>>       &"GDB remains in the frame where the signal was received.\n"
>>       &"To change this behavior use \"set unwindonsignal on\".\n"
>>       &"Evaluation of the expression containing the function\n"
>>       &"(cond_fail) will be abandoned.\n"
>>       &"When the function is done executing, GDB will silently stop.\n"
>>       =breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x000000000040111e",func="foo",file="/tmp/mi-condbreak-fail.c",fullname="/tmp/mi-condbreak-fail.c",line="30",thread-groups=["i1"],cond="cond_fail()",times="1",original-location="foo"}
>>       *stopped
>>       (gdb)
>>     
>>     Notice that we still see two '*stopped' lines, the first includes the
>>     full frame information, while the second has no frame information,
>>     this is a result of bpstat::print having been set.  Ideally, the
>>     second '*stopped' line should not be present.
>>     
>>     By setting bpstat::print I was addressing the problem too late, this
>>     flag really only changes how interp::on_normal_stop prints the stop
>>     event, and interp::on_normal_stop is called (indirectly) from the
>>     normal_stop function in infrun.c.  A better solution is to avoid
>>     calling normal_stop at all for the stops which should not be reported
>>     to the user, and this is what I do in this commit.
>>     
>>     This commit has 3 parts:
>>     
>>       1. In breakpoint.c, revert the above commit,
>>     
>>       2. In fetch_inferior_event (infrun.c), capture the stop-id before
>>       calling handle_inferior_event.  If, after calling
>>       handle_inferior_event, the stop-id has changed, then this indicates
>>       that somewhere within handle_inferior_event, a stop was announced to
>>       the user.  If this is the case then GDB should not call normal_stop,
>>       and we should rely on whoever announced the stop to ensure that we
>>       are in a PROMPT_NEEDED state, which means the prompt will be
>>       displayed once fetch_inferior_event returns.  And,
>>     
>>       3. In infcall.c, do two things:
>>     
>>          (a) In run_inferior_call, after making the inferior call, ensure
>>          that either async_disable_stdin or async_enable_stdin is called
>>          to put the prompt state, and stdin handling into the correct
>>          state based on whether the inferior call completed successfully
>>          or not, and
>>     
>>          (b) In call_thread_fsm::should_stop, call async_enable_stdin
>>          rather than changing the prompt state directly.  This isn't
>>          strictly necessary, but helped me understand this code more.
>>          This async_enable_stdin call is only reached if normal_stop is
>>          not going to be called, and replaces the async_enable_stdin call
>>          that exists in normal_stop.  Though we could just adjust the
>>          prompt state if felt (to me) much easier to understand when I
>>          could see this call and the corresponding call in normal_stop.
>>     
>>     With these changes in place now, when the inferior call (from the
>>     breakpoint condition) fails, infcall.c leaves the prompt state as
>>     PROMPT_NEEDED, and leaves stdin registered with the event loop.
>>     
>>     Back in fetch_inferior_event GDB notices that the stop-id has changed
>>     and so avoids calling normal_stop.
>>     
>>     And on return from fetch_inferior_event GDB will display the prompt
>>     and handle input from stdin.
>>     
>>     As normal_stop is not called the MI problem is solved, and the test
>>     added in the earlier mentioned commit still passes just fine, so the
>>     CLI has not regressed.
>>     
>>     [1] https://inbox.sourceware.org/gdb-patches/6fd4aa13-6003-2563-5841-e80d5a55d59e@palves.net/
>> 
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index d898167b7e1..93634bd7e51 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -5535,7 +5535,6 @@ 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
>> @@ -5555,17 +5554,6 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>>  		  (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/infcall.c b/gdb/infcall.c
>> index ddf325a62a5..0944097e18d 100644
>> --- a/gdb/infcall.c
>> +++ b/gdb/infcall.c
>> @@ -564,10 +564,13 @@ call_thread_fsm::should_stop (struct thread_info *thread)
>>  	 call..  */
>>        return_value = get_call_return_value (&return_meta_info);
>>  
>> -      /* Break out of wait_sync_command_done.  */
>> +      /* Break out of wait_sync_command_done.  This is similar to the
>> +	 async_enable_stdin call in normal_stop (which we don't call),
>> +	 however, in this case we only change the WAITING_UI, this is
>
> Full stop after WAITING_UI.
>
>> +	 enough for wait_sync_command_done.  */
>>        scoped_restore save_ui = make_scoped_restore (&current_ui, waiting_ui);
>> -      target_terminal::ours ();
>> -      waiting_ui->prompt_state = PROMPT_NEEDED;
>> +      gdb_assert (current_ui->prompt_state == PROMPT_BLOCKED);
>> +      async_enable_stdin ();
>>      }
>>  
>>    return true;
>> @@ -661,14 +664,30 @@ run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
>>    infcall_debug_printf ("thread is now: %s",
>>  			inferior_ptid.to_string ().c_str ());
>>  
>> -  /* If GDB has the prompt blocked before, then ensure that it remains
>> -     so.  normal_stop calls async_enable_stdin, so reset the prompt
>> -     state again here.  In other cases, stdin will be re-enabled by
>> -     inferior_event_handler, when an exception is thrown.  */
>> +  /* After the inferior call async_enable_stdin has been called, either
>> +     from normal_stop or from call_thread_fsm::should_stop, and the prompt
>> +     state has been restored by the scoped_restore in the try block above.
>
> It took me several reads to understand this comment.  I kept misreading this
> as "After ... async_enable_stdin has been called, then ..."
>
> I think putting a comma here
>
>  /* After the inferior call, async_enable_stdin has been called, either
>                           ^^^
>
> would have helped me.  Maybe even say:
>
>    "After the inferior call finishes, ".
>
> ?
>
>> +
>> +     If the inferior call finished successfully then we should disable
>> +     stdin as we don't know yet whether the inferior will be stopping,
>
> The last comma should be a period, IMO.
>
>> +     calling async_disable_stdin restores things as they were when this
>> +     function was called.
>
> "restores things as they were" -> "restores things to how they were" ?
>
>> +
>> +     If the inferior call didn't complete successfully then normal_stop has
>
> Comma before "then".
>
>> +     already been called, and we know for sure that we are going to present
>> +     this stop to the user, in this case we call async_enable_stdin, this
>
> Period after "the user" instead of comma.  Comma after "this case".
> Also period after async_enable_stdin; alternatively, say "which" instead of
> "this".
>
>> +     changes the prompt state to PROMPT_NEEDED.
>> +
>> +     If the previous prompt state was PROMPT_NEEDED then, as
>
> I think you mean to put the comma before "then" instead of after?
> Or is this "then" refering to some previously-referred-to situation?
>
>> +     async_enable_stdin has already been called, nothing additional needs
>> +     to be done here.  */
>
> I found it much easier to understand the whole comment block without applying
> my suggestions above locally.  Here is the result for your reference, and to
> double check I'm parsing this all correctly:
>
>   /* After the inferior call finished, async_enable_stdin has been
>      called, either from normal_stop or from
>      call_thread_fsm::should_stop, and the prompt state has been
>      restored by the scoped_restore in the try block above.
>
>      If the inferior call finished successfully, then we should
>      disable stdin as we don't know yet whether the inferior will be
>      stopping.  Calling async_disable_stdin restores things to how
>      they were when this function was called.
>
>      If the inferior call didn't complete successfully, then
>      normal_stop has already been called, and we know for sure that we
>      are going to present this stop to the user.  In this case, we
>      call async_enable_stdin.  This changes the prompt state to
>      PROMPT_NEEDED.
>
>      If the previous prompt state was PROMPT_NEEDED, then as
>      async_enable_stdin has already been called, nothing additional
>      needs to be done here.  */
>
>
>>    if (current_ui->prompt_state == PROMPT_BLOCKED)
>> -    current_ui->unregister_file_handler ();
>> -  else
>> -    current_ui->register_file_handler ();
>> +    {
>> +      if (call_thread->thread_fsm ()->finished_p ())
>> +	async_disable_stdin ();
>> +      else
>> +	async_enable_stdin ();
>> +    }
>>  
>>    /* If the infcall does NOT succeed, normal_stop will have already
>>       finished the thread states.  However, on success, normal_stop
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 58da1cef29e..95ab7c6f7dc 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -4395,6 +4395,8 @@ fetch_inferior_event ()
>>      auto defer_delete_threads
>>        = make_scope_exit (delete_just_stopped_threads_infrun_breakpoints);
>>  
>> +    int stop_id = get_stop_id ();
>> +
>>      /* Now figure out what to do with the result of the result.  */
>>      handle_inferior_event (&ecs);
>>  
>> @@ -4422,7 +4424,19 @@ fetch_inferior_event ()
>>  
>>  	    clean_up_just_stopped_threads_fsms (&ecs);
>>  
>> -	    if (thr != nullptr && thr->thread_fsm () != nullptr)
>> +	    if (stop_id != get_stop_id ())
>> +	      {
>> +		/* If the stop-id has changed then a stop has already been
>> +		   presented to the user in handle_inferior_event, this is
>> +		   likely a failed inferior call.  As the stop has already
>> +		   been announced then we should not notify again.
>> +
>> +		   Also, if the prompt state is not PROMPT_NEEDED then GDB
>> +		   will not be ready for user input after this function.  */
>> +		should_notify_stop = false;
>> +		gdb_assert (current_ui->prompt_state == PROMPT_NEEDED);
>> +	      }
>> +	    else if (thr != nullptr && thr->thread_fsm () != nullptr)
>>  	      should_notify_stop
>>  	       = thr->thread_fsm ()->should_notify_stop ();
>>  
>> diff --git a/gdb/testsuite/gdb.mi/mi-condbreak-fail.c b/gdb/testsuite/gdb.mi/mi-condbreak-fail.c
>> new file mode 100644
>> index 00000000000..94bd2484934
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.mi/mi-condbreak-fail.c
>> @@ -0,0 +1,39 @@
>> +/* Copyright 2023 Free Software Foundation, Inc.
>> +
>> +   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/>.  */
>> +
>> +volatile int global_counter = 0;
>> +
>> +int
>> +cond_fail ()
>> +{
>> +  volatile int *p = 0;
>> +  return *p;			/* Crash here.  */
>> +}
>> +
>> +int
>> +foo ()
>> +{
>> +  global_counter += 1;		/* Set breakpoint here.  */
>> +  return 0;
>> +}
>> +
>> +int
>> +main ()
>> +{
>> +  int res = foo ();
>> +  return res;
>> +}
>> diff --git a/gdb/testsuite/gdb.mi/mi-condbreak-fail.exp b/gdb/testsuite/gdb.mi/mi-condbreak-fail.exp
>> new file mode 100644
>> index 00000000000..86b1db5a8dd
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.mi/mi-condbreak-fail.exp
>> @@ -0,0 +1,67 @@
>> +# Copyright (C) 2023 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/>.
>> +
>> +# Check that when GDB fails to evaluate the condition of a conditional
>> +# breakpoint we only get one *stopped message.
>
> "message" -> "async record" or "notification".
>
>> +
>> +load_lib mi-support.exp
>> +set MIFLAGS "-i=mi"
>> +
>> +standard_testfile
>> +
>> +if [build_executable ${testfile}.exp ${binfile} ${srcfile}] {
>> +    return -1
>> +}
>> +
>> +if {[mi_clean_restart $binfile]} {
>> +    return
>> +}
>> +
>> +if {[mi_runto_main] == -1} {
>> +    return
>> +}
>> +
>> +# Create the conditional breakpoint.
>> +set bp_location [gdb_get_line_number "Set breakpoint here"]
>> +mi_create_breakpoint "-c \"cond_fail ()\" $srcfile:$bp_location" \
>> +    "insert conditional breakpoint" \
>> +    -func foo -file ".*$srcfile" -line "$bp_location" \
>> +    -cond "cond_fail \\(\\)"
>> +
>> +# Number of the previous breakpoint.
>> +set bpnum [mi_get_valueof "/d" "\$bpnum" "INVALID" \
>> +	       "get number for breakpoint"]
>> +
>> +# The line where we expect the inferior to crash.
>> +set crash_linenum [gdb_get_line_number "Crash here"]
>> +
>> +# Run the inferior and wait for it to stop.
>> +mi_send_resuming_command "exec-continue" "continue the inferior"
>> +mi_gdb_test "" \
>> +    [multi_line \
>> +	 "~\"\\\\nProgram\"" \
>> +	 "~\" received signal SIGSEGV, Segmentation fault\\.\\\\n\"" \
>> +	 "~\"$hex in cond_fail \\(\\) at \[^\r\n\]+\"" \
>> +	 "~\"${crash_linenum}\\\\t\\s+return \\*p;\[^\r\n\]+\\\\n\"" \
>> +	 "\\*stopped,reason=\"signal-received\",signal-name=\"SIGSEGV\"\[^\r\n\]+" \
>> +	 "&\"Error in testing condition for breakpoint $bpnum:\\\\n\"" \
>> +	 "&\"The program being debugged was signaled while in a function called from GDB\\.\\\\n\"" \
>> +	 "&\"GDB remains in the frame where the signal was received\\.\\\\n\"" \
>> +	 "&\"To change this behavior use \\\\\"set unwindonsignal on\\\\\"\\.\\\\n\"" \
>> +	 "&\"Evaluation of the expression containing the function\\\\n\"" \
>> +	 "&\"\\(cond_fail\\) will be abandoned\\.\\\\n\"" \
>> +	 "&\"When the function is done executing, GDB will silently stop\\.\\\\n\"" \
>> +	 "=breakpoint-modified,bkpt={number=\"$bpnum\",type=\"breakpoint\",.*}"] \
>
> I wonder about matching times="1" too, to confirm we're incrementing the hit
> count even if the condition fails.
>
> Also, that ".*" in the last line gave me pause.  It makes it so that a future
> change that results in an output like:
>
>   =breakpoint-modified,bkpt={number=\"$bpnum\",type=\"breakpoint\", [snip]
>   *stopped
>   =something-else-that-ends-in-curly-braces,{...}
>
> would still pass.  Maybe use "\[^\r\n\]*" instead.
>
> With the nits fixed, this LGTM.

I made the fixes you suggested and pushed this patch.

Thanks,
Andrew