From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id D7E61385843A for ; Thu, 3 Aug 2023 13:57:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D7E61385843A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1691071058; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=SXtfVfpD2+Ot6xnuDpCkMezYc05dnJcAfoc5jtpV8Ks=; b=buc4bI/zBnrgq5f0TxwYy11GpX2z7BD+IfMnHHgIOjZnYPvp2zpLdwXXnHj1TeH2HGeVwk 0BhZSAZEfY/iWBSsUN5SLgoStKQo5DVY0P71VfCfwsJlHaJkRz3/jcFPTLZxPaX0taSOpl SicZ27lXfY6LDLiwMLoyW5IBbz2OqqU= Received: from mail-ed1-f72.google.com (mail-ed1-f72.google.com [209.85.208.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-492-6f2fLwsUNNOslBlbE9wBdg-1; Thu, 03 Aug 2023 09:57:36 -0400 X-MC-Unique: 6f2fLwsUNNOslBlbE9wBdg-1 Received: by mail-ed1-f72.google.com with SMTP id 4fb4d7f45d1cf-5223854ef71so683195a12.1 for ; Thu, 03 Aug 2023 06:57:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691071055; x=1691675855; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=SXtfVfpD2+Ot6xnuDpCkMezYc05dnJcAfoc5jtpV8Ks=; b=IzWEZQ2LTJIRmYWsodeb9K4XeM90S+rCpzUEOlMJgIT+0bmNidQq1XXhExS/AGpz/8 V7koSReLncIb3xOz4W0N1/z3UcVD3Ic178zGCL/TNuaWKTslaDq/aJIitilV+pbQUkfq aoG8ruO2WbtPD9I6j683L69up+Ei6rf5j0xGgDFF8nINcfhdlecGRCDPAtN4gYntt3I3 34h43PpzTjL1tKGlfY63jwSEy7EqpwCF9zYsfK+v15HM2toC/tmLPfYL/yiIowt7xxbR b1t0sFCuZcvKin/bJfswBj1D8jvUJfo1fTR/rvfUdi3cF0rBiL8KcmQoSnlnVcufNazv 3wAA== X-Gm-Message-State: ABy/qLbmSVk5qOPV9ZVtQ4IpljmQTeVZnrVHFww1wfT/ahD19IQwMGal 235RzRRpWXRCiMosF5Csxkfncd/FGtuzA6BD5wyVTscR1JHOeUYoCrTcOa950MG/pImgLpioL/o w1Zbmp57lhk3Xg34eapirGZOJfWdJkQ== X-Received: by 2002:aa7:c706:0:b0:522:7ab4:31fe with SMTP id i6-20020aa7c706000000b005227ab431femr7490515edq.23.1691071054974; Thu, 03 Aug 2023 06:57:34 -0700 (PDT) X-Google-Smtp-Source: APBJJlGLw/O/ITsdveANoToKSmbXQ9iKTzWPhIv024QBLcC4Pd4XnTEVOJTPQqHecD2RNdK169RSDA== X-Received: by 2002:aa7:c706:0:b0:522:7ab4:31fe with SMTP id i6-20020aa7c706000000b005227ab431femr7490496edq.23.1691071054379; Thu, 03 Aug 2023 06:57:34 -0700 (PDT) Received: from localhost ([31.111.84.232]) by smtp.gmail.com with ESMTPSA id n10-20020aa7c78a000000b0052314366967sm132416eds.80.2023.08.03.06.57.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Aug 2023 06:57:33 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCHv5 05/11] gdb: don't always print breakpoint location after failed condition check In-Reply-To: References: <6fd4aa13-6003-2563-5841-e80d5a55d59e@palves.net> <796e9bc1-6272-3528-93b9-f1463e6b8156@palves.net> <87mt07ju8x.fsf@redhat.com> <4118312a-d86d-57d6-6a4b-8f0eb0f38300@palves.net> <87mszyit6o.fsf@redhat.com> Date: Thu, 03 Aug 2023 14:57:32 +0100 Message-ID: <87h6pgdy9f.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Pedro Alves writes: > Hi! > > Overall this looks good. Some nits below. > > On 2023-07-14 13:17, Andrew Burgess wrote: > >> commit bd3309a50423114c2cb31241500820efa21d0452 >> Author: Andrew Burgess >> 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 (¤t_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 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 . */ >> + >> +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 . >> + >> +# 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