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

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

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

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 <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"
>      }
>  }
>