Re: [PATCH] Symbolize stripped binaries without buildid

[Arnaldo Carvalho de Melo <acme@ghostprotocols.net>]

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

Em Fri, Jan 28, 2011 at 02:11:33PM -0800, Arun Sharma escreveu:
> Symbolize binaries which don't have buildids and are stripped, but have a .dynsym.
> 
> Signed-off-by: Arun Sharma <asharma@fb.com>

Sorry for taking soooo long to look at this, as I mentioned some time
ago I wasn't on the CC list, so it fell thru the cracks.

Srikar just mentioned that to me and by coincidence I was working on
fixing bugs on cross analysis, collecting perf.data files on a ppc64
system to run report on my workstation, a x86_64 machine.o

But if we do as in your patch we will miss looking for the .dynsym on
the build id cache (DSO__ORIG_BUILD_ID_CACHE):

[acme@emilia ~]$ cat want_symtab.c 
#include <stdio.h>

int main(void)
{
	int origin, want_symtab;

	for (origin = 0, want_symtab = 1; origin < 5; ++origin) {
		printf("origin=%d, want_symtab=%d\n", origin,
want_symtab);
		if (origin == 4 && want_symtab) {
			want_symtab = 0;
			origin = 0;
			continue;
		}
	}
}
[acme@emilia ~]$ ./want_symtab 
origin=0, want_symtab=1
origin=1, want_symtab=1
origin=2, want_symtab=1
origin=3, want_symtab=1
origin=4, want_symtab=1
origin=1, want_symtab=0
origin=2, want_symtab=0
origin=3, want_symtab=0
origin=4, want_symtab=0
[acme@emilia ~]$

So I'm applying another patch I came up with that I'm attaching to this
message.

Please let me know if you guys are ok with it and if I can add a
Tested-by: you.

- Arnaldo

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 15ccfba..a84db85 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1480,7 +1480,7 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
>  	 * Failing that, do a second pass where we accept .dynsym also
>  	 */
>  	for (self->origin = DSO__ORIG_BUILD_ID_CACHE, want_symtab = 1;
> -	     self->origin != DSO__ORIG_NOT_FOUND;
> +	     self->origin != DSO__ORIG_END;
>  	     self->origin++) {
>  		switch (self->origin) {
>  		case DSO__ORIG_BUILD_ID_CACHE:
> @@ -1530,6 +1530,7 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
>  				 self->long_name);
>  			break;
>  
> +		case DSO__ORIG_NOT_FOUND:
>  		default:
>  			/*
>  			 * If we wanted a full symtab but no image had one,
> @@ -1538,8 +1539,7 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
>  			if (want_symtab) {
>  				want_symtab = 0;
>  				self->origin = DSO__ORIG_BUILD_ID_CACHE;
> -			} else
> -				continue;
> +			}
>  		}
>  
>  		/* Name is now the name of the next image to try */
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 670cd1c..6b79ab9 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -201,6 +201,7 @@ enum dso_origin {
>  	DSO__ORIG_GUEST_KMODULE,
>  	DSO__ORIG_KMODULE,
>  	DSO__ORIG_NOT_FOUND,
> +	DSO__ORIG_END,
>  };

[-- Attachment #2: want_dynsym.patch --]
[-- Type: text/plain, Size: 2529 bytes --]

commit e988a8fbd20e5562399f802c69276aa42938e229
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Tue Mar 22 15:42:14 2011 -0300

    perf symbols: Look at .dymsym again if .symtab not found
    
    The original intent of the code was to repeat the search with
    want_symtab = 0. But as the code stands now, we never hit the "default"
    case of the switch statement. Which means we never repeat the search.
    
    Reported-by: Arun Sharma <asharma@fb.com>
    Reported-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
    Cc: Dave Martin <dave.martin@linaro.org>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: Ingo Molnar <mingo@elte.hu>
    Cc: Mike Galbraith <efault@gmx.de>
    Cc: Paul Mackerras <paulus@samba.org>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Stephane Eranian <eranian@google.com>
    Cc: Tom Zanussi <tzanussi@gmail.com>
    LKML-Reference: <new-submission>
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 651dbfe..17df793 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1486,7 +1486,9 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
 	 * On the first pass, only load images if they have a full symtab.
 	 * Failing that, do a second pass where we accept .dynsym also
 	 */
-	for (self->symtab_type = SYMTAB__BUILD_ID_CACHE, want_symtab = 1;
+	want_symtab = 1;
+restart:
+	for (self->symtab_type = SYMTAB__BUILD_ID_CACHE;
 	     self->symtab_type != SYMTAB__NOT_FOUND;
 	     self->symtab_type++) {
 		switch (self->symtab_type) {
@@ -1536,17 +1538,7 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
 			snprintf(name, size, "%s%s", symbol_conf.symfs,
 				 self->long_name);
 			break;
-
-		default:
-			/*
-			 * If we wanted a full symtab but no image had one,
-			 * relax our requirements and repeat the search.
-			 */
-			if (want_symtab) {
-				want_symtab = 0;
-				self->symtab_type = SYMTAB__BUILD_ID_CACHE;
-			} else
-				continue;
+		default:;
 		}
 
 		/* Name is now the name of the next image to try */
@@ -1573,6 +1565,15 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
 		}
 	}
 
+	/*
+	 * If we wanted a full symtab but no image had one,
+	 * relax our requirements and repeat the search.
+	 */
+	if (ret <= 0 && want_symtab) {
+		want_symtab = 0;
+		goto restart;
+	}
+
 	free(name);
 	if (ret < 0 && strstr(self->name, " (deleted)") != NULL)
 		return 0;