[sysprof/ftrace: 4/16] A number of clean-ups

[Søren Sandmann Pedersen <ssp src gnome org>]

commit aa8e875281eedc752d2d124f9819a70a51f1e393
Author: Soren Sandmann <sandmann daimi au dk>
Date:   Sat May 10 23:01:20 2008 +0000

    A number of clean-ups
    
    TODO: Add note about weird stack frames in vdso

 TODO        |   16 ++++
 collector.c |  258 +++++++++++++++++------------------------------------------
 2 files changed, 89 insertions(+), 185 deletions(-)
---
diff --git a/TODO b/TODO
index 2938aae..67fcb33 100644
--- a/TODO
+++ b/TODO
@@ -96,6 +96,22 @@ Before 1.2:
 	- Does get_vdso_bytes() belong in process.c?
 	- Is basing on "[vdso]" always correct?
 
+  On some systems (or some kernels?) the vdso contains this:
+
+  ffffe414 <__kernel_vsyscall>:
+  ffffe414:	51                   	push   %ecx
+  ffffe415:	52                   	push   %edx
+  ffffe416:	55                   	push   %ebp
+  ffffe417:	89 e5                	mov    %esp,%ebp
+  ffffe419:	0f 34                	sysenter 
+
+  Note that push %ecx; push %edx come before push %ebp which results
+  in the return address not being at its usual position. This will have
+  to be corrected in the kernel:
+
+     if (in_vdso() && vdso_has_unusual_stack_layout())
+     	    copy *(ebp + 12) rather than *(ebp + 4)
+
 * Convert things like [heap] and [stack] to more understandable labels.
 
 * Strategies for taking reliable stacktraces.
diff --git a/collector.c b/collector.c
index 8b61494..76a5799 100644
--- a/collector.c
+++ b/collector.c
@@ -146,7 +146,8 @@ add_trace_to_stash (const SysprofStackTrace *trace,
 	
 	if (!process_ensure_map (process, trace->pid, addr))
 	{
-	    g_print ("user address #%d, %p, was unmapped in %d (%d kernels)\n", i, addr, trace->pid, trace->n_kernel_words);
+	    g_print ("user address #%d, %p, was unmapped in %d (%d kernels)\n",
+		     i, (void *)addr, trace->pid, trace->n_kernel_words);
 	    bad = TRUE;
 	}
 	    
@@ -318,6 +319,18 @@ collector_set_tracing (Collector *collector,
 	write_text ("/sys/kernel/debug/tracing/tracing_enabled", "0\n");
 }
 
+/* Format of a trace entry:
+ *
+ *
+ *  pid			4 bytes
+ *  cpu			1 byte
+ *  t			8 bytes
+ *  arg1 (type)		4 bytes on 32 bit, 8 bytes on 64
+ *  arg2 (address)	4 bytes on 32 bit, 8 bytes on 64
+ *  arg3 (location)	4 bytes on 32 bit, 8 bytes on 64
+ *
+ *  TOTAL: 25 bytes on x86
+ */
 #define ENTRY_SIZE  (							\
 	sizeof (guint32) +  /* pid */					\
 	sizeof (guint8) +   /* cpu */					\
@@ -355,123 +368,43 @@ read8 (const guchar *line)
     return (rh << 32) | rl;
 }
 
