Remove old commented out code

2006-08-27  Soren Sandmann <sandmann@redhat.com>

	* binparser.c: Remove old commented out code

2006-08-27  Soren Sandmann  <sandmann@redhat.com>

	* module/Makefile ($(MODULE).o): Add dependency on sysprof-module.h

	* elfparser.c (elf_parser_get_eh_frame): Add this function.
	Remove some commented out code.

ChangeLog

@@ -1,3 +1,14 @@
+2006-08-27  Soren Sandmann <sandmann@redhat.com>
+
+	* binparser.c: Remove old commented out code
+
+2006-08-27  Soren Sandmann  <sandmann@redhat.com>
+
+	* module/Makefile ($(MODULE).o): Add dependency on sysprof-module.h
+
+	* elfparser.c (elf_parser_get_eh_frame): Add this function.
+	Remove some commented out code.
+
 Sat Aug 26 19:05:51 2006  Søren Sandmann  <sandmann@redhat.com>
 
 	* Formatting fixes in header files.

TODO

@@ -32,6 +32,52 @@ Before 1.0.4:
 
 Before 1.2:
 
+* Elf bugs:
+
+	- when an elf file is read, it should be checked that the various
+	  sections are of the right type. For example the debug information
+	  for emacs is just a stub file where all the sections are NOBITS.
+
+	- Also error handling for bin_parser is necessary.
+
+	- Can .gnu_debuglink recurse?
+
+* Strategies for taking reliable stacktraces.
+
+	Three different kinds of files
+
+	- kernel
+	- vdso
+	- regular elf files
+
+	- kernel
+		- eh_frame annotations, in kernel or in kernel debug
+		- /proc/kallsyms
+		- userspace can look at _stext and _etext to determine 
+		  start and end of kernel text segment
+		- copying kernel stack to userspace
+		- heuristically determine functions based on address
+		- is eh_frame usually loaded into memory during normal
+		  operation
+
+	- vdso
+		- assume its the same across processes, just look at
+		  sysprof's own copy.
+		- send copy of it to userspace once, or for every
+		  sample
+
+	- regular elf
+		- usually have eh_frame section which is mapped into memory
+		  during normal operation
+		- is usually mapped into memory
+		- do stackwalk in kernel based on eh_frame
+		- do stackwalk in userland based on eh_frame
+		- do ebp based stackwalk in kernel
+		- do ebp based stackwalk in userland
+		- do heuristic stackwalk in kernel
+		- do heuristic stackwalk in userland
+		- 
+
 * "Expand all" is horrendously slow because update screenshot gets called 
   for every "expanded" signal.
 

binfile.c

@@ -68,6 +68,9 @@ separate_debug_file_exists (const char *name, guint32 crc)
     if (!parser)
 	return NULL;
 
+    g_print ("debug for the debug file: %s\n",
+	     elf_parser_get_debug_link (parser, &file_crc));
+    
     file_crc = elf_parser_get_crc32 (parser);
 
     if (file_crc != crc)
