Re: [RFA] Fix PR gdb/17016: Expect for probe "map_complete" instead of "rtld_map_complete"

[Pedro Alves <palves at redhat dot com>]

On 09/24/2014 07:03 PM, Sergio Durigan Junior wrote:
> This commit fixes PR gdb/17016.  It could be considered to be obvious,
> but I am sending to the mailing list for approval either way...
> 
> The problem, described by Pedro, is that a specific test on
> gdb.threads/dlopen-libpthread.exp is XFAILing.  The test is:
> 
>   XFAIL: gdb.threads/dlopen-libpthread.exp: info probes all rtld rtld_map_complete
> 
> According to Pedro, he was not seeing this failure on Fedora 17, but
> is now seeing it on Fedora 20.  This has been caused by a difference
> between a Fedora-local glibc patch and an upstream glibc patch,
> submitted by Gary in 2012.
> 
> Back then, Gary submitted this patch to glibc upstream:
> 
>   <https://sourceware.org/ml/libc-alpha/2012-06/msg00690.html>
> 
> Which implemented the SDT probes on the runtime linker.  Note that the
> initial patch included the "rtld_" prefix on the probes' names.
> Roland McGrath asked him to remove this prefix, and this is what was
> pushed to the repo:
> 
>   <https://sourceware.org/ml/libc-alpha/2012-06/msg00723.html>
> 
>   commit 815e6fa3e010628f77838abec18692cbfeb60809
>   Author: Gary Benson <gbenson@redhat.com>
>   Date:   Thu Jul 26 11:03:35 2012 +0100
> 
>       Add SystemTap static probes to the runtime linker.  [BZ #14298]
> 
> Gary and Jan also added the gdb.threads/dlopen-libpthread.exp file later on GDB:
> 
>   commit a29a3fb7a350b70ec755b1964d2830094314dba8
>   Author: Gary Benson <gary@redhat.com>
>   Date:   Tue Jun 4 13:23:32 2013 +0000
> 
>       2013-06-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	      Gary Benson  <gbenson@redhat.com>
> 
> 	  * lib/gdb.exp (build_executable_from_specs): Use gdb_compile_pthread,
> 	  gdb_compile_shlib or gdb_compile_shlib_pthreads where appropriate.
> 	  * lib/prelink-support.exp (build_executable_own_libs): Allow INTERP
> 	  to be set to "no" to indicate that no ld.so copy should be made.
> 	  * gdb.base/break-interp.exp (solib_bp): New constant.
> 	  (reach_1): Use the above instead of "_dl_debug_state".
> 	  (test_attach): Likewise.
> 	  (test_ld): Likewise.
> 	  * gdb.threads/dlopen-libpthread.exp: New file.
> 	  * gdb.threads/dlopen-libpthread.c: Likewise.
> 	  * gdb.threads/dlopen-libpthread-lib.c: Likewise.
> 	  * gdb.base/solib-corrupted.exp: Disable test if GDB is using probes.
> 
> And this file is also using the "rtld_" prefix when trying to match
> the glibc probe (probably because they were using a Fedora glibc with
> the first patch applied, see below).
> 
> However, on the Fedora land, we included Gary's first patch on the
> glibc package for Fedora 17:
> 
>   <http://pkgs.fedoraproject.org/cgit/glibc.git/tree/glibc-rh179072.patch?h=f17>
> 
> And now, 2 years later, this local patch is obviously not needed
> anymore, because upstream glibc already has the necessary patch in
> it.  But we forgot to update our local testcase, so that is what this
> patch does.
> 
> OK to apply?

Well, AFAICS, upstream GDB still supports F17's probes.  See
svr4_create_solib_event_breakpoints:

	  memset (probes, 0, sizeof (probes));
	  for (i = 0; i < NUM_PROBES; i++)
	    {
	      const char *name = probe_info[i].name;
	      struct probe *p;
	      char buf[32];

	      /* Fedora 17 and Red Hat Enterprise Linux 6.2-6.4
		 shipped with an early version of the probes code in
		 which the probes' names were prefixed with "rtld_"
		 and the "map_failed" probe did not exist.  The
		 locations of the probes are otherwise the same, so
		 we check for probes with prefixed names if probes
		 with unprefixed names are not present.  */
	      if (with_prefix)
		{
		  xsnprintf (buf, sizeof (buf), "rtld_%s", name);
		  name = buf;
		}

	      probes[i]


So it seems to me the test should cope with both variants.

But, on a cursory look, I can't see what is being tested by this
test that wouldn't work without probes?  If the test would pass just
the same without probes, I think we should just remove the
probe probing.   OTOH, if this is testing functionally that only
works if probes are available, then I still think the test is
lacking a comment.

I also find it a bit odd to check for a probe point that GDB
doesn't seem to be using:

static const struct probe_info probe_info[] =
{
  { "init_start", DO_NOTHING },
  { "init_complete", FULL_RELOAD },
  { "map_start", DO_NOTHING },
  { "map_failed", DO_NOTHING },
  { "reloc_complete", UPDATE_OR_RELOAD },
  { "unmap_start", DO_NOTHING },
  { "unmap_complete", FULL_RELOAD },
};

Thanks,
Pedro Alves