-/* Format of a trace entry:
- *
- *
- *  pid			4 bytes
- *  cpu			1 byte
- *  t			8 bytes
- *  arg1 (type)		4 bytes on 32 bit, 8 bytes on 64
- *  arg2 (address)	4 bytes on 32 bit, 8 bytes on 64
- *  arg3 (location)	4 bytes on 32 bit, 8 bytes on 64
- *
- *  TOTAL: 25 bytes on x86
- */
-static const guchar *
-parse_line (const guchar *line, int len, Entry *entry)
+static void
+read_entry (const guchar *input, Entry *entry)
 {
-    char *eol;
-
-    if (len >= 25)
+    entry->pid = read4 (input + 0);
+    entry->cpu = *(input + 4);
+    entry->timestamp = read8 (input + 5);
+    entry->type = read4 (input + 13);
+    entry->address = read4 (input + 17);
+    entry->location = read4 (input + 21);
+    
+    if (entry->type < -1 || entry->type > 3 || entry->pid < 0)
     {
 	int i;
-
-#if 0
-	g_print ("%p: \n", line);
-	for (i = 0; i < 25; ++i)
-	{
-	    g_print ("%2x ", (unsigned char)line[i]);
-	}
-	g_print ("\n");
-#endif
 	
-	entry->pid = read4 (line + 0);
-	entry->cpu = *(line + 4);
-	entry->timestamp = read8 (line + 5);
-	entry->type = read4 (line + 13);
-	entry->address = read4 (line + 17);
-	entry->location = read4 (line + 21);
-
-#if 0
-	g_print ("Entry:\n");
-	g_print ("- pid: %d (%x)\n", entry->pid, entry->pid);
-	g_print ("- cpu: %d\n", entry->cpu);
-	g_print ("- timestamp: %lld\n", entry->timestamp);
-	g_print ("- type: %ld\n", entry->type);
-	g_print ("- address: %ld\n", entry->address);
-	g_print ("- location: %ld\n", entry->location);
-#endif
-
-	if (entry->type < -1 || entry->type > 3 || entry->pid < 0)
-	{
-	    g_printerr ("bad entry: %d\n", entry->type);
-	    g_printerr ("Entry:\n");
-	    g_printerr ("- pid: %d (%x)\n", entry->pid, entry->pid);
-	    g_printerr ("- cpu: %d\n", entry->cpu);
-	    g_printerr ("- timestamp: %lld\n", entry->timestamp);
-	    g_printerr ("- type: %ld\n", entry->type);
-	    g_printerr ("- address: %ld\n", entry->address);
-	    g_printerr ("- location: %ld\n", entry->location);
-	    g_printerr ("%p: \n", line);
-	    for (i = 0; i < 25; ++i)
-	    {
-		g_printerr ("%2x ", (unsigned char)line[i]);
-	    }
-	    g_printerr ("\n");
-	    
-	    g_assert (FALSE);
-	}
-	
-	return line + 25;
+	g_printerr ("bad entry: %ld\n", entry->type);
+	g_printerr ("Entry:\n");
+	g_printerr ("- pid: %d (%x)\n", entry->pid, entry->pid);
+	g_printerr ("- cpu: %d\n", entry->cpu);
+	g_printerr ("- timestamp: %lld\n", entry->timestamp);
+	g_printerr ("- type: %ld\n", entry->type);
+	g_printerr ("- address: %ld\n", entry->address);
+	g_printerr ("- location: %ld\n", entry->location);
+
+	g_printerr ("%p: \n", input);
+	for (i = 0; i < ENTRY_SIZE; ++i)
+	    g_printerr ("%2x ", (unsigned char)input[i]);
+	g_printerr ("\n");
+
+	g_assert (FALSE);
     }
-
-    return NULL;
 }