@@ -188,6 +191,7 @@ bin_file_lookup_symbol (BinFile    *bin_file,
 {
     if (bin_file->elf)
     {
+	g_print ("lookup in %s\n", bin_file->filename);
 	const ElfSym *sym = elf_parser_lookup_symbol (bin_file->elf, address);
 
 	if (sym)

binparser.c

@@ -174,6 +174,9 @@ convert_uint (const guchar *data,
     guint32 r32;
     guint64 r64;
 
+    if (width == 4)
+	g_print ("converting at %p  %d %d %d %d\n", data, data[0], data[1], data[2], data[3]);
+    
     switch (width)
     {
     case 1:
@@ -378,12 +381,18 @@ bin_record_get_uint (BinRecord *record,
     field = get_field (record->format, name);
     pos = record->parser->data + record->offset + field->offset;
 
+    g_print ("   record offset: %d\n", record->offset);
+    g_print ("   record index: %d\n", record->index);
+    g_print ("   field offset %d\n", field->offset);
+    
     if (record->offset + field->offset + field->width > record->parser->length)
     {
 	/* FIXME: generate error */
 	return 0;
     }
 
+    g_print ("    uint %d at %p    =>   %d\n", field->width, pos, convert_uint (pos, record->format->big_endian, field->width));
+    
     return convert_uint (pos, record->format->big_endian, field->width);
 }
 
@@ -415,210 +424,3 @@ bin_field_new_fixed_array (int n_elements,
     field->align = element_size;
     return field;
 }
-
-#if 0
-#include <elf.h>
-
-static gboolean
-find_elf_type (const guchar *data, gsize length,
-	       gboolean *is_64, gboolean *is_be)
-{
-    /* FIXME: this function should be able to return an error */
-    if (length < EI_NIDENT)
-	return FALSE;
-    
-    /* 32 or 64? */
-    
-    switch (data[EI_CLASS])
-    {
-    case ELFCLASS32:
-	*is_64 = FALSE;
-	break;
-	
-    case ELFCLASS64:
-	*is_64 = TRUE;
-	break;
-	
-    default:
-	/* return ERROR */
-	return FALSE;
-	break;
-    }
-    
-    /* big or little endian? */
-    switch (data[EI_DATA])
-    {
-    case ELFDATA2LSB:
-	*is_be = FALSE;
-	break;
-	
-    case ELFDATA2MSB:
-	*is_be = TRUE;
-	break;
-	
-    default:
-	/* return Error */
-	return FALSE;
-	break;
-    }
-    
-    g_print ("This elf file is %s %s\n",
-	     *is_64? "64 bit" : "32 bit",
-	     *is_be? "big endiann" : "little endian");
-    
-    return TRUE;
-}
-
-void
-parse_elf (const guchar *data,
-	   gsize length)
-{
-    gboolean is_64, is_big_endian;
-    BinFormat *elf_header;
-    BinFormat *shn_entry;
-    const guchar *section_header;
-    BinParser *parser;
-    BinParser *sh_parser;
-    BinFormat *sym;
-    int i;
-    
-    find_elf_type (data, length, &is_64, &is_big_endian);
-    
-    elf_header = bin_format_new (
-	is_big_endian,
-	"e_ident",	bin_field_new_fixed_array (EI_NIDENT, 1),
-	
-	"e_type",	bin_field_new_uint16 (),
-	"e_machine",	bin_field_new_uint16 (),
-	"e_version",	bin_field_new_uint32 (),
-	
-	"e_entry",	make_word (is_64),
-	"e_phoff",	make_word (is_64),
-	
-	"e_shoff",	make_word (is_64),
-	"e_flags",	make_uint32 (),
-	"e_ehsize",	make_uint16(),
-	"e_phentsize",	make_uint16 (),
-	"e_phnum",	make_uint16 (),
-	"e_shentsize",	make_uint16 (),
-	"e_shnum",	make_uint16 (),
-	"e_shstrndx",	make_uint16 (),
-	NULL);
-    
-    shn_entry = bin_format_new (
-	is_big_endian,
-	"sh_name",	make_uint32(),
-	"sh_type",	make_uint32(),
-	"sh_flags",	make_word (is_64),
-	"sh_addr",	make_word (is_64),
-	"sh_offset",	make_word (is_64),
-	"sh_size",	make_word (is_64),
-	"sh_link",	make_uint32(),
-	"sh_info",	make_uint32(),
-	"sh_addralign",	make_word (is_64),
-	"sh_entsize",	make_word (is_64),
-	NULL);
-    
-    if (is_64)
-    {
-	sym = bin_format_new (
-	    is_big_endian,
-	    "st_name",		make_uint32(),
-	    "st_info",		make_uint8 (),
-	    "st_other",		make_uint8 (),
-	    "st_shndx",		make_uint16 (),
-	    "st_value",		make_uint64 (),
-	    "st_size",		make_uint64 (),
-	    NULL);
-    }
-    else
-    {
-	sym = bin_format_new (
-	    is_big_endian,
-	    "st_name",		make_uint32 (),
-	    "st_value",		make_uint32 (),
-	    "st_size",		make_uint32 (),
-	    "st_info",		make_uint8 (),
-	    "st_other",		make_uint8 (),
-	    "st_shndx",		make_uint16 ());
-    }
-    
-    parser = bin_parser_new (elf_header, data, length);
-    
-    section_header = data + bin_parser_get_uint (parser, "e_shoff");
-    
-#if 0
-    g_print ("section header offset: %u\n",
-	     section_header - data);
-    
-    g_print ("There are %llu sections\n",
-	     bin_parser_get_uint (parser, "e_shnum"));
-#endif
-    
-    /* should think through how to deal with offsets, and whether parsers
-     * are always considered parsers of an array. If yes, then it
-     * may be reasonable to just pass the length of the array.
-     *
-     * Hmm, although the parser still needs to know the end of the data.
-     * Maybe create yet another structure, a subparser, that also contains
-     * an offset in addition to the beginning and length.
-     *
-     * Ie., bin_sub_parser_new (parser, section_header, shn_entry, n_headers);
-     *
-     * In that case, it might be interesting to merge format and parser,
-     * and just call it 'file' or something, then call the subparser "parser"
-     *
-     * Also, how do we deal with strings?
-     *
-     * "asdf", make_string()?
-     *
-     */
-    sh_parser = bin_parser_new (shn_entry, section_header, (guint)-1);
-    
-    for (i = 0; i < bin_parser_get_uint (parser, "e_shnum"); ++i)
-    {
-#if 0
-	bin_parser_set_index (sh_parser, i);
-#endif
-	
-#if 0
-	bin_parser_get_uint
-	    parser, data + i * bin_format_length (shn_entry));
-    section_header =
-	data + bin_parser_get_uint (parser, "e_shoff");
-    
-    parser = bin_parser_new (
-#endif
-	}
-	
-#if 0
-	bin_format_array_get_string (shn_table, data, "sh_name");
-    
-    bin_format_array_get_uint (shn_table, data, "sh_addr");
-#endif
-}
-
-static void
-disaster (const char *str)
-{
-    g_printerr ("%s\n", str);
-    
-    exit (-1);
-}
-
-int
-main ()
-{
-    GMappedFile *libgtk;
-    
-    libgtk = g_mapped_file_new ("/usr/lib/libgtk-x11-2.0.so", FALSE, NULL);
-    
-    if (!libgtk)
-	disaster ("Could not map the file\n");
-    
-    parse_elf ((const guchar *)g_mapped_file_get_contents (libgtk),
-	       g_mapped_file_get_length (libgtk));
-    
-    return 0 ;
-} 
-#endif

collector.c

@@ -129,7 +129,6 @@ on_read (gpointer data)
     if (diff >= 0.0 && diff < RESET_DEAD_PERIOD)
 	return;
     
-#if 0
     {
 	int i;
 	g_print ("pid: %d (%d)\n", trace.pid, trace.n_addresses);
@@ -137,7 +136,6 @@ on_read (gpointer data)
 	    g_print ("rd: %08x\n", trace.addresses[i]);
 	g_print ("-=-\n");
     }
-#endif
     
     if (rd > 0)
     {

elfparser.c

@@ -55,9 +55,17 @@ section_new (BinRecord *record,
     
     section->name = bin_record_get_string_indirect (
 	record, "sh_name", name_table);
+
+    g_print ("new section: %s\n", section->name);
+    
     section->size = bin_record_get_uint (record, "sh_size");
+
+    g_print ("size: %d\n", section->size);
+    
     section->offset = bin_record_get_uint (record, "sh_offset");
 
+    g_print ("offset: %d\n", section->offset);
+
     flags = bin_record_get_uint (record, "sh_flags");
     section->allocated = !!(flags & SHF_ALLOC);
 
@@ -81,14 +89,21 @@ find_section (ElfParser *parser,
 {
     int i;
 
+    g_print ("looking for section %s ... ", name);
+    
     for (i = 0; i < parser->n_sections; ++i)
     {
 	Section *section = parser->sections[i];
 	
 	if (strcmp (section->name, name) == 0)
+	{
+	    g_print ("found it as number %d with offset %d\n", i, section->offset);
 	    return section;
+	}
     }
 
+    g_print ("not found\n");
+    
     return NULL;
 }
 
@@ -122,6 +137,9 @@ parser_new_from_data (const guchar *data, gsize length)
     parser->n_sections = bin_record_get_uint (elf_header, "e_shnum");
     section_names_idx = bin_record_get_uint (elf_header, "e_shstrndx");
     section_headers = bin_record_get_uint (elf_header, "e_shoff");
+    g_print ("e_shoff  %d\n", section_headers);
+    g_print ("header size: %d\n", bin_record_get_uint (elf_header, "e_shentsize"));
+    g_print ("real size: %d\n", bin_format_get_size (parser->shn_entry));
     
     bin_record_free (elf_header);
     
@@ -162,6 +180,8 @@ elf_parser_new (const char *filename,
     data = (guchar *)g_mapped_file_get_contents (file);
     length = g_mapped_file_get_length (file);
 
+    g_print ("data %p: for %s\n", data, filename);
+    
     parser = parser_new_from_data (data, length);
 
     parser->file = file;
@@ -305,10 +325,10 @@ read_table (ElfParser *parser,
     parser->n_symbols = sym_table->size / sym_size;
     parser->symbols = g_new (ElfSym, parser->n_symbols);
     
-#if 0
     g_print ("\nreading %d symbols (@%d bytes) from %s\n",
 	     parser->n_symbols, sym_size, sym_table->name);
-#endif
+
+    g_print ("table offset: %d\n", str_table->offset);
     
     symbol = bin_parser_get_record (parser->parser, parser->sym_format, sym_table->offset);
 
@@ -367,6 +387,7 @@ read_symbols (ElfParser *parser)
     }
     else if (dynsym && dynstr)
     {
+	g_print ("reading from dynstr at offset %d\n", dynstr->offset);
 	read_table (parser, dynsym, dynstr);
     }
     else
@@ -498,36 +519,16 @@ elf_parser_get_debug_link (ElfParser *parser, guint32 *crc32)
     return result;
 }
 
-#if 0
+const guchar *
-get_debug_link_info (bfd *abfd, unsigned long *crc32_out)
+elf_parser_get_eh_frame   (ElfParser   *parser)
 {
-    asection *sect;
+    const Section *eh_frame = find_section (parser, ".eh_frame");
-    bfd_size_type debuglink_size;
-    unsigned long crc32;
-    char *contents;
-    int crc_offset;
 
-    sect = bfd_get_section_by_name (abfd, ".gnu_debuglink");
+    if (eh_frame)
-    
+	return bin_parser_get_data (parser->parser) + eh_frame->offset;
-    if (sect == NULL)
+    else
 	return NULL;
-    
-    debuglink_size = bfd_section_size (abfd, sect);
-    
-    contents = g_malloc (debuglink_size);
-    bfd_get_section_contents (abfd, sect, contents,
-			      (file_ptr)0, (bfd_size_type)debuglink_size);
-    
-    /* Crc value is stored after the filename, aligned up to 4 bytes. */
-    crc_offset = strlen (contents) + 1;
-    crc_offset = (crc_offset + 3) & ~3;
-    
-    crc32 = bfd_get_32 (abfd, (bfd_byte *) (contents + crc_offset));
-    
-    *crc32_out = crc32;
-    return contents;
 }
-#endif
 
 const char *
 elf_parser_get_sym_name (ElfParser *parser,

elfparser.h

@@ -3,12 +3,12 @@
 typedef struct ElfSym ElfSym;
 typedef struct ElfParser ElfParser;
 
-ElfParser * elf_parser_new            (const char  *filename,
+ElfParser *   elf_parser_new            (const char  *filename,
-				       GError     **err);
+					 GError     **err);
-void        elf_parser_free           (ElfParser   *parser);
+void          elf_parser_free           (ElfParser   *parser);
-const char *elf_parser_get_debug_link (ElfParser   *parser,
+const char *  elf_parser_get_debug_link (ElfParser   *parser,
-				       guint32     *crc32);
+					 guint32     *crc32);
-
+const guchar *elf_parser_get_eh_frame   (ElfParser   *parser);
 
 /* Lookup a symbol in the file.
  *

module/Makefile

@@ -28,7 +28,7 @@ endif
 
 # build module
 
-$(MODULE).o: $(MODULE).c
+$(MODULE).o: $(MODULE).c $(MODULE).h
 	$(KMAKE) modules
 
 

module/sysprof-module.c

@@ -126,6 +126,7 @@ timer_notify (struct pt_regs *regs)
 	StackFrame frame;
 	int result;
 	static atomic_t in_timer_notify = ATOMIC_INIT(1);
+	int stacksize;
 
 	if (((++get_cpu_var(n_samples)) % INTERVAL) != 0)
 		return 0;
@@ -158,10 +159,40 @@ timer_notify (struct pt_regs *regs)
 
 	frame_pointer = (void *)regs->REG_FRAME_PTR;
 
+	{
+		/* In principle we should use get_task_mm() but
+		 * that will use task_lock() leading to deadlock
+		 * if somebody already has the lock
+		 */
+		if (spin_is_locked (&current->alloc_lock))
+			printk ("alreadylocked\n");
+		{
+			struct mm_struct *mm = current->mm;
+			if (mm)
+			{
+				printk (KERN_ALERT "stack size: %d (%d)\n",
+					mm->start_stack - regs->REG_STACK_PTR,
+					current->pid);
+				
+				stacksize = mm->start_stack - regs->REG_STACK_PTR;
+			}
+			else
+				stacksize = 1;
+		}
+#if 0
+		else
+			printk (KERN_ALERT "could not lock on %d\n", current->pid);
+#endif
+	}
+	
+	if (stacksize < 100000)
+		goto out;
+
 	while (((result = read_frame (frame_pointer, &frame)) == 0)	&&
 	       i < SYSPROF_MAX_ADDRESSES				&&
 	       (unsigned long)frame_pointer >= regs->REG_STACK_PTR)
 	{
+		printk ("frame pointer: %p (retaddr: %p)\n", frame_pointer, frame.return_address);
 		trace->addresses[i++] = (void *)frame.return_address;
 		frame_pointer = (StackFrame *)frame.next;
 	}

testelf.c

@@ -44,9 +44,11 @@ main ()
 	return -1;
     }
 
+    g_print ("eh frame starts at %p\n", elf_parser_get_eh_frame (elf));
+    
     elf_parser_get_crc32 (elf);
     
-    for (i = 0; i < 5000000; ++i)
+    for (i = 0; i < 1; ++i)
     {
 	elf_parser_get_crc32 (elf);
 	check (elf, 0x077c80f0 - (0x07787000 - 0)); /* gtk_about_dialog_set_artists  (add - (map - offset)) */