Re: Failures with exelib.exp testcase (was Re: minutes 2010-08-19)

[Mark Wielaard <mjw at redhat dot com>]

On Thu, 2011-01-20 at 10:53 -0800, Roland McGrath wrote:
> > Aha. Thanks. I now remember we dealt with this somehow for the kernel
> > addresses: http://sourceware.org/bugzilla/show_bug.cgi?id=10206
> > Although reading your explanation now I don't understand why that
> > actually worked, since we don't have any special code to resolve such
> > addresses in translate.cxx, we always just store the st_value as sym
> > address (possibly adjusted with dwfl_module_relocate_address). Is there
> > something special about powerpc user code symbols that make it different
> > from kernel code symbols?
> 
> I'm no ppc64 expert, but I've just now looked at the symbol tables of a
> ppc64 kernel and a random ppc64 user executable.  
> 
> In the kernel, there is both an STT_FUNC symbol named ".foo" whose st_value
> is the actual code address, and an STT_NOTYPE symbol named "foo" whose
> st_value is the address in the .opd section (the function descriptor
> address).
> 
> In the user executable, there is only one symbol for a function still left
> after the final link, an STT_FUNC symbol named "foo" whose st_value is the
> address in the .opd section.
> 
> So I would surmise that what happens in systemtap in the kernel case is
> that it finds the ".foo" symbol and is happy with that, and ppc64 users
> just ignore the leading . when reading the output.  In the user case, there
> is no symbol at all whose value is the code address, so there is no match.

That explains why we have all these "." suppression stuff around in the
runtime and tapsets:

#ifdef __powerpc__
  // Map ".sys_foo" to "sys_foo".
  if (name[0] == '.')
    name++;
#endif

Strangely enough we have symbol table parsing code in both tapsets.cxx,
which handles the special case powerpc .odp section and the function
description setup, responsible for calculating the addresses to set
probes on, and totally separate symbol table parsing code in
translate.cxx, responsible for writing the lookup table we use at
runtime, that doesn't have any special case powerpc (except for that one
bit where it accepts symbols with STT_NOTYPE).

It would be nice to merge these one day. Although the tapsets.cxx one
does exactly the opposite from what we want in translate.cxx:

/*
 * The .opd section contains function descriptors that can look
 * just like function entry points.  For example, there's a function
 * descriptor called "do_exit" that links to the entry point ".do_exit".
 * Reject all symbols in .opd.
 */

Maybe this is a little too naive, but would it be as simple as the
attached? I don't actually have any powerpc machine to check.

Cheers,

Mark

diff --git a/translate.cxx b/translate.cxx
index e5038f9..fab3da4 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -5058,23 +5058,33 @@ dump_unwindsyms (Dwfl_Module *m,
                       && ! strcmp (name, "kretprobe_trampoline_holder"))
                     c->stp_kretprobe_trampoline_addr = sym_addr;
                 }
-              else if (n > 0)
-                {
-                  assert (secname != NULL);
-                  // secname adequately set
-
-                  // NB: it may be an empty string for ET_DYN objects
-                  // like shared libraries, as their relocation base
-                  // is implicit.
-                  if (secname[0] == '\0')
-                    secname = ".dynamic";
-                }
               else
-                {
-                  assert (n == 0);
-                  // sym_addr is absolute, as it must be since there are no relocation bases
-                  secname = ".absolute"; // sentinel
-                }
+		{
+#ifdef __powerpc__
+		  // ppc64 uses function descriptors in user space
+		  // XXX - actually check this is a legal address in .odp!
+		  if (GELF_ST_TYPE (sym.st_info) == STT_FUNC
+		      && name[0] != '.')
+		    Dwarf_Addr sym_addr = *((Dwarf_Addr *) sym_addr);
+#endif
+		  if (n > 0)
+		    {
+		      assert (secname != NULL);
+		      // secname adequately set
+
+		      // NB: it may be an empty string for ET_DYN objects
+		      // like shared libraries, as their relocation base
+		      // is implicit.
+		      if (secname[0] == '\0')
+			secname = ".dynamic";
+		    }
+		  else
+		    {
+		      assert (n == 0);
+		      // sym_addr is absolute, as it must be since there are no relocation bases
+		      secname = ".absolute"; // sentinel
+		    }
+		}
 
               // Compute our section number
               unsigned secidx;