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

[Luis Machado <luis.machado@arm.com>]

Hi Andrew,

This seems to have broken armhf on Ubuntu 22.04.

https://builder.sourceware.org/buildbot/#/builders/169/builds/1163

On 11/28/22 17:18, Andrew Burgess via Gdb-patches wrote:
> 
> Thanks you all for your review feedback.
> 
> I've now pushed this version of the patch (inc Pedro's suggested typo
> fix).
> 
> Thanks,
> Andrew
> 
> 
> Andrew Burgess <aburgess@redhat.com> writes:
> 
>> 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.
>>
>> 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.
>>
>> [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
>> +	     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),
>> +	     then we will still use the probes interface, even if that
>> +	     probe is missing.
>> +
>> +	     The only (possible) downside of this is that, if the user has
>> +	     'set stop-on-solib-events on' in effect, then they might get
>> +	     fewer events using the probes interface than with the classic
>> +	     non-probes interface.  */
>> +	  if (prove_info[i].action == DO_NOTHING)
>> +	    continue;
>> +	  else
>> +	    return false;
>> +	}
>>   
>>         /* Ensure probe arguments can be evaluated.  */
>>         for (probe *p : probes[i])
>