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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTPS id 984CF385781E for ; Wed, 10 Nov 2021 03:34:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 984CF385781E Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-589-SAiSp6QDOMeTW_3-D0KY6A-1; Tue, 09 Nov 2021 22:34:28 -0500 X-MC-Unique: SAiSp6QDOMeTW_3-D0KY6A-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2BD301006AA1; Wed, 10 Nov 2021 03:34:27 +0000 (UTC) Received: from f35-m3 (ovpn-112-180.phx2.redhat.com [10.3.112.180]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 04D0867842; Wed, 10 Nov 2021 03:34:26 +0000 (UTC) Date: Tue, 9 Nov 2021 20:34:26 -0700 From: Kevin Buettner To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] Fix PR 28308 - dprintf breakpoints not working when run from script Message-ID: <20211109203426.0656c9ea@f35-m3> In-Reply-To: References: <20211002010054.1546736-1-kevinb@redhat.com> <20211002010054.1546736-2-kevinb@redhat.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Nov 2021 03:34:31 -0000 On Fri, 5 Nov 2021 12:05:02 -0400 Simon Marchi wrote: >... > > Yet another thing to be aware of regarding this problem is the > > difference in the way that commands associated dprintf breakpoints > > associated *to* dprintf breakpoints Fixed. [...] > Not really related to your patch, but reading your patch made me wonder > if it's really necessary to implement dprintfs using pre-generated > command lines. It just looks like a lot of unnecessary layers (one of > which you removed, calling bpstat_do_actions_1) that are hard to follow. > Couldn't we just do the necessary actions in > dprintf_after_condition_true based on the current settings? Not by > building strings of GDB commands and evaluating them, but just with > regular C++ code? Another way of implementing it would have been to associate three commands with a dprintf breakpoint: quiet printf ... / call (void) printf (...) / agent-print ... continue This is basically what you'd use as a breakpoint command list if GDB didn't have the dprintf facility. The virtue of this approach is that it could use the existing breakpoint w/ associated command logic without having to introduce some of the stuff that's only used by the dprintf mechanism. This current bug either wouldn't have happened or, if it did, would have affected the rest of the breakpoint/command functionality as well. I think it's probable that this approach (that I'm suggesting) was considered but was found lacking in some way. If so, I'd guess that either there's some major "gotcha" which I have considered or else it's too slow. > > --- > > gdb/breakpoint.c | 13 ++++--------- > > 1 file changed, 4 insertions(+), 9 deletions(-) > > > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > > index 10b28c97be7..0ca3995894e 100644 > > --- a/gdb/breakpoint.c > > +++ b/gdb/breakpoint.c > > @@ -13029,9 +13029,6 @@ dprintf_print_recreate (struct breakpoint *tp, struct ui_file *fp) > > static void > > dprintf_after_condition_true (struct bpstats *bs) > > { > > - struct bpstats tmp_bs; > > - struct bpstats *tmp_bs_p = &tmp_bs; > > - > > /* dprintf's never cause a stop. This wasn't set in the > > check_status hook instead because that would make the dprintf's > > condition not be evaluated. */ > > @@ -13042,14 +13039,12 @@ dprintf_after_condition_true (struct bpstats *bs) > > bpstat_do_actions, if a breakpoint that causes a stop happens to > > be set at same address as this dprintf, or even if running the > > commands here throws. */ > > - tmp_bs.commands = bs->commands; > > + struct command_line *cmds = (bs->commands == NULL) > > + ? NULL > > + : bs->commands.get (); > > bs->commands = NULL; > > > > - bpstat_do_actions_1 (&tmp_bs_p); > > - > > - /* 'tmp_bs.commands' will usually be NULL by now, but > > - bpstat_do_actions_1 may return early without processing the whole > > - list. */ > > + execute_control_commands (cmds, 0); > > Given that bs->commands (the counted_command_line type, actually) is a > shared pointer, this would be more straightforward, and make it clear > that we steal the ownership of the commands, as the comment explains: > > counted_command_line cmds = std::move (bs->commands); > gdb_assert (cmds != nullptr); > execute_control_commands (cmds.get (), 0); > > > I put a gdb_assert in there, because from my understanding there will > always be at least one command in a dprintf. But if that's not true, > then it should be a condition. I've made this change too. I've pushed this commit along with the commit for the new tests. Thanks for the review! Kevin