From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by sourceware.org (Postfix) with ESMTPS id 5C62A3858C52 for ; Thu, 2 Feb 2023 19:17:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5C62A3858C52 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f48.google.com with SMTP id r2so2641796wrv.7 for ; Thu, 02 Feb 2023 11:17:48 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:to:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=hKd2WDuOVBoEiConLfBlh5Umh1avCenWrZwDCTjBPAs=; b=pezPXDiY7TwX1MgsesS8ao9I62BzYxt1vWhdTuav+/pAlD4UT272iv9luDo66N1ZmN UP8tQaHiKWxqBQgTFiV4sp7JqZvM+uaEJi1Lk2HGRy1p1Fxtxcl4OFUp+YtOAQPuCdvl RdOYyvTL2rxAa9TiXrcS0O9xiBV/zJJaSi19pK48LctR239xLp+cJlEtE3Yd1jsDtn2w d6wAn4eanOCdbbNS40hGc2wpxeyGADyltXD1UwYNnukrc6RrhuFy/oknMZPbnBQhlk3U aB1yuO7xkMtKKkBN2VsFwc2Y6ykPA8HNJEFTCYMSsxmxF9+vi1OKz4WkkG/f2t93kZYx 9woQ== X-Gm-Message-State: AO0yUKXlpSHiygJtV2tZ2VMQPcHC/yd1vU89k3/zkoGW4m0XKD0ybelP rtO/mEoWNCkfnOUg4tzGit9do/p+Yn9y1Q== X-Google-Smtp-Source: AK7set9NkLpdgaDZmJ7PaA6zLc7WkRKlo23YKoF5a/Kwrf7BVuQ/zcJZW9hErTGQqvhREcNWQVuYIQ== X-Received: by 2002:a05:6000:10c5:b0:2c2:de2b:e7ca with SMTP id b5-20020a05600010c500b002c2de2be7camr5849603wrx.68.1675365466933; Thu, 02 Feb 2023 11:17:46 -0800 (PST) Received: from ?IPv6:2001:8a0:f92b:9e00::1fe? ([2001:8a0:f92b:9e00::1fe]) by smtp.gmail.com with ESMTPSA id e8-20020a5d6d08000000b00297dcfdc90fsm263220wrq.24.2023.02.02.11.17.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 02 Feb 2023 11:17:46 -0800 (PST) Subject: Re: [PATCHv2 5/6] gdb: add inferior-specific breakpoints and watchpoints To: Andrew Burgess , gdb-patches@sourceware.org References: <050da90b0b8c886983ec472a957b1075d4ecf7d6.1674207665.git.aburgess@redhat.com> From: Pedro Alves Message-ID: Date: Thu, 2 Feb 2023 19:17:45 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <050da90b0b8c886983ec472a957b1075d4ecf7d6.1674207665.git.aburgess@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2023-01-20 9:46 a.m., Andrew Burgess via Gdb-patches wrote: > This commit extends the breakpoint mechanism to allow for inferior > specific breakpoints (and watchpoints). > > As GDB gains better support for multiple connections, and so for > running multiple (possibly unrelated) inferiors, then it is not hard > to imagine that a user might wish to create breakpoints that apply to > any thread in a single inferior. To achieve this currently, the user > would need to create a condition possibly making use of the $_inferior > convenience variable, which, though functional, isn't the most user > friendly. So IIUC, if you do: (gdb) break foo inferior 1 and foo exists in other inferiors, the resulting breakpoint will still have locations for the other inferiors. Is that so? I think that instead, we should not get any location for other inferiors. I.e., we should do location pre-filtering, simply by not looking for locations in pspaces other than the inferior's. I think that can be done easily by simply passing the inferior's pspace as search_pspace argument to decode_line_full. And then it might be interesting to have a testcase that makes sure that after following a vfork, the breakpoint still only triggers in the inferior the user specified, even though the breakpoint is physicaly planted in both parent/child (because they share the address space). > @@ -1462,11 +1462,28 @@ breakpoint_set_thread (struct breakpoint *b, int thread) > { > int old_thread = b->thread; > > + gdb_assert (thread == -1 || b->inferior == -1); > + > b->thread = thread; > if (old_thread != thread) > gdb::observers::breakpoint_modified.notify (b); > } > > +/* Set the inferior for breakpoint B to INFERIOR. If INFERIOR is -1, make > + the breakpoint work for any inferior. */ > + > +void > +breakpoint_set_inferior (struct breakpoint *b, int inferior) > +{ > + int old_inferior = b->inferior; > + > + gdb_assert (inferior == -1 || b->thread == -1); > + > + b->inferior = inferior; > + if (old_inferior != inferior) > + gdb::observers::breakpoint_modified.notify (b); > +} > + > /* Set the task for this breakpoint. If TASK is 0, make the > breakpoint work for any task. */ > > @@ -3152,6 +3169,12 @@ insert_breakpoint_locations (void) > && !valid_global_thread_id (bl->owner->thread)) > continue; > > + /* Or inferior specific breakpoints if the inferior no longer > + exists. */ > + if (bl->owner->inferior != -1 > + && !valid_global_inferior_id (bl->owner->inferior)) > + continue; > + > switch_to_program_space_and_thread (bl->pspace); > > /* For targets that support global breakpoints, there's no need > @@ -3256,6 +3279,31 @@ Thread-specific breakpoint %d deleted - thread %s no longer in the thread list.\ > } > } > > +/* Called when inferior INF has exited. Remove per-inferior breakpoints. */ We remove thread-specific breapoints when the thread exits, because when the thread exits, we delete it (the thread). But when the inferior exits, the inferior is not deleted, it still exists, ready for a re-run. So why would we remove the breakpoints when the inferior has exited, if the inferior still exists? I'd have thought we'd remove the breakpoints when the inferior is _removed_, instead. If removing then on exit, then it feels like it could be done from breakpoint_init_inferior, as that is called when mourning the inferior, and it is responsible for deleting breakpoints that no longer make sense to carry for the inferior. But I'm not convinced we should delete the breakpoint on exit, anyhow. > + > +static void > +remove_inferior_breakpoints (struct inferior *inf) > +{ > + for (breakpoint *b : all_breakpoints_safe ()) > + { > + if (b->inferior == inf->num && user_breakpoint_p (b)) > + { > + /* Tell the user the breakpoint has been deleted. But only for > + breakpoints that would not normally have been deleted at the > + next stop anyway. */ > + if (b->disposition != disp_del > + && b->disposition != disp_del_at_next_stop) > + gdb_printf (_("\ > +Inferior-specific breakpoint %d deleted - inferior %d has exited.\n"), > + b->number, inf->num); > + > + /* Hide it from the user and mark it for deletion. */ > + b->number = 0; Doesn't this mess up MI breakpoint deleted notifications? > + b->disposition = disp_del_at_next_stop; If the inferior is gone, can't we delete the breakpoint straight away? Maybe we'd need to call mark_breakpoints_out ? > + } > + } > +} > + > /* See breakpoint.h. */ > > void > @item break > When called without any arguments, @code{break} sets a breakpoint at > @@ -4983,7 +5036,7 @@ > > @table @code > @kindex watch > -@item watch @r{[}-l@r{|}-location@r{]} @var{expr} @r{[}thread @var{thread-id}@r{]} @r{[}mask @var{maskvalue}@r{]} @r{[}task @var{task-id}@r{]} > +@item watch @r{[}-l@r{|}-location@r{]} @var{expr} @r{[}thread @var{thread-id}@r{]} @r{[}inferior @var{inferior-id}@r{]} @r{[}mask @var{maskvalue}@r{]} @r{[}task @var{task-id}@r{]} > Set a watchpoint for an expression. @value{GDBN} will break when the > expression @var{expr} is written into by the program and its value > changes. The simplest (and the most popular) use of this command is > @@ -5000,8 +5053,10 @@ > that watchpoints restricted to a single thread in this way only work > with Hardware Watchpoints. > > -Similarly, if the @code{task} argument is given, then the watchpoint > -will be specific to the indicated Ada task (@pxref{Ada Tasks}). > +Similarly, if the @code{inferior} argument is given, then the > +watchpoint will trigger only for the specific inferior, or if the > +@code{task} argument is given, then the watchpoint will be specific to > +the indicated Ada task (@pxref{Ada Tasks}). A watchpoint is already per-pspace, so inherently per-inferior in most scenarios -- we only evaluate the watched expression in the pspace where the watchpoint was created, and only track the resulting value for changes in one pspace. Note what happens currently if you set a watchpoint and you have multiple inferiors: (gdb) info inferiors Num Description Connection Executable 1 process 3974843 1 (native) /home/pedro/watch * 2 process 3975163 1 (native) /home/pedro/watch (gdb) watch global Hardware watchpoint 3: global (gdb) info breakpoints Num Type Disp Enb Address What 3 hw watchpoint keep y global inf 2 ^^^^^ (gdb) The only case where the new feature is useful for watchpoints, is if you're debugging multiple inferiors that share the address space, like right after following a vfork. In that case, here's what you get today: $ gdb ./testsuite/outputs/gdb.base/foll-vfork/foll-vfork GNU gdb (GDB) 14.0.50.20230127-git ... (gdb) start ... Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdbe8) at /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/foll-vfork.c:31 31 pid = 1 + argc; (gdb) set detach-on-fork off (gdb) set follow-fork-mode child (gdb) info inferiors Num Description Connection Executable * 1 process 3979825 1 (native) /home/pedro/rocm/gdb/build/gdb/testsuite/outputs/gdb.base/foll-vfork/foll-vfork (gdb) n [Attaching after Thread 0x7ffff7d6f740 (LWP 3979825) vfork to child process 3980120] [New inferior 2 (process 3980120)] [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". 44 in ../sysdeps/unix/sysv/linux/x86_64/vfork.S (gdb) info inferiors Num Description Connection Executable 1 process 3979825 1 (native) /home/pedro/rocm/gdb/build/gdb/testsuite/outputs/gdb.base/foll-vfork/foll-vfork is vfork parent of inferior 2 * 2 process 3980120 1 (native) /home/pedro/rocm/gdb/build/gdb/testsuite/outputs/gdb.base/foll-vfork/foll-vfork is vfork child of inferior 1 (gdb) up #1 0x000055555555523f in main (argc=1, argv=0x7fffffffdbe8) at /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/foll-vfork.c:32 32 pid = vfork (); /* VFORK */ (gdb) watch pid Hardware watchpoint 2: pid (gdb) info breakpoints Num Type Disp Enb Address What 2 hw watchpoint keep y pid inf 1, 2 ^^^^^^^^ so gdb is saying that the watchpoint will trigger in either inferior, because they are both sharing the same address space. I think the manual need to explain this right, otherwise the user might be confused about whether watchpoints triggers on all inferiors if you don't use "watch ... inferior". Also, this deserves a testcase. I'd think it is best even to split watchpoints support to a separate patch, as it's sufficiently different from breakpoints. I noticed that the NEWS entry does not mention watchpoints. Pedro Alves