Re: dwarfless failures on ppc64

[Jim Keniston <jkenisto at us dot ibm dot com>]

Srinivasa, here's another patch to correct handling of weak symbols, as
described below.

On Wed, 2008-05-21 at 14:26 -0700, Jim Keniston wrote:
> On Tue, 2008-05-20 at 15:58 -0700, Roland McGrath wrote:
> > > The problem with 'W' entries in nm listings is that some of them refer
> > > to "real" functions and some don't.  
> > 
> ...
> > 
> > What is true is that there may be many symbols that are all aliases for
> > the same actual address.  Some of those symbols might be weak and others
> > not (also some might be local).  In the kernel there is one case where
> > this is commonly true, that is many weak symbols are aliases for
> > sys_ni_syscall (which is a stub function that returns (long)-ENOSYS).
> 
> Yes, that's exactly what I'm seeing on my system.  The weak syscall
> symbols are created via the cond_syscall macro.
> 
> > 
> > I don't have a clear sense from what you've said of exactly how
> > systemtap is behaving.  If you probe sys_* for example, that will
> > include multiple names that resolve to the same address as
> > sys_ni_syscall, and so is requesting several different probes at one
> > address.
> 
> Without --kelf or --kmap, stap consults only the dwarf.  Since
> cond_syscall doesn't yield any dwarf, the "functions" defined thereby
> don't get considered by stap.
> 
> With --kelf, the weak syscall symbols look like any other text symbols
> -- we don't check STB_WEAK -- so they get considered.  This isn't good
> in cases like
> 
> probe syscall.foo = kernel.function("sys_foo_weak") ?
> 		kernel.function("compat_sys_foo") { ... }
> 
> if sys_foo_weak is an alias for sys_ni_syscall and compat_sys_foo
> actually implements the system call.
> 
> With --kmap, we currently ignore ALL weak symbols.  This is plainly
> wrong, in light of the various weak symbols that correspond to non-stub
> functions.
> 
> I think the following fix should provide the desired behavior, at least
> wrt syscalls: For --kelf or --kmap, consider all weak symbols except
> those with the same value as sys_ni_syscall.
> 
> Comments/corrections welcome.
> 
> > 
> > 
> > Thanks,
> > Roland
> 
> Thanks very much.
> Jim
> 

Srini, attached are two patches:
1. powerpc_symtab.patch, which I sent you Monday
2. weak_symbols.patch, which applies atop #1

Please let me know whether these fixes resolve the powerpc failures
you're seeing.  If not, please send me your results, as previously
discussed.

Thanks.
Jim

--- a/tapsets.cxx	2008-05-19 12:02:37.000000000 -0700
+++ b/tapsets.cxx	2008-05-19 12:02:51.000000000 -0700
@@ -4681,6 +4681,11 @@
 symbol_table::add_symbol(const char *name, Dwarf_Addr addr,
                                      Dwarf_Addr *high_addr)
 {
+#ifdef __powerpc__
+  // Map ".sys_foo" to "sys_foo".
+  if (name[0] == '.')
+    name++;
+#endif
   func_info *fi = new func_info();
   fi->addr = addr;
   fi->name = name;

--- b/tapsets.cxx	2008-05-19 12:02:51.000000000 -0700
+++ c/tapsets.cxx	2008-05-21 16:52:35.000000000 -0700
@@ -468,7 +468,7 @@
 func_info
 {
   func_info()
-    : decl_file(NULL), decl_line(-1), addr(0), prologue_end(0)
+    : decl_file(NULL), decl_line(-1), addr(0), prologue_end(0), weak(false)
   {
     memset(&die, 0, sizeof(die));
   }
@@ -478,6 +478,7 @@
   Dwarf_Die die;
   Dwarf_Addr addr;
   Dwarf_Addr prologue_end;
+  bool weak;
 };
 
 struct
@@ -551,12 +552,14 @@
   map<string, func_info*> map_by_name;
   vector<func_info*> list_by_addr;
 
-  void add_symbol(const char *name, Dwarf_Addr addr, Dwarf_Addr *high_addr);
+  void add_symbol(const char *name, bool weak, Dwarf_Addr addr,
+                                               Dwarf_Addr *high_addr);
   enum info_status read_symbols(FILE *f, const string& path);
   enum info_status read_from_elf_file(const string& path);
   enum info_status read_from_text_file(const string& path);
   enum info_status get_from_elf();
   void mark_dwarf_redundancies(dwflpp *dw);
+  void purge_syscall_stubs();
   func_info *lookup_symbol(const string& name);
   Dwarf_Addr lookup_symbol_address(const string& name);
   func_info *get_func_containing_address(Dwarf_Addr addr);
@@ -4678,8 +4681,8 @@
 }
 
 void