-#if 0
-    
-    for (i = 0; i < len; ++i)
-    {
-    
-    
-    while ((eol = strchr (line, '\n')))
-    {
-#if 0
-	char *l;
 	
-	l = g_strndup (line, eol + 1 - line);
-	g_print ("line: %s", l);
-	g_free (l);
-#endif
-	if (6 == sscanf (line, "%d %d %llu # %ld %ld %ld",
-			 &(entry->pid),
-			 &(entry->cpu),
-			 &(entry->timestamp),
-			 &(entry->type),
-			 &(entry->address),
-			 &(entry->location)))
-	{
-#if 0
-#endif
-	    
-	    return eol + 1;
-	}
-#if 0
-	else
-	    g_print ("unparsable\n");
-#endif
-
-	line = eol + 1;
-    }
-
-    return NULL;
-#endif
-
 static CPUInfo *
 find_cpu_info (Collector *collector, int pid)
 {
     int i;
     
-    /* New stack trace - find a CPUInfo that is not in use */
     for (i = 0; i < N_CPU; ++i)
     {
 	if (collector->cpu_info[i].trace.pid == pid)
@@ -481,7 +414,7 @@ find_cpu_info (Collector *collector, int pid)
     return NULL;
 }
 
-static gboolean
+static void
 process_entry (Collector *collector, Entry *entry)
 {
     CPUInfo *info = NULL;
@@ -493,9 +426,9 @@ process_entry (Collector *collector, Entry *entry)
      * I am assuming here that we can't have more than one stacktrace
      * in progress per PID
      */
-    if (entry->type == 0)
+    switch (entry->type)
     {
-	/* Beginning of stack trace */
+    case 0: /* Beginning of stack trace */
 	if ((info = find_cpu_info (collector, -1)))
 	{
 	    info->trace.pid = entry->pid;
@@ -503,10 +436,9 @@ process_entry (Collector *collector, Entry *entry)
 	    info->user_pos = 0;
 	    info->kernel_pos = 0;
 	}
-    }
-    else if (entry->type == 1)
-    {
-	/* Kernel address */
+	break;
+	
+    case 1: /* Kernel address */
 	if ((info = find_cpu_info (collector, entry->pid)))
 	{
 	    if (info->kernel_pos < SYSPROF_MAX_ADDRESSES)
@@ -514,10 +446,9 @@ process_entry (Collector *collector, Entry *entry)
 	    else
 		info->trace.truncated = TRUE;
 	}
-    }
-    else if (entry->type == 2)
-    {
-	/* User address */
+	break;
+	
+    case 2: /* User address */
 	if ((info = find_cpu_info (collector, entry->pid)))
 	{
 	    if (info->user_pos < SYSPROF_MAX_ADDRESSES)
@@ -525,53 +456,25 @@ process_entry (Collector *collector, Entry *entry)
 	    else
 		info->trace.truncated = TRUE;	    
 	}
-    }
-    else if (entry->type == 3)
-    {
-	/* End of stack trace */
+	break;
+	
+    case 3: /* End of stack trace */
 	if ((info = find_cpu_info (collector, entry->pid)))
 	{
 	    info->trace.n_kernel_words = info->kernel_pos;
 	    info->trace.n_addresses = info->user_pos;
 
-#if 0
-	    if (info->kernel_pos == 1)
-		g_print ("only one kernel address: user words: %d\n", info->user_pos);
-#endif
-	    
 	    add_trace_to_stash (&(info->trace), collector->stash);
 	
 	    collector->n_samples++;
 
 	    info->trace.pid = -1;
 	}
-    }
-}
+	break;
 
-static void
-parse_input (Collector *collector)
-{
-    Entry entry;
-    const guchar *str, *s;
-    int len;
-    /* Four states */
-    gboolean ignoring;
-    SysprofStackTrace trace;
-    int pos;
-    gboolean update = FALSE;
-    
-    str = collector->input->str;
-    len = collector->input->len;
-
-    while ((s = parse_line (str, len, &entry)))
-    {
-	process_entry (collector, &entry);
-
-	len -= (s - str);
-	str = s;
+    default:
+	break;
     }
-
-    g_string_erase (collector->input, 0, collector->input->len - len);
 }
 
 /* */
@@ -581,8 +484,6 @@ on_read (gpointer data)
     Collector *collector = data;
     char input [N_ENTRIES * ENTRY_SIZE];
     int n_bytes;
-    gboolean first;
-    int i;
 
     memset (input, '1', sizeof input);
 
@@ -592,48 +493,35 @@ on_read (gpointer data)
 
 	if (!in_dead_period (collector))
 	{
+	    gboolean first_sample;
+ 	    const guchar *s;
+	    int len;
+	    
 	    g_string_append_len (collector->input, input, n_bytes);
 	    
-	    first = collector->n_samples == 0;
+	    first_sample = collector->n_samples == 0;
+
+	    len = collector->input->len;
+	    s = (const guchar *)collector->input->str;
+	    while (len >= ENTRY_SIZE)
+	    {
+		Entry entry;
+
+		read_entry (s, &entry);
+		process_entry (collector, &entry);
 
-	    parse_input (collector);
+		len -= ENTRY_SIZE;
+		s += ENTRY_SIZE;
+	    }
+	    g_string_erase (collector->input, 0, collector->input->len - len);
 	    
 	    if (collector->callback && collector->n_samples > 0)
-		collector->callback (first, collector->data);
+		collector->callback (first_sample, collector->data);
 	}
     }
 
     if (n_bytes == -1 && errno != EAGAIN)
-    {
 	g_warning ("Read from trace pipe: %s\n", strerror (errno));
-    }
-
-
-
-    
-    
-#if 0
-    if (n_bytes <= 0)
-    {
-	g_print ("result: %d\n", n_bytes);
-	return;
-    }
-
-    if (in_dead_period (collector))
-	return;
-#endif
-    
-#if 0
-    /* Sometimes we get nul bytes in the input. This is inconvenient, so
-     * replace them with newlines.
-     */
-    for (i = 0; i < n_bytes; ++i)
-    {
-	if (input[i] == 0)
-	    input[i] = '\n';
-    }
-#endif
-    
 }
 
 static gboolean