Add commented out code to reject callback.

2008-02-17  Soren Sandmann  <sandmann@redhat.com>

	* collector.c (lookup_symbol): Add commented out code to reject
	callback.

	* elfparser.c (struct ElfParser): Store the filename if any
	(elf_parser_get_sym_address): Subtract the load address, so the
	result will be an offset into the text section.

	* process.[ch] (process_lookup_symbol): Add an offset out-argument
	
	* binfile.[ch] (bin_symbol_get_address): New function

	* TODO: updates



svn path=/trunk/; revision=397

ChangeLog

@@ -1,3 +1,18 @@
+2008-02-17  Soren Sandmann  <sandmann@redhat.com>
+
+	* collector.c (lookup_symbol): Add commented out code to reject
+	callback.
+
+	* elfparser.c (struct ElfParser): Store the filename if any
+	(elf_parser_get_sym_address): Subtract the load address, so the
+	result will be an offset into the text section.
+
+	* process.[ch] (process_lookup_symbol): Add an offset out-argument
+	
+	* binfile.[ch] (bin_symbol_get_address): New function
+
+	* TODO: updates
+
 2008-02-16  Soren Sandmann  <sandmann@redhat.com>
 
 	* module/sysprof-module.c (nt_memcpy): Add a memcpy() that uses

TODO

@@ -44,7 +44,7 @@ Before 1.2:
 		  - GObject signal emission overhead accounts for 18% of
 		    the time. 
 	Consider adding a forked version of GtkTreeStore with
-	performance fixes.
+	performance and bug fixes.
 
 * Make sure that labels look decent in case of "No Map" etc.
 
@@ -363,6 +363,17 @@ Before 1.2:
 		  one can be completed using the DWARF information for
 		  the shared part.
 
+* Notes on heuristic stack walking
+
+  - We can reject addresses that point exactly to the beginning of a
+    function since these are likely callbacks. Note though that the
+    first time a function in a shared library is called, it goes
+    through dynamic linker resolution which will cause the stack to
+    contain a callback of the function. This needs to be investigated
+    in more detail.
+
+  - We are already rejecting addresses outside the text section
+    (addresses of global variables and the like).
 
 * How to get the user stack:
 

binfile.c

@@ -407,3 +407,13 @@ bin_symbol_get_name (BinFile *file,
     else
 	return elf_parser_get_sym_name (file->elf, (const ElfSym *)symbol);
 }
+
+gulong
+bin_symbol_get_address (BinFile         *file,
+			const BinSymbol *symbol)
+{
+    if (file->undefined_name == (char *)symbol)
+	return 0x0;
+    else
+	return file->text_offset + elf_parser_get_sym_address (file->elf, (const ElfSym *)symbol);
+}

binfile.h

@@ -40,5 +40,7 @@ gboolean         bin_file_check_inode   (BinFile         *bin_file,
 					 ino_t		  inode);
 const char *     bin_symbol_get_name    (BinFile         *bin_file,
 					 const BinSymbol *symbol);
+gulong		 bin_symbol_get_address (BinFile         *bin_file,
+					 const BinSymbol *symbol);
 
 #endif

collector.c

