From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id hsfzB/VKZWOLExQAWB0awg (envelope-from ) for ; Fri, 04 Nov 2022 13:25:09 -0400 Received: by simark.ca (Postfix, from userid 112) id 15C921E124; Fri, 4 Nov 2022 13:25:09 -0400 (EDT) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=tLMhE63t; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.3 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A, RCVD_IN_DNSWL_MED,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 53D0F1E0D3 for ; Fri, 4 Nov 2022 13:25:08 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E44FD3858424 for ; Fri, 4 Nov 2022 17:25:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E44FD3858424 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1667582702; bh=yaQ9cvZ1hsoVXVQJ9iUd16gC1rQHTryPIgdRGgE+PG4=; h=Date:Subject:To:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=tLMhE63toD8tpbKKpFpekquV76ouoUHW39sMA7vAbk26JhMvjFsKO70uF9P9l8pp/ 3COzEea5Bvj9uh5HINo/aVbmsjajmp9mvUpOBYMlN4eef6TlcTlw6/HkojaKzuskSO HL4CXnVKHwL1rk1lceKfHPyaet+bMtyMWHmagrWQ= Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 1C0E73858D32 for ; Fri, 4 Nov 2022 17:24:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1C0E73858D32 Received: from [172.16.42.100] (modemcable092.73-163-184.mc.videotron.ca [184.163.73.92]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8A2221E0D3; Fri, 4 Nov 2022 13:24:33 -0400 (EDT) Message-ID: Date: Fri, 4 Nov 2022 13:24:33 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Subject: Re: [PATCH] gdb: make "start" breakpoint inferior-specific Content-Language: fr To: Andrew Burgess , Simon Marchi via Gdb-patches References: <20220804174035.2441960-1-simon.marchi@efficios.com> <87ler3cr4x.fsf@redhat.com> In-Reply-To: <87ler3cr4x.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Cc: Simon Marchi Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" > I think test cases like this should have an `alarm` call, just in case > something goes wrong, this'll stop the test running forever. Done, added this: diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific-other.c b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c index b593c3aa068..522971e81d4 100644 --- a/gdb/testsuite/gdb.multi/start-inferior-specific-other.c +++ b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c @@ -16,6 +16,7 @@ along with this program. If not, see . */ #include +#include int main (int argc, const char **argv) @@ -23,6 +24,8 @@ main (int argc, const char **argv) if (argc == 999) return 0; + alarm (30); + for (;;) main (999, NULL); > >> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.c b/gdb/testsuite/gdb.multi/start-inferior-specific.c >> new file mode 100644 >> index 00000000000..b69e218962a >> --- /dev/null >> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific.c >> @@ -0,0 +1,22 @@ >> +/* This testcase is part of GDB, the GNU debugger. >> + >> + Copyright 2022 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 . */ >> + >> +int >> +main () >> +{ >> + return 0; >> +} >> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.exp b/gdb/testsuite/gdb.multi/start-inferior-specific.exp >> new file mode 100644 >> index 00000000000..5a5572f591a >> --- /dev/null >> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific.exp >> @@ -0,0 +1,58 @@ >> +# Copyright 2022 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 . >> + >> +# Test that "start"ing an inferior does not inadvertently stop in another >> +# inferior. >> +# >> +# To achieve this, we have an inferior (the "other") running in background >> +# constantly re-calling main. We then "start" a second inferior. A buggy GDB >> +# would report a breakpoint hit in "other". >> + >> +standard_testfile .c -other.c >> + >> +if { [use_gdb_stub] } { >> + return >> +} >> + >> +set srcfile_other ${srcfile2} >> +set binfile_other ${binfile}-other >> + >> +if { [build_executable ${testfile}.exp ${binfile} "${srcfile}" {debug}] != 0 } { >> + return -1 >> +} >> + >> +if { [build_executable ${testfile}.exp ${binfile_other} "${srcfile_other}" {debug}] != 0 } { >> + return -1 >> +} >> + >> +proc do_test {} { >> + # With remote, to be able to start an inferior while another one is >> + # running, we need to use the non-stop variant of the protocol. >> + save_vars { ::GDBFLAGS } { >> + if { [target_info gdb_protocol] == "extended-remote"} { >> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\"" >> + } >> + >> + clean_restart ${::binfile_other} >> + } >> + >> + gdb_test "run&" "Starting program: .*" "start background inferior" >> + gdb_test "add-inferior" "Added inferior 2.*" >> + gdb_test "inferior 2" "Switching to inferior 2.*" >> + gdb_file_cmd ${::binfile} >> + gdb_test "start" "Thread 2.1 .* hit Temporary breakpoint .*" > > I'm pretty sure there's a race here. We could set the breakpoint for > the 'start', start inferior 2 executing, hit the b/p and remove it, all > within the time it takes inferior 1 to perform a single iteration ... in > theory, though it does seem unlikely. Hmm, but it would just mean that we have a teeny tiny chance of not catching the bug. A race condition in this way is more acceptable than the opposite (the kind that gives random failures), especially if the risk is super low. If you think that the startup time of inferior 1 could be significant enough to cause trouble, we could always run it until its main, then continue. In that case we would know it's already in its tight loop by the time we start inferior 2. Since I added that alarm call, inferior 1 has a bit more work to do before getting to the tight loop, so I might as well do that just to be sure. Even then, there is always a theoritical chance that inferior 1 gets scheduled out for the whole time of inferior 2's "start". I don't think there is any way around that, but I also think it doesn't matter given the low probability, and the fact that over many test runs, it won't happen all the time. > If, before the `start` you did `break *main` then we should hit the > `*main` breakpoint before the temporary `start` breakpoint, the > temporary breakpoint is left in place, which gives inferior 1 additional > time to not hit the breakpoint. I don't understand your proposal. You mean that the start of inferior 2 would hit that breakpoint on *main? But what you describe makes me think that we don't really handle well the case of "start" hitting another breakpoint than the one we insert. I just tried debugging this program: #include __attribute__((constructor)) void func(void) { printf("yoyoyo\n"); } int main() { printf("Hi\n"); return 0; } $ ./gdb -nx -q --data-directory=data-directory a.out Reading symbols from a.out... (gdb) b func Breakpoint 1 at 0x1151: file test.c, line 5. (gdb) start Temporary breakpoint 2 at 0x116b: file test.c, line 10. Starting program: /home/smarchi/build/binutils-gdb/gdb/a.out [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Breakpoint 1, func () at test.c:5 5 printf("yoyoyo\n"); (gdb) i b Num Type Disp Enb Address What 1 breakpoint keep y 0x0000555555555151 in func at test.c:5 breakpoint already hit 1 time 2 breakpoint del y 0x000055555555516b in main at test.c:10 stop only if $_inferior == 1 (gdb) c Continuing. yoyoyo Temporary breakpoint 2, main () at test.c:10 10 printf("Hi\n"); I believe that we should delete the breakpoint on main if we present the user another stop. But I don't intend to fix this as part of this patch. > Of course, this relies on the `*main` and `start` breakpoints being a`t > different addresses, which should be the case if main has some prologue > to skip over. > > I guess, ideally we would also do something to confirm that inferior 1 > has gone past main while the temporary breakpoint is in place, right now > we're just relying on inferior 1 running fast enough. > > In general I'm happy enough with the actual GDB change you propose here, > but like Bruno, I also wondered if it would be better to add inferior > specific breakpoints as an actual thing, rather than relying on the > condition logic like you do here? > > I wasn't sure how you'd feel about this idea, so I didn't spend too > long, but below is a VERY rough proof of concept patch, that appies on > top of yours, that adds inferior specific breakpoints. You temporary > breakpoint now becomes: > > tbreak -qualified main inferior 1 > > But this functionality would also be available to a user if they wanted > it, which might be nice. I think it's a good idea, but I would prefer if that wasn't a prerequesite for this patch, since adding this feature can become somewhat of a rabbit hole. Functionally, a condition will essentially behave the same, I think it's robust, and I would like to get rid of the flakiness in the testsuite sooner than later :). > When I say the code below is rough, I really mean it, here's a list of > things I think still need doing: > > * I initially added a `breakpoint::inferior` field just like we have a > `breakpoint::thread` field, but I think a better solution would be > to change `breakpoint::thread` into a ptid_t and store inferior and > thread information in that one field, The problem is that you could create inferior-specific breakpoints before you have execution, so before you have a p(t)id. Another thing we need to handle (haven't looked at your code yet, maybe you already do) is when an inferior gets deleted, breakpoints specific to that inferior should probably be deleted too. > * Obviously it should give an error if a user specifies both `thread` > and `inferior` for a breakpoint, Makes sense. > * The `info breakpoints` output is fine for single location > breakpoints, but I think multi-location breakpoints would need > sorting out still, Not sure what you are referring to exactly, but if you have two inferiors with two distinct program spaces, I think we could manage to create only a location in the program space attached to the inferior we target. If we have two inferiors sharing a program space, I guess there will be a single location in any case. > * There's a couple of TODO comments in there still to address, > > * Need to write docs, NEWS, and add tests, > > Like I said, you might see a reason why this is a bad idea, so I didn't > want to put any more work in without having a discussion about the idea. > > Thoughts? As I said, I would prefer not to make this work a prerequisite for the fix. It would be possible to change the internal API to make it possible to make breakpoints inferior-specific, internally, but that just seems more trouble and more risky for nothing. In the end, it would just end up doing the same comparison, but in a different way. I'm unable to apply your patch unfortunately, for some reason, and I'm not good at reading raw diffs. I could look at it if you sent a clean version (could be an RFC). Simon