-symbol_table::add_symbol(const char *name, Dwarf_Addr addr,
-                                     Dwarf_Addr *high_addr)
+symbol_table::add_symbol(const char *name, bool weak, Dwarf_Addr addr,
+                                                      Dwarf_Addr *high_addr)
 {
 #ifdef __powerpc__
   // Map ".sys_foo" to "sys_foo".
@@ -4689,6 +4692,7 @@
   func_info *fi = new func_info();
   fi->addr = addr;
   fi->name = name;
+  fi->weak = weak;
   map_by_name[fi->name] = fi;
   // TODO: Use a multimap in case there are multiple static
   // functions with the same name?
@@ -4739,8 +4743,8 @@
           free(mod);
           goto done;
         }
-      if (type == 'T' || type == 't')
-        add_symbol(name, (Dwarf_Addr) addr, &high_addr);
+      if (type == 'T' || type == 't' || type == 'W')
+        add_symbol(name, (type == 'W'), (Dwarf_Addr) addr, &high_addr);
       free(name);
     }
 
@@ -4805,9 +4809,11 @@
   for (int i = 1; i < syments; ++i)
     {
       GElf_Sym sym;
-      const char *name = dwfl_module_getsym(mod, i, &sym, NULL);
-      if (name && GELF_ST_TYPE(sym.st_info) == STT_FUNC)
-        add_symbol(name, sym.st_value, &high_addr);
+      GElf_Word section;
+      const char *name = dwfl_module_getsym(mod, i, &sym, &section);
+      if (name && GELF_ST_TYPE(sym.st_info) == STT_FUNC && section != SHN_UNDEF)
+        add_symbol(name, (GELF_ST_BIND(sym.st_info) == STB_WEAK),
+                                              sym.st_value, &high_addr);
     }
   return info_present;
 }
@@ -4899,6 +4905,33 @@
   return 0;
 }
 
+// This is the kernel symbol table.  The kernel macro cond_syscall creates
+// a weak symbol for each system call and maps it to sys_ni_syscall.
+// For system calls not implemented elsewhere, this weak symbol shows up
+// in the kernel symbol table.  Following the precedent of dwarfful stap,
+// we refuse to consider such symbols.  Here we delete them from our
+// symbol table.
+// TODO: Consider generalizing this and/or making it part of blacklist
+// processing.
+void
+symbol_table::purge_syscall_stubs()
+{
+  Dwarf_Addr stub_addr = lookup_symbol_address("sys_ni_syscall");
+  if (stub_addr == 0)
+    return;
+  for (size_t i = 0; i < list_by_addr.size(); i++)
+    {
+      func_info *fi = list_by_addr.at(i);
+      if (fi->weak && fi->addr == stub_addr && fi->name != "sys_ni_syscall")
+        {
+	  list_by_addr.erase(list_by_addr.begin()+i);
+	  map_by_name.erase(fi->name);
+	  delete fi;
+	  i--;
+	}
+    }
+}
+
 void
 module_info::get_symtab(dwarf_query *q)
 {
@@ -4954,6 +4987,9 @@
   // precedes the call to query_module_symtab().  So we should never read
   // a module's symbol table without first having tried to get its dwarf.
   sym_table->mark_dwarf_redundancies(&q->dw);
+
+  if (name == TOK_KERNEL)
+    sym_table->purge_syscall_stubs();
 }
 
 module_info::~module_info()