@@ -383,7 +383,7 @@ static char *
 lookup_symbol (Process *process, gpointer address,
 	       GHashTable *unique_symbols,
 	       gboolean kernel,
-	       gboolean first_kernel_addr)
+	       gboolean first_addr)
 {
     const char *sym;
 
@@ -396,11 +396,11 @@ lookup_symbol (Process *process, gpointer address,
 
 	/* If offset is 0, it is a callback, not a return address.
 	 *
-	 * If "first_kernel_addr" is true, then the address is an
+	 * If "first_addr" is true, then the address is an
 	 * instruction pointer, not a return address, so it may
 	 * legitimately be at offset 0.
 	 */
-	if (offset == 0 && !first_kernel_addr)
+	if (offset == 0 && !first_addr)
 	    sym = NULL;
 
 	/* If offset is greater than 4096, then what happened is most
@@ -416,7 +416,19 @@ lookup_symbol (Process *process, gpointer address,
     }
     else
     {
-	sym = process_lookup_symbol (process, (gulong)address);
+	gulong offset;
+	
+	sym = process_lookup_symbol (process, (gulong)address, &offset);
+
+	if (offset == 0 && !first_addr)
+	{
+#if 0
+	    sym = g_strdup_printf ("%s [callback]", sym);
+	    g_print ("rejecting %s since it looks like a callback\n",
+		     sym);
+	    sym = NULL;
+#endif
+	}
     }
     
     if (sym)
@@ -435,7 +447,7 @@ resolve_symbols (GList *trace, gint size, gpointer data)
     GPtrArray *resolved_trace = g_ptr_array_new ();
     char *cmdline;
     gboolean in_kernel = FALSE;
-    gboolean first_kernel_addr = TRUE;
+    gboolean first_addr = TRUE;
     
     for (list = trace; list && list->next; list = list->next)
     {
@@ -452,8 +464,8 @@ resolve_symbols (GList *trace, gint size, gpointer data)
 	    in_kernel = FALSE;
 	
 	symbol = lookup_symbol (process, address, info->unique_symbols,
-				in_kernel, first_kernel_addr);
-	first_kernel_addr = FALSE;
+				in_kernel, first_addr);
+	first_addr = FALSE;
 
 	if (symbol)
 	    g_ptr_array_add (resolved_trace, symbol);

elfparser.c

@@ -57,6 +57,8 @@ struct ElfParser
     gsize		sym_strings;
     
     GMappedFile *	file;
+
+    char *		filename;
     
     const Section *	text_section;
 };
@@ -213,6 +215,8 @@ elf_parser_new_from_data (const guchar *data,
     parser->text_section = find_section (parser, ".text", SHT_PROGBITS);
     if (!parser->text_section)
 	parser->text_section = find_section (parser, ".text", SHT_NOBITS);
+
+    parser->filename = NULL;
     
     return parser;
 }
@@ -252,6 +256,8 @@ elf_parser_new (const char *filename,
 	g_mapped_file_free (file);
 	return NULL;
     }
+
+    parser->filename = g_strdup (filename);
     
     parser->file = file;
     
@@ -356,6 +362,9 @@ elf_parser_free (ElfParser *parser)
     g_free (parser->symbols);
     
     bin_parser_free (parser->parser);
+
+    if (parser->filename)
+	g_free (parser->filename);
     
     g_free (parser);
 }
@@ -461,18 +470,16 @@ read_table (ElfParser *parser,
 	    n_functions++;
 
 #if 0
-	    g_print ("symbol: %s:   %d\n", get_string_indirect (parser->parser,
+	    g_print ("    symbol: %s:   %lx\n", get_string_indirect (parser->parser,
 								   parser->sym_format, "st_name",
-								   str_table->offset), addr);
-#endif
-#if 0
-	    g_print ("   sym %d in %p (info: %d:%d) (func:global  %d:%d)\n", addr, parser, info & 0xf, info >> 4, STT_FUNC, STB_GLOBAL);
+								   str_table->offset), addr - parser->text_section->load_address);
+	    g_print ("        sym %d in %p (info: %d:%d) (func:global  %d:%d)\n", addr, parser, info & 0xf, info >> 4, STT_FUNC, STB_GLOBAL);
 #endif
 	}
 	else if (addr != 0)
 	{
 #if 0
-	    g_print ("   rejecting %d in %p (info: %d:%d) (func:global  %d:%d)\n", addr, parser, info & 0xf, info >> 4, STT_FUNC, STB_GLOBAL);
+	    g_print ("        rejecting %d in %p (info: %d:%d) (func:global  %d:%d)\n", addr, parser, info & 0xf, info >> 4, STT_FUNC, STB_GLOBAL);
 #endif
 	}
 	
@@ -496,10 +503,16 @@ read_symbols (ElfParser *parser)
     
     if (symtab && strtab)
     {
+#if 0
+	g_print ("reading symbol table of %s\n", parser->filename);
+#endif
 	read_table (parser, symtab, strtab);
     }
     else if (dynsym && dynstr)
     {
+#if 0
+	g_print ("reading dynamic symbol table of %s\n", parser->filename);
+#endif
 	read_table (parser, dynsym, dynstr);
     }
     else
@@ -565,7 +578,7 @@ elf_parser_lookup_symbol (ElfParser *parser,
 	return NULL;
     
     address += parser->text_section->load_address;
-    
+
 #if 0
     g_print ("elf: the address we are looking up is %p\n", address);
 #endif
@@ -596,6 +609,13 @@ elf_parser_lookup_symbol (ElfParser *parser,
 	    result = NULL;
     }
     
+    if (result)
+    {
+	/* Reject the symbols if the address is outside the text section */
+	if (address > parser->text_section->load_address + parser->text_section->size)
+	    result = NULL;
+    }
+
     return result;
 }
 
@@ -662,7 +682,7 @@ gulong
 elf_parser_get_sym_address (ElfParser *parser,
 			    const ElfSym *sym)
 {
-    return sym->address;
+    return sym->address - parser->text_section->load_address;
 }
 
 /*

process.c

@@ -673,7 +673,7 @@ process_lookup_kernel_symbol (gulong address,
 }
 
 const char *
-process_lookup_symbol (Process *process, gulong address)
+process_lookup_symbol (Process *process, gulong address, gulong *offset)
 {
     static const char *const kernel = "kernel";
     const BinSymbol *result;
@@ -752,6 +752,17 @@ process_lookup_symbol (Process *process, gulong address)
 #endif
     
 /*     g_print ("(%x) %x %x name; %s\n", address, map->start, map->offset, result->name); */
+
+#if 0
+    g_print ("name: %s (in %s)\n", bin_symbol_get_name (map->bin_file, result), map->filename);
+    g_print ("  in addr: %lx\n", address);
+    g_print ("  out addr: %lx\n", bin_symbol_get_address (map->bin_file, result));
+    g_print ("  map start: %lx\n", map->start);
+    g_print ("  map offset: %lx\n", map->offset);
+#endif
+
+    if (offset)
+	*offset = address - bin_symbol_get_address (map->bin_file, result);
     
     return bin_symbol_get_name (map->bin_file, result);
 }

process.h

@@ -52,7 +52,8 @@ void          process_ensure_map                  (Process    *process,
 						   int         pid,
 						   gulong      address);
 const char *  process_lookup_symbol		  (Process    *process,
-						   gulong      address);
+						   gulong      address,
+						   gulong     *offset);
 const char *  process_get_cmdline                 (Process    *process);
 void	      process_flush_caches                (void);
 const guint8 *process_get_vdso_bytes		  (gsize      *length);