Re: [PATCH] gdb: make "start" breakpoint inferior-specific

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

> 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 <http://www.gnu.org/licenses/>.  */

 #include <stdlib.h>
+#include <unistd.h>

 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 <http://www.gnu.org/licenses/>.  */
>> +
>> +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 <http://www.gnu.org/licenses/>.
>> +
>> +# 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 <stdio.h>

__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