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.129.124]) by sourceware.org (Postfix) with ESMTPS id 8C0253858C52 for ; Fri, 3 Feb 2023 16:55:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8C0253858C52 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=1675443308; 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=GysUAQ94MSD9ar/AeoNwbwW/f6JlU+7iYMPw9I1yDoI=; b=XFPcxM4R7a0pEdOhXD6O7KO+DJSGe0PERNfkZQFpMZZK/M1l5vASFuwmMAW7i34aiFxALa rIZ5qvUYawvCq1LizmifFReXnm2ffCowNwsUTgt7gUK67XA7N0nVX+4CFzGxCkJOVP4YQn +ghN3pFm1dkpN2cHdIP3lfChzI/Al7M= Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-83-4_TVNm3aNAeUf7TfHBcZPA-1; Fri, 03 Feb 2023 11:55:07 -0500 X-MC-Unique: 4_TVNm3aNAeUf7TfHBcZPA-1 Received: by mail-qv1-f72.google.com with SMTP id ob12-20020a0562142f8c00b004c6c72bf1d0so3007291qvb.9 for ; Fri, 03 Feb 2023 08:55:06 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=GysUAQ94MSD9ar/AeoNwbwW/f6JlU+7iYMPw9I1yDoI=; b=zqM8nMSMO0bBpPByfxecmEvJOIFMIVjJkF4tQYM5IVZ0FP1jqERYaOgRHyCFEwUmwb Kt1dlJdxix15FQhfz3SlrGV9CM57xDO1WBirHMjJeLxSfZHiOKqazVoXaPcmhANKIy9j W+5hWew161KQS29jzDkUWLUcKYAnF4oYC92qGcTDk2fn1ew9PyIxTSFUkAY3yJT3hMMn q7gi8Kacwtn+1/p2YhVAnSq6z1TdoS5iYYRjasd5iG3AZZHZtH7aHayo57nol1TkEkTy BE41GYcydEuAwhPL8y6ARF7iQpP5ujRipCogroZ3m0jCtJu8dzcXmqeI57cTYMJ5mH5c oPbw== X-Gm-Message-State: AO0yUKXrDEPe4Jo2ropTnrT2He8QytH2o50BvF7bKchv8QVXzOV1qbC1 28fzuZxzTo9lK4rJ4ioHmUiKbPVyVDPKKiBYhVggDzezI5skpJzABoxjURbVDadKSjMuRWhvlXU ggnbCwg/gQw5ErD4sawUK/w== X-Received: by 2002:a05:622a:4399:b0:3b9:bc8c:c214 with SMTP id em25-20020a05622a439900b003b9bc8cc214mr6523226qtb.31.1675443306074; Fri, 03 Feb 2023 08:55:06 -0800 (PST) X-Google-Smtp-Source: AK7set8WwB16CL3l90Z+aQ2FONSFz8QVCEq3w9hRMYaE+PUUA/3DqZA53WE6ol2Vrr/n1np2m8rtBQ== X-Received: by 2002:a05:622a:4399:b0:3b9:bc8c:c214 with SMTP id em25-20020a05622a439900b003b9bc8cc214mr6523199qtb.31.1675443305694; Fri, 03 Feb 2023 08:55:05 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id dz5-20020a05620a2b8500b006fc3fa1f589sm2072585qkb.114.2023.02.03.08.55.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 Feb 2023 08:55:05 -0800 (PST) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCHv2 5/6] gdb: add inferior-specific breakpoints and watchpoints In-Reply-To: References: <050da90b0b8c886983ec472a957b1075d4ecf7d6.1674207665.git.aburgess@redhat.com> Date: Fri, 03 Feb 2023 16:55:03 +0000 Message-ID: <87k00yr8ns.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=-5.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP 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: > 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. Thanks for all your great feedback. I'll work through the rest of your comments, but I want to follow up on this one. Baris already raised the point of filtering per-inferior breakpoints so a to avoid adding extra locations. I 100% agree that this is something that we should do, and this is on my plan if/when I land this series. But I don't see any particular difference between per-inferior and per-thread breakpoints in this regard. Please correct me if I'm wrong, but a per-thread breakpoint will insert into every inferior despite only ever triggering for a single thread in a single inferior (per thread breakpoints are on the global thread-id). So, because this oddity already exists in GDB, my plan was to land this feature, then follow up with a patch to limit both per-thread and per-inferior breakpoints to only insert into the correct pspace. Thanks, Andrew > > 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