Re: [PATCH 6/6] gdb: don't share aspace/pspace on fork with "detach-on-fork on" and "follow-fork-mode child"

[Simon Marchi <simon.marchi@efficios.com>]

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   <MULTIPLE>         ^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.

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.

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.

From 38f41f929f46926fede29c70064871fc23e0e215 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
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   <MULTIPLE>
    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"
     }
 }
-- 
2.33.0