[PATCH] xenctx: misc adjustments

[PATCH] xenctx: misc adjustments

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

- fix off-by-one errors during symbol insertion and lookup
- don't store the symbol type, as it wasn't needed at all so far and
  is only needed now at parsing time
- don't insert certain kinds of symbols

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -19,6 +19,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <signal.h>
+#include <ctype.h>
 #include <string.h>
 #include <inttypes.h>
 #include <getopt.h>
@@ -58,7 +59,6 @@ int disp_tlb;
 
 struct symbol {
     guest_word_t address;
-    char type;
     char *name;
     struct symbol *next;
 } *symbol_table = NULL;
@@ -112,12 +112,12 @@ static void insert_symbol(struct symbol 
 
     /* The System.map is usually already sorted... */
     if (prev
-        && prev->address < symbol->address
+        && prev->address <= symbol->address
         && (!prev->next || prev->next->address > symbol->address)) {
         s = prev;
     } else {
         /* ... otherwise do crappy/slow search for the correct place */
-        while(s && s->next && s->next->address < symbol->address)
+        while (s->next && s->next->address <= symbol->address)
             s = s->next;
     }
 
@@ -130,13 +130,13 @@ static struct symbol *lookup_symbol(gues
 {
     struct symbol *s = symbol_table;
 
-    while(s && s->next && s->next->address < address)
-        s = s->next;
+    if (!s)
+        return NULL;
 
-    if (s && s->address < address)
-        return s;
+    while (s->next && s->next->address < address)
+        s = s->next;
 
-    return NULL;
+    return s->next && s->next->address <= address ? s->next : s;
 }
 
 static void print_symbol(guest_word_t addr)
@@ -159,7 +159,7 @@ static void print_symbol(guest_word_t ad
 
 static void read_symbol_table(const char *symtab)
 {
-    char line[256];
+    char type, line[256];
     char *p;
     struct symbol *symbol;
     FILE *f;
@@ -178,9 +178,13 @@ static void read_symbol_table(const char
 
         /* need more checks for syntax here... */
         symbol->address = strtoull(line, &p, 16);
-        p++;
-        symbol->type = *p++;
-        p++;
+        if (!isspace(*p++))
+            continue;
+        type = *p++;
+        if (!isalpha(type) && type != '?')
+            continue;
+        if (!isspace(*p++))
+            continue;
 
         /* in the future we should handle the module name
          * being appended here, this would allow us to use
@@ -190,7 +194,18 @@ static void read_symbol_table(const char
             p[strlen(p)-1] = '\0';
         symbol->name = strdup(p);
 
-        insert_symbol(symbol);
+        switch (type) {
+        case 'A': /* global absolute */
+        case 'a': /* local absolute */
+            break;
+        case 'U': /* undefined */
+        case 'v': /* undefined weak object */
+        case 'w': /* undefined weak function */
+            continue;
+        default:
+            insert_symbol(symbol);
+            break;
+        }
 
         if (strcmp(symbol->name, "_stext") == 0)
             kernel_stext = symbol->address;




[-- Attachment #2: xenctx-misc.patch --]
[-- Type: text/plain, Size: 3185 bytes --]

- fix off-by-one errors during symbol insertion and lookup
- don't store the symbol type, as it wasn't needed at all so far and
  is only needed now at parsing time
- don't insert certain kinds of symbols

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -19,6 +19,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <signal.h>
+#include <ctype.h>
 #include <string.h>
 #include <inttypes.h>
 #include <getopt.h>
@@ -58,7 +59,6 @@ int disp_tlb;
 
 struct symbol {
     guest_word_t address;
-    char type;
     char *name;
     struct symbol *next;
 } *symbol_table = NULL;
@@ -112,12 +112,12 @@ static void insert_symbol(struct symbol 
 
     /* The System.map is usually already sorted... */
     if (prev
-        && prev->address < symbol->address
+        && prev->address <= symbol->address
         && (!prev->next || prev->next->address > symbol->address)) {
         s = prev;
     } else {
         /* ... otherwise do crappy/slow search for the correct place */
-        while(s && s->next && s->next->address < symbol->address)
+        while (s->next && s->next->address <= symbol->address)
             s = s->next;
     }
 
@@ -130,13 +130,13 @@ static struct symbol *lookup_symbol(gues
 {
     struct symbol *s = symbol_table;
 
-    while(s && s->next && s->next->address < address)
-        s = s->next;
+    if (!s)
+        return NULL;
 
-    if (s && s->address < address)
-        return s;
+    while (s->next && s->next->address < address)
+        s = s->next;
 
-    return NULL;
+    return s->next && s->next->address <= address ? s->next : s;
 }
 
 static void print_symbol(guest_word_t addr)
@@ -159,7 +159,7 @@ static void print_symbol(guest_word_t ad
 
 static void read_symbol_table(const char *symtab)
 {
-    char line[256];
+    char type, line[256];
     char *p;
     struct symbol *symbol;
     FILE *f;
@@ -178,9 +178,13 @@ static void read_symbol_table(const char
 
         /* need more checks for syntax here... */
         symbol->address = strtoull(line, &p, 16);
-        p++;
-        symbol->type = *p++;
-        p++;
+        if (!isspace(*p++))
+            continue;
+        type = *p++;
+        if (!isalpha(type) && type != '?')
+            continue;
+        if (!isspace(*p++))
+            continue;
 
         /* in the future we should handle the module name
          * being appended here, this would allow us to use
@@ -190,7 +194,18 @@ static void read_symbol_table(const char
             p[strlen(p)-1] = '\0';
         symbol->name = strdup(p);
 
-        insert_symbol(symbol);
+        switch (type) {
+        case 'A': /* global absolute */
+        case 'a': /* local absolute */
+            break;
+        case 'U': /* undefined */
+        case 'v': /* undefined weak object */
+        case 'w': /* undefined weak function */
+            continue;
+        default:
+            insert_symbol(symbol);
+            break;
+        }
 
         if (strcmp(symbol->name, "_stext") == 0)
             kernel_stext = symbol->address;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] xenctx: misc adjustments

[Ian Jackson]

Jan Beulich writes ("[Xen-devel] [PATCH] xenctx: misc adjustments"):
> - fix off-by-one errors during symbol insertion and lookup
> - don't store the symbol type, as it wasn't needed at all so far and
>   is only needed now at parsing time
> - don't insert certain kinds of symbols
...
> -        insert_symbol(symbol);
> +        switch (type) {
> +        case 'A': /* global absolute */
> +        case 'a': /* local absolute */
> +            break;

Is there some reason why these shouldn't included ?  Perhaps I'm
misunderstanding what their purpose is.

Ian.

Re: [PATCH] xenctx: misc adjustments

[Jan Beulich]

>>> On 11.01.11 at 20:27, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("[Xen-devel] [PATCH] xenctx: misc adjustments"):
>> - fix off-by-one errors during symbol insertion and lookup
>> - don't store the symbol type, as it wasn't needed at all so far and
>>   is only needed now at parsing time
>> - don't insert certain kinds of symbols
> ...
>> -        insert_symbol(symbol);
>> +        switch (type) {
>> +        case 'A': /* global absolute */
>> +        case 'a': /* local absolute */
>> +            break;
> 
> Is there some reason why these shouldn't included ?  Perhaps I'm
> misunderstanding what their purpose is.

Absolute symbols (with very few exceptions) don't represent
addresses (take the symbol CRC values in older Linux as an
example; current Linux has vDSO relative(!) addresses among
them).

Since some versions have absolute _text etc, they're being
allowed to fall into the code path following the switch
statement (other than undefined symbols), but for the purpose
of annotating stack traces they're useless (and in all reality, for
relocatable kernels, there ought to be a mechanism to add a
fixed offset to all addresses, which immediately disqualifies
all absolute ones except those few special purpose ones).

Jan

Re: [PATCH] xenctx: misc adjustments

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH] xenctx: misc adjustments"):
> On 11.01.11 at 20:27, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > Is there some reason why these shouldn't included ?  Perhaps I'm
> > misunderstanding what their purpose is.
> 
> Absolute symbols (with very few exceptions) don't represent
> addresses (take the symbol CRC values in older Linux as an
> example; current Linux has vDSO relative(!) addresses among
> them).

Thanks for the explanation.

> Since some versions have absolute _text etc, they're being
> allowed to fall into the code path following the switch
> statement (other than undefined symbols), but for the purpose
> of annotating stack traces they're useless (and in all reality, for
> relocatable kernels, there ought to be a mechanism to add a
> fixed offset to all addresses, which immediately disqualifies
> all absolute ones except those few special purpose ones).

Right.  Well I think this is a bugfix so can go in during the feature
freeze.  But ATM we're hoping for a test pass so we can 4.1.0 rc1 so
I'm going to hold off.

Thanks,
Ian.