[PATCH] perf: Get kernel start address by symbol name

[PATCH] perf: Get kernel start address by symbol name

[Simon Que]

The function machine__get_kernel_start_addr() was taking the first symbol
of kallsyms as the start address. This is incorrect in certain cases
where the first symbol is something at 0, while the actual kernel
functions begin at a later point (e.g. 0x80200000).

This patch fixes machine__get_kernel_start_addr() to search for the
symbol "_text" or "_stext", which marks the beginning of kernel mapping.
This was already being done in machine__create_kernel_maps(). Thus, this
patch is just a refactor, to move that code into
machine__get_kernel_start_addr().

Change-Id: I0c38c36f5e8b0f4fb92a6f57211fa45aabe545a6
Signed-off-by: Simon Que <sque@chromium.org>
---
 tools/perf/util/machine.c | 54 +++++++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 32 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 620a198..acda3d2 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -484,18 +484,6 @@ struct process_args {
 	u64 start;
 };
 
-static int symbol__in_kernel(void *arg, const char *name,
-			     char type __maybe_unused, u64 start)
-{
-	struct process_args *args = arg;
-
-	if (strchr(name, '['))
-		return 0;
-
-	args->start = start;
-	return 1;
-}
-
 static void machine__get_kallsyms_filename(struct machine *machine, char *buf,
 					   size_t bufsz)
 {
@@ -505,27 +493,41 @@ static void machine__get_kallsyms_filename(struct machine *machine, char *buf,
 		scnprintf(buf, bufsz, "%s/proc/kallsyms", machine->root_dir);
 }
 
-/* Figure out the start address of kernel map from /proc/kallsyms */
-static u64 machine__get_kernel_start_addr(struct machine *machine)
+const char *ref_reloc_sym_names[] = {"_text", "_stext", NULL};
+
+/* Figure out the start address of kernel map from /proc/kallsyms.
+ * Returns the name of the start symbol in *symbol_name. Pass in NULL as
+ * symbol_name if it's not that important.
+ */
+static u64 machine__get_kernel_start_addr(struct machine *machine,
+					  const char** symbol_name)
 {
 	char filename[PATH_MAX];
-	struct process_args args;
+	int i;
+	const char* name;
+	u64 addr;
 
 	machine__get_kallsyms_filename(machine, filename, PATH_MAX);
 
 	if (symbol__restricted_filename(filename, "/proc/kallsyms"))
 		return 0;
 
-	if (kallsyms__parse(filename, &args, symbol__in_kernel) <= 0)
-		return 0;
+	for (i = 0; (name = ref_reloc_sym_names[i]) != NULL; i++) {
+		addr = kallsyms__get_function_start(filename, name);
+		if (addr)
+			break;
+	}
+
+	if (symbol_name)
+		*symbol_name = name;
 
-	return args.start;
+	return addr;
 }
 
 int __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 {
 	enum map_type type;
-	u64 start = machine__get_kernel_start_addr(machine);
+	u64 start = machine__get_kernel_start_addr(machine, NULL);
 
 	for (type = 0; type < MAP__NR_TYPES; ++type) {
 		struct kmap *kmap;
@@ -832,23 +834,11 @@ static int machine__create_modules(struct machine *machine)
 	return 0;
 }
 
-const char *ref_reloc_sym_names[] = {"_text", "_stext", NULL};
-
 int machine__create_kernel_maps(struct machine *machine)
 {
 	struct dso *kernel = machine__get_kernel(machine);
-	char filename[PATH_MAX];
 	const char *name;
-	u64 addr = 0;
-	int i;
-
-	machine__get_kallsyms_filename(machine, filename, PATH_MAX);
-
-	for (i = 0; (name = ref_reloc_sym_names[i]) != NULL; i++) {
-		addr = kallsyms__get_function_start(filename, name);
-		if (addr)
-			break;
-	}
+	u64 addr = machine__get_kernel_start_addr(machine, &name);
 	if (!addr)
 		return -1;
 
-- 
1.8.3.2

Re: [PATCH] perf: Get kernel start address by symbol name

[Jiri Olsa]

On Fri, Jun 13, 2014 at 11:45:21AM -0700, Simon Que wrote:
> The function machine__get_kernel_start_addr() was taking the first symbol
> of kallsyms as the start address. This is incorrect in certain cases
> where the first symbol is something at 0, while the actual kernel
> functions begin at a later point (e.g. 0x80200000).
> 
> This patch fixes machine__get_kernel_start_addr() to search for the
> symbol "_text" or "_stext", which marks the beginning of kernel mapping.
> This was already being done in machine__create_kernel_maps(). Thus, this
> patch is just a refactor, to move that code into
> machine__get_kernel_start_addr().
> 
> Change-Id: I0c38c36f5e8b0f4fb92a6f57211fa45aabe545a6
> Signed-off-by: Simon Que <sque@chromium.org>

hi,
looks good to me, adding Adrian to the loop

thanks,
jirka

> ---
>  tools/perf/util/machine.c | 54 +++++++++++++++++++----------------------------
>  1 file changed, 22 insertions(+), 32 deletions(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 620a198..acda3d2 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -484,18 +484,6 @@ struct process_args {
>  	u64 start;
>  };
>  
> -static int symbol__in_kernel(void *arg, const char *name,
> -			     char type __maybe_unused, u64 start)
> -{
> -	struct process_args *args = arg;
> -
> -	if (strchr(name, '['))
> -		return 0;
> -
> -	args->start = start;
> -	return 1;
> -}
> -
>  static void machine__get_kallsyms_filename(struct machine *machine, char *buf,
>  					   size_t bufsz)
>  {
> @@ -505,27 +493,41 @@ static void machine__get_kallsyms_filename(struct machine *machine, char *buf,
>  		scnprintf(buf, bufsz, "%s/proc/kallsyms", machine->root_dir);
>  }
>  
> -/* Figure out the start address of kernel map from /proc/kallsyms */
> -static u64 machine__get_kernel_start_addr(struct machine *machine)
> +const char *ref_reloc_sym_names[] = {"_text", "_stext", NULL};
> +
> +/* Figure out the start address of kernel map from /proc/kallsyms.
> + * Returns the name of the start symbol in *symbol_name. Pass in NULL as
> + * symbol_name if it's not that important.
> + */
> +static u64 machine__get_kernel_start_addr(struct machine *machine,
> +					  const char** symbol_name)
>  {
>  	char filename[PATH_MAX];
> -	struct process_args args;
> +	int i;
> +	const char* name;
> +	u64 addr;
>  
>  	machine__get_kallsyms_filename(machine, filename, PATH_MAX);
>  
>  	if (symbol__restricted_filename(filename, "/proc/kallsyms"))
>  		return 0;
>  
> -	if (kallsyms__parse(filename, &args, symbol__in_kernel) <= 0)
> -		return 0;
> +	for (i = 0; (name = ref_reloc_sym_names[i]) != NULL; i++) {
> +		addr = kallsyms__get_function_start(filename, name);
> +		if (addr)
> +			break;
> +	}
> +
> +	if (symbol_name)
> +		*symbol_name = name;
>  
> -	return args.start;
> +	return addr;
>  }
>  
>  int __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
>  {
>  	enum map_type type;
> -	u64 start = machine__get_kernel_start_addr(machine);
> +	u64 start = machine__get_kernel_start_addr(machine, NULL);
>  
>  	for (type = 0; type < MAP__NR_TYPES; ++type) {
>  		struct kmap *kmap;
> @@ -832,23 +834,11 @@ static int machine__create_modules(struct machine *machine)
>  	return 0;
>  }
>  
> -const char *ref_reloc_sym_names[] = {"_text", "_stext", NULL};
> -
>  int machine__create_kernel_maps(struct machine *machine)
>  {
>  	struct dso *kernel = machine__get_kernel(machine);
> -	char filename[PATH_MAX];
>  	const char *name;
> -	u64 addr = 0;
> -	int i;
> -
> -	machine__get_kallsyms_filename(machine, filename, PATH_MAX);
> -
> -	for (i = 0; (name = ref_reloc_sym_names[i]) != NULL; i++) {
> -		addr = kallsyms__get_function_start(filename, name);
> -		if (addr)
> -			break;
> -	}
> +	u64 addr = machine__get_kernel_start_addr(machine, &name);
>  	if (!addr)
>  		return -1;
>  
> -- 
> 1.8.3.2
> 

Re: [PATCH] perf: Get kernel start address by symbol name

[Jiri Olsa]

On Mon, Jun 16, 2014 at 10:06:49AM +0200, Jiri Olsa wrote:
> On Fri, Jun 13, 2014 at 11:45:21AM -0700, Simon Que wrote:
> > The function machine__get_kernel_start_addr() was taking the first symbol
> > of kallsyms as the start address. This is incorrect in certain cases
> > where the first symbol is something at 0, while the actual kernel
> > functions begin at a later point (e.g. 0x80200000).
> > 
> > This patch fixes machine__get_kernel_start_addr() to search for the
> > symbol "_text" or "_stext", which marks the beginning of kernel mapping.
> > This was already being done in machine__create_kernel_maps(). Thus, this
> > patch is just a refactor, to move that code into
> > machine__get_kernel_start_addr().
> > 
> > Change-Id: I0c38c36f5e8b0f4fb92a6f57211fa45aabe545a6
> > Signed-off-by: Simon Que <sque@chromium.org>
> 
> hi,
> looks good to me, adding Adrian to the loop

well, apart from this compile error ;-)

[jolsa@krava perf]$ make
  BUILD:   Doing 'make -j4' parallel build
  CC       util/machine.o
  SUBDIR   /home/jolsa/kernel.org/linux-perf/tools/lib/traceevent/
util/machine.c: In function ‘machine__get_kernel_start_addr’:
util/machine.c:520:6: error: ‘addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  u64 addr;
      ^
util/machine.c: In function ‘machine__process_kernel_mmap_event’:
util/machine.c:547:42: error: ‘addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   machine->vmlinux_maps[type] = map__new2(start, kernel, type);
                                          ^
util/machine.c:520:6: note: ‘addr’ was declared here
  u64 addr;
      ^
cc1: all warnings being treated as errors
make[1]: *** [util/machine.o] Error 1
make: *** [all] Error 2


jirka

Re: [PATCH] perf: Get kernel start address by symbol name

[Adrian Hunter]

On 06/16/2014 11:09 AM, Jiri Olsa wrote:
> On Mon, Jun 16, 2014 at 10:06:49AM +0200, Jiri Olsa wrote:
>> On Fri, Jun 13, 2014 at 11:45:21AM -0700, Simon Que wrote:
>>> The function machine__get_kernel_start_addr() was taking the first symbol
>>> of kallsyms as the start address. This is incorrect in certain cases
>>> where the first symbol is something at 0, while the actual kernel
>>> functions begin at a later point (e.g. 0x80200000).
>>>
>>> This patch fixes machine__get_kernel_start_addr() to search for the
>>> symbol "_text" or "_stext", which marks the beginning of kernel mapping.
>>> This was already being done in machine__create_kernel_maps(). Thus, this
>>> patch is just a refactor, to move that code into
>>> machine__get_kernel_start_addr().
>>>
>>> Change-Id: I0c38c36f5e8b0f4fb92a6f57211fa45aabe545a6
>>> Signed-off-by: Simon Que <sque@chromium.org>
>>
>> hi,
>> looks good to me, adding Adrian to the loop

Yes, it looks fine except for the stuff below.

> 
> well, apart from this compile error ;-)
> 
> [jolsa@krava perf]$ make
>   BUILD:   Doing 'make -j4' parallel build
>   CC       util/machine.o
>   SUBDIR   /home/jolsa/kernel.org/linux-perf/tools/lib/traceevent/
> util/machine.c: In function ‘machine__get_kernel_start_addr’:
> util/machine.c:520:6: error: ‘addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   u64 addr;
>       ^
> util/machine.c: In function ‘machine__process_kernel_mmap_event’:
> util/machine.c:547:42: error: ‘addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    machine->vmlinux_maps[type] = map__new2(start, kernel, type);
>                                           ^
> util/machine.c:520:6: note: ‘addr’ was declared here
>   u64 addr;
>       ^
> cc1: all warnings being treated as errors
> make[1]: *** [util/machine.o] Error 1
> make: *** [all] Error 2
> 

There are checkpatch errors too:

ERROR: Remove Gerrit Change-Id's before submitting upstream.
#17:
Change-Id: I0c38c36f5e8b0f4fb92a6f57211fa45aabe545a6

ERROR: "foo** bar" should be "foo **bar"
#59: FILE: tools/perf/util/machine.c:515:
+                                         const char** symbol_name)

ERROR: "foo* bar" should be "foo *bar"
#64: FILE: tools/perf/util/machine.c:519:
+       const char* name;

total: 3 errors, 0 warnings, 0 checks, 90 lines checked