From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id BDCEE3858D35 for ; Tue, 28 Sep 2021 22:38:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BDCEE3858D35 Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id EBCFE22263; Tue, 28 Sep 2021 22:38:01 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id CC4E413EB3; Tue, 28 Sep 2021 22:38:01 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id mnG1MEmZU2GMJQAAMHmgww (envelope-from ); Tue, 28 Sep 2021 22:38:01 +0000 Subject: Re: [PATCH 6/6] gdb: don't share aspace/pspace on fork with "detach-on-fork on" and "follow-fork-mode child" To: Simon Marchi , Simon Marchi , John Baldwin , gdb-patches@sourceware.org References: <20210910205402.3853607-1-simon.marchi@efficios.com> <20210910205402.3853607-6-simon.marchi@efficios.com> <021ba846-e43e-e6ef-c827-7e2b8ff8f5e9@FreeBSD.org> <58ca5b53-9d70-b8e7-e4c9-86b9f080f731@polymtl.ca> <41efe1d2-64fe-b0d1-029f-363bfc567bf5@polymtl.ca> From: Tom de Vries Message-ID: Date: Wed, 29 Sep 2021 00:38:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, 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: Tue, 28 Sep 2021 22:38:04 -0000 On 9/28/21 9:12 PM, Simon Marchi wrote: > > > On 2021-09-28 11:10, Tom de Vries wrote: >> On 9/27/21 9:32 PM, Simon Marchi via Gdb-patches wrote: >>> On 2021-09-10 23:16, Simon Marchi via Gdb-patches wrote: >>>>>> Note that the problem described above happens today with "detach-on-fork >>>>>> off" and "follow-fork-mode child", because we create new spaces for the >>>>>> child. This will have to be addressed later. >>>>>> >>>>>> Test-wise, improve gdb.base/foll-fork.exp to set a breakpoint that is >>>>>> expected to have a location in each inferiors. Without the fix, when >>>>>> the two inferiors erroneously share a program space, GDB reports a >>>>>> single location. >>>>> >>>>> So I wonder about the case where follow-fork-mode is parent and >>>>> detach-on-fork is off? In that case, should the existing aspace/pspace >>>>> stay with the parent and the child get clones? That is, using the >>>>> follow-fork-mode setting to determine which inferior gets the existing >>>>> aspace/pspace and assigning the cloned copies to the !follow-fork-mode >>>>> inferior? >>>> >>>> I think that would work, to address the problem described above. >>> >>> FYI, I tried doing the above (giving the original program space to the >>> child with "follow-fork-mode child" and "detach-on-fork off") and I hit >>> some problems. Attached to the program space is the list of shared >>> libraries. So GDB would now think the parent has no shared library. I >>> added a solib_create_inferior_hook call to force re-discovering the >>> shared libraries in the parent. That seemed to work, but then in MI >>> there were spurious =library-loaded events that made it look like >>> inferior 1 re-loaded shared libraries. I'm also worried about what >>> other per-program space is kept, that could cause some kind of trouble >>> for the parent. So I will put this off for now, as it's not something >>> that should be done in a rush, I think. >>> >>> In the mean time, I'll re-test this series and push it if all seems >>> good. >> >> I'm seeing: >> ... >> (gdb) PASS: gdb.base/foll-fork.exp: follow-fork-mode=child: >> detach-on-fork=on: cmd=next 2: test_follow_fork: break callee >> info breakpoints 2^M >> Num Type Disp Enb Address What^M >> 2 breakpoint keep y ^M >> 2.1 y 0x00000000004005ae in callee at >> /home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base/foll-fork.c:9 >> inf 2^M >> 2.2 y 0x00000000004005ae in callee at >> /home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base/foll-fork.c:9 >> inf 1^M >> (gdb) FAIL: gdb.base/foll-fork.exp: follow-fork-mode=child: >> detach-on-fork=on: cmd=next 2: test_follow_fork: info breakpoints >> ... >> >> Looks like the test-case expects first inf 1, then inf 2, and this is >> the other way around. >> >> Should the test-case regexp be updated? > > Huh, I remember that have encountered this, and thought I had changed > the regexp to allow both orders. I must have lost that change. But it > still bugged me, why did I see one order sometimes and the other at > other times. > > On my home computer (Arch Linux) I see "2, 1" and on my work computer > (Ubuntu 20.04) I see "1, 2". So that lets me compare both cases. The > order comes from the SALs passed to init_breakpoint_sal. It comes from > the compare_symbols function here: > > https://gitlab.com/gnutools/binutils-gdb/-/blob/3a6a0158ee07ba2f960ae4a898897460382dc5ec/gdb/linespec.c#L3568-3592 > > It compares the value of program_space pointers to determine the order > of symbols, so that can explain the difference between two machines. > > That then decides the order of the locations in the breakpoint::loc > list, since add_location_to_breakpoint only sorts by address. So > depending on which location is added first, it will give different > orders. > Ack. > I think that the order of locations should be as stable as possible. > I > started writing a patch for that, but then realized that even then, the > order in the test could vary, based on the architecture, whether you use > PIE or not, etc. And in the end the order doesn't really matter, what > is important is that we have two locations. > > Having a more stable breakpoint location order would be nice, but not as > a way to fix this test failure. > Agreed. > So, what do you think of the patch below? The regexp accepts (1|2) for > both lines. If you have a simple way to say "either 1 then 2 or 2 then > 1" (so that it rejects 1 then 1 or 2 then 2), I would take it, but > otherwise I think this is sufficient. > FWIW, I prefer Pedro's gdb_assert variant, which is a bit more strict thank this one, and takes the or-ing out the regexp domain. Thanks, - Tom > From 38f41f929f46926fede29c70064871fc23e0e215 Mon Sep 17 00:00:00 2001 > From: Simon Marchi > Date: Tue, 28 Sep 2021 13:22:53 -0400 > Subject: [PATCH] gdb.base/foll-fork.exp: accept "info breakpoints" output in > any order > > The test currently requires the "inf 1" breakpoint to be before the "inf > 2" breakpoint. This is not always the case: > > info breakpoints 2 > Num Type Disp Enb Address What > 2 breakpoint keep y > 2.1 y 0x0000555555554730 in callee at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/foll-fork.c:9 inf 2 > 2.2 y 0x0000555555554730 in callee at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/foll-fork.c:9 inf 1 > (gdb) FAIL: gdb.base/foll-fork.exp: follow-fork-mode=parent: detach-on-fork=off: cmd=next 2: test_follow_fork: info breakpoints > > Since add_location_to_breakpoint uses only the address as a criterion to > sort locations, the order of locations at the same address is not > stable: it will depend on the insertion order. Here, the insertion > order comes from the order of SALs when creating the breakpoint, which > can vary from machine to machine. While it would be more user-friendly > to have a more stable order for printed breakpoint locations, it doesn't > really matter for this test, and it would be hard to define an order > that will be the same everywhere, all the time. > > So, loosen the regexp to not check the "inf 1" vs "inf 2" order. > > Change-Id: I5ada2e0c6ad0669e0d161bfb6b767229c0970d16 > --- > gdb/testsuite/gdb.base/foll-fork.exp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gdb/testsuite/gdb.base/foll-fork.exp b/gdb/testsuite/gdb.base/foll-fork.exp > index 7f9e1cf87c6a..c0beb089d451 100644 > --- a/gdb/testsuite/gdb.base/foll-fork.exp > +++ b/gdb/testsuite/gdb.base/foll-fork.exp > @@ -195,8 +195,8 @@ proc_with_prefix test_follow_fork { follow-fork-mode detach-on-fork cmd } { > } > gdb_test "info breakpoints $bpnum" \ > [multi_line \ > - "$bpnum\\.1 .* inf 1" \ > - "$bpnum\\.2 .* inf 2"] \ > + "$bpnum\\.1 .* inf (1|2)" \ > + "$bpnum\\.2 .* inf (1|2)"] \ > "info breakpoints" > } > } >