This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2] Fix PPC64 ELF ABI v2 symbol address retrieval


Hey, posted the v3.


On 03/23/2015 06:55 PM, Mark Wielaard wrote:
It seems important to get this cleaned up.
Does the review below make sense?

On Fri, 2015-02-13 at 20:13 +0100, Mark Wielaard wrote:
On Fri, 2015-02-06 at 12:04 +0530, Hemant Kumar wrote:
@@ -2099,7 +2106,19 @@ query_dwarf_func (Dwarf_Die * func, dwarf_query * q)
            q->dw.function_line (&func.decl_line);
Dwarf_Addr entrypc;
-          if (q->dw.function_entrypc (&entrypc))
+          func.entrypc = 0;
+          /* Giving priority to sym_table */
+          if (q->dw.mod_info->sym_table)
+            {
+              func_info * fi;
+              fi = q->dw.mod_info->sym_table->lookup_symbol(func.name);
+              if (fi)
+                {
+                  func.entrypc = fi->addr;
+                  q->filtered_functions.push_back(func);
+                }
+            }
+          if (!func.entrypc && q->dw.function_entrypc (&entrypc))
              {
                func.entrypc = entrypc;
                q->filtered_functions.push_back (func);
I am still concerned about this lookup overriding the normal
function_entrypc. It would be good to guard this with a check to see if
it is necessary (only for a ppc64le target) so no extra lookups are
necessary in the normal case. But I believe in general it is wrong since
it only does the name lookup, and doesn't try to see if the address
matched. Take for example this program:

::::::::::::::
/tmp/baz.c
::::::::::::::
static int
foo (int v)
{
   return v + 1;
}

int
bar (int i)
{
   return foo (i - 1);
}

::::::::::::::
/tmp/main.c
::::::::::::::
int foo (int v)
{
   return bar (v - 1);
}

int
main (int argc, char **argv)
{
   return foo (argc);
}

gcc -g -c baz.c
gcc -g -c main.c
gcc -g -o prog baz.o main.o

This will have two function symbols called "foo" in the symbol table.
Since you only match on the name, you will likely get one of them wrong.
You could check before/after your patch with a simple script like:

stap -e 'process.function("foo") { printf ("%s: %x\n", pp(), uaddr()) }'
-c ./prog

@@ -8154,6 +8173,25 @@ symbol_table::get_from_elf()
        reject = reject_section(section);
  #endif
+ /*
+      * For ELF ABI v2 on PPC64 LE, we need to adjust sym.st_value corresponding
+      * to the bits of sym.st_other. These bits will tell us what's the offset
+      * of the local entry point from the global entry point.
+      */
+      Dwarf_Addr bias;
+      Elf* elf = (dwarf_getelf (dwfl_module_getdwarf (mod, &bias))
+             ?: dwfl_module_getelf (mod, &bias));
+
+      GElf_Ehdr ehdr_mem;
+      GElf_Ehdr* em = gelf_getehdr (elf, &ehdr_mem);
+      if (em == 0) { DWFL_ASSERT ("dwfl_getehdr", dwfl_errno()); }
+      assert(em);
This sanity check seems to confuse a couple of things.
There are 3 ways this could end up as NULL. Either dwarf_getelf or
dwfl_module_getelf returned NULL, then gelf_getehdr would return NULL
also. Or gelf_getehdr returns NULL because it got passed a faulty Elf.
All cases are very unlikely, because we wouldn't have arrived at this if
any of those cases were true. DWFL_ASSERT will print only the dwfl_errno
when set, but not the elf_errno or dwarf_errno. There also isn't
anything called dwfl_getehdr. Normally DWFL_ASSERT would throw on error,
so the assert is never reached. I would simply only assert (em). Or if
you really want to throw an error do:
  if (em == NULL) throw SEMANTIC_ERROR (_("Couldn't get elf header"));

Also please move this out of the loop. Just fetch the ehdr once at the
start.

+      /* st_other is currently only used with ABIv2 on ppc64 */
+      if ((em->e_machine == EM_PPC64) && (GELF_ST_TYPE(sym.st_info) == STT_FUNC)
+	  && sym.st_other)
+	addr = addr + PPC64_LOCAL_ENTRY_OFFSET(sym.st_other);
+
        if (name && GELF_ST_TYPE(sym.st_info) == STT_FUNC)
          add_symbol(name, (GELF_ST_BIND(sym.st_info) == STB_WEAK),
                     reject, addr, &high_addr);
Thanks,

Mark

--
Thanks,
Hemant Kumar


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]