Re: [PATCH] gdb: relax requirement for the map_failed stap probe to be present

[Simon Marchi <simark@simark.ca>]

On 11/24/22 05:46, Andrew Burgess via Gdb-patches wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 11/22/22 10:09, Andrew Burgess via Gdb-patches wrote:
>>> From glibc 2.35 and later, the "map_failed" stap probe is no longer
>>> included in glibc.  The removal of the probe looks like an accident,
>>> but it was caused by a glibc commit which meant that the "map_failed"
>>> probe could no longer be reached; the compiler than helpfully
>>> optimised out the probe.
>>>
>>> In GDB, in solib-svr4.c, we have a list of probes that we look for
>>> related to the shared library loading detection.  If any of these
>>> probes are missing then GDB will fall back to the non-probe based
>>> mechanism for detecting shared library loading.  The "map_failed"
>>> probe is include in the list of required probes.
>>>
>>> This means that on glibc 2.35 (or later) systems, GDB is going to
>>> always fall back to the non-probes based mechanism for detecting
>>> shared library loading.
>>>
>>> I raised a glibc bug to discuss this issue:
>>>
>>>   https://sourceware.org/bugzilla/show_bug.cgi?id=29818
>>>
>>> But, whatever the ultimate decision from the glibc team, given there
>>> are version of glibc in the wild without the "map_failed" probe, we
>>> probably should update GDB to handle this situation.
>>>
>>> The "map_failed" probe is already a little strange, very early
>>> versions of glibc didn't include this probe, so, in some cases, if
>>> this probe is missing GDB is happy to ignore it.  In this commit I
>>> just expand this logic to make the "map_failed" probe fully optional.
>>>
>>> With this commit in place, then, when using a glibc 2.35 or later
>>> system, GDB will once again use the stap probes for shared library
>>> detection.
>>> ---
>>>  gdb/solib-svr4.c | 13 +++++++++----
>>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
>>> index 6acaf87960b..87cd06f251a 100644
>>> --- a/gdb/solib-svr4.c
>>> +++ b/gdb/solib-svr4.c
>>> @@ -2205,10 +2205,15 @@ svr4_find_and_create_probe_breakpoints (svr4_info *info,
>>>  
>>>        probes[i] = find_probes_in_objfile (os->objfile, "rtld", name);
>>>  
>>> -      /* The "map_failed" probe did not exist in early
>>> -	 versions of the probes code in which the probes'
>>> -	 names were prefixed with "rtld_".  */
>>> -      if (with_prefix && streq (name, "rtld_map_failed"))
>>> +      /* The "map_failed" probe did not exist in early versions of the
>>> +	 probes code in which the probes' names were prefixed with
>>> +	 "rtld_".
>>> +
>>> +	 Additionally, the "map_failed" probe was accidentally removed
>>> +	 from glibc 2.35 and later, when changes in glibc meant the probe
>>> +	 could no longer be reached.  In this case the probe name doesn't
>>> +	 have the "rtld_" prefix.  */
>>> +      if (streq (probe_info[i].name, "map_failed"))
>>>  	continue;
>>>  
>>>        /* Ensure at least one probe for the current name was found.  */
>>>
>>> base-commit: 84f9fbe90e5429adb9dee68f04f44c92fa9e2345
>>> -- 
>>> 2.25.4
>>
>> Hi,
>>
>> I looked at this separately, and this was one of the fixes I considered.
>>
>> Another option was to make GDB not give up on the probes interface if
>> failing to look up a probe whose action is DO_NOTHING.  Probes with that
>> action are not used by GDB for solib bookkeeping, but can be used to
>> stop on solib events, with "set stop-on-solib-events".  I was just
>> worried if there was some cases where a probe would be missing, but the
>> corresponding event could be caught if using the original interface.  In
>> that case, using the probes interface would be a regression.  But it's
>> probably not worth wondering about.  If that happens it's just a bug
>> that needs to be fixed.  In the case we are looking at, if the
>> map_failed probe gets optimized out, then surely the corresponding call
>> to the r_brk function would also be optimized out.
> 
> I also considered just ignoring any probe was (a) missing, and (b) had a
> DO_NOTHING action.  The reason I didn't post this patch was because, at
> the time, my thinking was: if we don't care about any probe with a
> DO_NOTHING action, why even look for those probes, why not just remove
> them from the list?
> 
> I think you've (partially) convinced me that the user might be
> interested in seeing a stop at these probes even if GDB's action is
> DO_NOTHING.
> 
> I say partially above because GDB doesn't really do anything to tell the
> user which probe we stopped at, e.g. was is "init_start", "map_start",
> "map_failed", etc.  The user might be able to figure it out from the
> backtrace, but I still think it's not going to be trivial in all cases,
> e.g. "map_start" and "map_failed" are both located in the same function,
> so I think the user would need to lookup the probe address in the ELF,
> then compare that to the stop address.  Not impossible, but, I suspect,
> the complexity is an indication that users are not doing this much.
> Thus, I suspect, in reality, nobody really cares about the DO_NOTHING
> probes.

That's a good point.

> However, I think there is enough of a justification there for keeping
> the probes in the list, and just skipping any DO_NOTHING probes that
> don't exist.
> 
> Below then, is an alternative patch.  I don't have a strong preference
> between this one, and the original[1], but I thought I'd post this for
> discussion.  If this is preferred then I can just merge this.

You convinced me that this is fine, I think I am really over-worrying.

I like this version of the patch.

> [1] I'll also post an update to the original patch shortly that
> addresses Lancelot's feedback.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 11be1f25f446e68c23d0709cde46e32ff24b7eb9
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Tue Nov 22 12:45:56 2022 +0000
> 
>     gdb: relax requirement for the map_failed stap probe to be present
>     
>     From glibc 2.35 and later, the "map_failed" stap probe is no longer
>     included in glibc.  The removal of the probe looks like an accident,
>     but it was caused by a glibc commit which meant that the "map_failed"
>     probe could no longer be reached; the compiler than helpfully
>     optimised out the probe.
>     
>     In GDB, in solib-svr4.c, we have a list of probes that we look for
>     related to the shared library loading detection.  If any of these
>     probes are missing then GDB will fall back to the non-probe based
>     mechanism for detecting shared library loading.  The "map_failed"
>     probe is include in the list of required probes.
>     
>     This means that on glibc 2.35 (or later) systems, GDB is going to
>     always fall back to the non-probes based mechanism for detecting
>     shared library loading.
>     
>     I raised a glibc bug to discuss this issue:
>     
>       https://sourceware.org/bugzilla/show_bug.cgi?id=29818
>     
>     But, whatever the ultimate decision from the glibc team, given there
>     are version of glibc in the wild without the "map_failed" probe, we
>     probably should update GDB to handle this situation.
>     
>     The "map_failed" probe is already a little strange, very early
>     versions of glibc didn't include this probe, so, in some cases, if
>     this probe is missing GDB is happy to ignore it.  This is fine, the
>     action associated with this probe inside GDB is DO_NOTHING, this means
>     the probe isn't actually required in order for GDB to correctly detect
>     the loading of shared libraries.
>     
>     In this commit I propose changing the rules so that any probe whose
>     action is DO_NOTHING, is optional.
>     
>     There is one possible downside to this change, and that concerns 'set
>     stop-on-solib-events on'.  If a probe is removed from glibc, but the
>     old style breakpoint based mechanism is still in place within glibc
>     for that same event, then GDB will stop when using the old style
>     non-probe based mechanism, but not when using the probes based
>     mechanism.
>     
>     For the map_failed case this is not a problem, both the map_failed
>     probe, and the call to the old style breakpoint location were
>     optimised out, and so neither event (probes based, or breakpoint
>     based) will trigger.  This would only become an issue if glibc removed
>     a probe, but left the breakpoint in place (this would almost certainly
>     be a bug in glibc).
>     
>     For now, I'm proposing that we just don't worry about this.  Because
>     some probes have actions that are not DO_NOTHING, then we know the
>     user will always seem _some_ stops when a shared library is
>     loaded/unloaded, and (I'm guessing), in most cases, that's all they
>     care about.  I figure when someone complains then we can figure out
>     what the right solution is then.
>     
>     With this commit in place, then, when using a glibc 2.35 or later
>     system, GDB will once again use the stap probes for shared library
>     detection.
> 
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index 6acaf87960b..10e446af908 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -2205,15 +2205,34 @@ svr4_find_and_create_probe_breakpoints (svr4_info *info,
>  
>        probes[i] = find_probes_in_objfile (os->objfile, "rtld", name);
>  
> -      /* The "map_failed" probe did not exist in early
> -	 versions of the probes code in which the probes'
> -	 names were prefixed with "rtld_".  */
> -      if (with_prefix && streq (name, "rtld_map_failed"))
> -	continue;
> -
>        /* Ensure at least one probe for the current name was found.  */
>        if (probes[i].empty ())
> -	return false;
> +	{
> +	  /* The "map_failed" probe did not exist in early versions of the
> +	     probes code in which the probes' names were prefixed with
> +	     "rtld_".
> +
> +	     Additionally, the "map_failed" probe was accidentally removed
> +	     from glibc 2.35 and later, when changes in glibc meant the

Nit: I would maybe just say "removed in glibc 2.35" and not imply
anything about later versions, as we don't know whether it will be there
or not.

> +	     probe could no longer be reached, and the compiler optimized
> +	     the probe away.  In this case the probe name doesn't have the
> +	     "rtld_" prefix.
> +
> +	     To handle this, and give GDB as much flexibility as possible,
> +	     we make the rule that, if a probe isn't required for the
> +	     correct operation of GDB (i.e. it's action is DO_NOTHING),

it's -> its, I think

Simon