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

[Andrew Burgess <aburgess@redhat.com>]

Lancelot SIX <lsix@lancelotsix.com> writes:

> On Tue, Nov 22, 2022 at 03:09:40PM +0000, 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;
>
> Hi,
>
> Wouldn't this just disable the "map_failed" probe for everyone, even if
> it is available for the current glibc?
>
> Should this become:
>
>   if (streq (probe_info[i].name, "map_failed") && probes[i].empty ())
>     continue
>
> to only ignore this probe if it is missing?

That's a very good point.

In the update below, I've moved the name check inside the immediately
following 'if (probes[i].empty)' block.  I've also added an assert that
the action for the probe is DO_NOTHING - if that action did ever change,
and we then skipped the probe we'd be in trouble.

However, I've also replied to Simon with an alternative approach to this
fix.  I'm not really sure which of the two solutions I prefer, so I'm
posting both to see if there's a preference in the group.

Thanks,
Andrew

---

commit 137177ba43c07aa3e33f5244fcc826e26f2474eb
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.  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.

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 6acaf87960b..c948808f853 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -2205,15 +2205,26 @@ 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.  */
+	  if (streq (probe_info[i].name, "map_failed"))
+	    {
+	      gdb_assert (probe_info[i].action == DO_NOTHING);
+	      continue;
+	    }
+
+	  return false;
+	}
 
       /* Ensure probe arguments can be evaluated.  */
       for (probe *p : probes[i])