[PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Song Shan Gong]

At preset, when creating module's map, perf gets 'start' address by parsing
'proc/modules', but it's module base address, isn't the start address of
'.text' section. In most archs, it's OK. But for s390, it places 'GOT' and
'PLT' relocations before '.text' section. So there exists an offset between
module base address and '.text' section, which will incur wrong symbol
resolution for modules.

Fix this bug by getting 'start' address of module's map from parsing
'/sys/module/[module name]/sections/.text', not from '/proc/modules'.

Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com>
---
 tools/perf/arch/s390/util/Build          |  2 ++
 tools/perf/arch/s390/util/sym-handling.c | 49 ++++++++++++++++++++++++++++++++
 tools/perf/util/machine.c                |  6 ++++
 tools/perf/util/machine.h                |  2 ++
 4 files changed, 59 insertions(+)
 create mode 100644 tools/perf/arch/s390/util/sym-handling.c

diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build
index 8a61372..5e322ed 100644
--- a/tools/perf/arch/s390/util/Build
+++ b/tools/perf/arch/s390/util/Build
@@ -2,3 +2,5 @@ libperf-y += header.o
 libperf-y += kvm-stat.o
 
 libperf-$(CONFIG_DWARF) += dwarf-regs.o
+
+libperf-y += sym-handling.o
diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c
new file mode 100644
index 0000000..efe2a50
--- /dev/null
+++ b/tools/perf/arch/s390/util/sym-handling.c
@@ -0,0 +1,49 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include "symbol.h"
+#include "map.h"
+#include "util.h"
+#include "machine.h"
+
+int arch__fix_module_baseaddr(struct machine *machine,
+		u64 *start, const char *name)
+{
+	char path[PATH_MAX];
+	char *module_name = strdup(name);
+	int len = strlen(module_name);
+	FILE *file;
+	int err = 0;
+	u64 text_start;
+	char *line = NULL;
+	size_t n;
+	char *sep;
+
+	module_name[len - 1] = '\0';
+	module_name += 1;
+	snprintf(path, PATH_MAX, "%s/sys/module/%s/sections/.text",
+				machine->root_dir, module_name);
+	file = fopen(path, "r");
+	if (file == NULL)
+		return -1;
+
+	len = getline(&line, &n, file);
+	if (len < 0) {
+		err = -1;
+		goto out;
+	}
+	line[--len] = '\0'; /* \n */
+	sep = strrchr(line, 'x');
+	if (sep == NULL) {
+		err = -1;
+		goto out;
+	}
+	hex2u64(sep + 1, &text_start);
+
+	*start = text_start;
+out:
+	free(line);
+	fclose(file);
+	free(module_name - 1);
+	return err;
+}
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index b177218..e5c2721 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1091,12 +1091,18 @@ static int machine__set_modules_path(struct machine *machine)
 
 	return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
 }
+int __weak arch__fix_module_baseaddr(struct machine *machine __maybe_unused,
+				u64 *start __maybe_unused, const char *name __maybe_unused)
+{
+	return 0;
+}
 
 static int machine__create_module(void *arg, const char *name, u64 start)
 {
 	struct machine *machine = arg;
 	struct map *map;
 
+	arch__fix_module_baseaddr(machine, &start, name);
 	map = machine__findnew_module_map(machine, start, name);
 	if (map == NULL)
 		return -1;
diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index 41ac9cf..da7b6c0 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -216,6 +216,8 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine,
 
 struct map *machine__findnew_module_map(struct machine *machine, u64 start,
 					const char *filename);
+int arch__fix_module_baseaddr(struct machine *machine, u64 *start,
+					const char *name);
 
 int __machine__load_kallsyms(struct machine *machine, const char *filename,
 			     enum map_type type, bool no_kcore, symbol_filter_t filter);
-- 
2.3.0

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Songshan Gong]

在 7/7/2016 9:49 AM, Song Shan Gong 写道:
> At preset, when creating module's map, perf gets 'start' address by parsing
> 'proc/modules', but it's module base address, isn't the start address of
> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and
> 'PLT' relocations before '.text' section. So there exists an offset between
> module base address and '.text' section, which will incur wrong symbol
> resolution for modules.
>
> Fix this bug by getting 'start' address of module's map from parsing
> '/sys/module/[module name]/sections/.text', not from '/proc/modules'.
>
> Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com>
> ---
>  tools/perf/arch/s390/util/Build          |  2 ++
>  tools/perf/arch/s390/util/sym-handling.c | 49 ++++++++++++++++++++++++++++++++
>  tools/perf/util/machine.c                |  6 ++++
>  tools/perf/util/machine.h                |  2 ++
>  4 files changed, 59 insertions(+)
>  create mode 100644 tools/perf/arch/s390/util/sym-handling.c
>
> diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build
> index 8a61372..5e322ed 100644
> --- a/tools/perf/arch/s390/util/Build
> +++ b/tools/perf/arch/s390/util/Build
> @@ -2,3 +2,5 @@ libperf-y += header.o
>  libperf-y += kvm-stat.o
>
>  libperf-$(CONFIG_DWARF) += dwarf-regs.o
> +
> +libperf-y += sym-handling.o
> diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c
> new file mode 100644
> index 0000000..efe2a50
> --- /dev/null
> +++ b/tools/perf/arch/s390/util/sym-handling.c
> @@ -0,0 +1,49 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include "symbol.h"
> +#include "map.h"
> +#include "util.h"
> +#include "machine.h"
> +
> +int arch__fix_module_baseaddr(struct machine *machine,
> +		u64 *start, const char *name)
> +{
> +	char path[PATH_MAX];
> +	char *module_name = strdup(name);
> +	int len = strlen(module_name);
> +	FILE *file;
> +	int err = 0;
> +	u64 text_start;
> +	char *line = NULL;
> +	size_t n;
> +	char *sep;
> +
> +	module_name[len - 1] = '\0';
> +	module_name += 1;
> +	snprintf(path, PATH_MAX, "%s/sys/module/%s/sections/.text",
> +				machine->root_dir, module_name);
> +	file = fopen(path, "r");
> +	if (file == NULL)
> +		return -1;
> +
> +	len = getline(&line, &n, file);
> +	if (len < 0) {
> +		err = -1;
> +		goto out;
> +	}
> +	line[--len] = '\0'; /* \n */
> +	sep = strrchr(line, 'x');
> +	if (sep == NULL) {
> +		err = -1;
> +		goto out;
> +	}
> +	hex2u64(sep + 1, &text_start);
> +
> +	*start = text_start;
> +out:
> +	free(line);
> +	fclose(file);
> +	free(module_name - 1);
> +	return err;
> +}
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index b177218..e5c2721 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1091,12 +1091,18 @@ static int machine__set_modules_path(struct machine *machine)
>
>  	return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
>  }
> +int __weak arch__fix_module_baseaddr(struct machine *machine __maybe_unused,
> +				u64 *start __maybe_unused, const char *name __maybe_unused)
> +{
> +	return 0;
> +}
>
>  static int machine__create_module(void *arg, const char *name, u64 start)
>  {
>  	struct machine *machine = arg;
>  	struct map *map;
>
> +	arch__fix_module_baseaddr(machine, &start, name);

As the description says, I would change the function name to 
'arch__fix_module_text_start';

>  	map = machine__findnew_module_map(machine, start, name);
>  	if (map == NULL)
>  		return -1;
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index 41ac9cf..da7b6c0 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -216,6 +216,8 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine,
>
>  struct map *machine__findnew_module_map(struct machine *machine, u64 start,
>  					const char *filename);
> +int arch__fix_module_baseaddr(struct machine *machine, u64 *start,
> +					const char *name);
>
>  int __machine__load_kallsyms(struct machine *machine, const char *filename,
>  			     enum map_type type, bool no_kcore, symbol_filter_t filter);
>

-- 
SongShan Gong

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Jiri Olsa]

On Fri, Jul 08, 2016 at 10:17:30AM +0800, Songshan Gong wrote:

SNIP

> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index b177218..e5c2721 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1091,12 +1091,18 @@ static int machine__set_modules_path(struct machine *machine)
> > 
> >  	return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
> >  }
> > +int __weak arch__fix_module_baseaddr(struct machine *machine __maybe_unused,
> > +				u64 *start __maybe_unused, const char *name __maybe_unused)
> > +{
> > +	return 0;
> > +}
> > 
> >  static int machine__create_module(void *arg, const char *name, u64 start)
> >  {
> >  	struct machine *machine = arg;
> >  	struct map *map;
> > 
> > +	arch__fix_module_baseaddr(machine, &start, name);
> 
> As the description says, I would change the function name to
> 'arch__fix_module_text_start';

both of them sound good to me

jirka

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Jiri Olsa]

On Thu, Jul 07, 2016 at 09:49:36AM +0800, Song Shan Gong wrote:
> At preset, when creating module's map, perf gets 'start' address by parsing
> 'proc/modules', but it's module base address, isn't the start address of
> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and
> 'PLT' relocations before '.text' section. So there exists an offset between
> module base address and '.text' section, which will incur wrong symbol
> resolution for modules.
> 
> Fix this bug by getting 'start' address of module's map from parsing
> '/sys/module/[module name]/sections/.text', not from '/proc/modules'.

cool, does this fix the 'perf test 1' for s390? that'd be great

I'll send few coments shortly

thanks,
jirka

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Songshan Gong]

在 7/8/2016 11:18 PM, Jiri Olsa 写道:
> On Thu, Jul 07, 2016 at 09:49:36AM +0800, Song Shan Gong wrote:
>> At preset, when creating module's map, perf gets 'start' address by parsing
>> 'proc/modules', but it's module base address, isn't the start address of
>> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and
>> 'PLT' relocations before '.text' section. So there exists an offset between
>> module base address and '.text' section, which will incur wrong symbol
>> resolution for modules.
>>
>> Fix this bug by getting 'start' address of module's map from parsing
>> '/sys/module/[module name]/sections/.text', not from '/proc/modules'.
>
> cool, does this fix the 'perf test 1' for s390? that'd be great

I've checked, 'perf test 1' is still failed. But the test is for testing 
whether 'vmlinux symtab matches kallsyms'. For vmlinux, it's built when 
compiling linux-kernel, so there's no any symbol info about module. For 
kallsyms, when loading, it also splits kernel symbols with module 
symbols. But this patch is intended to fix wrong symbol resolution for 
modules, so I think there's no relationship between this patch and the 
testcase 'perf test 1'.

I also debuged the reason why 'perf test 1' fails, there are two reasons 
at least:
1. perf gets kernel start address by finding the first no-zero value of 
symbols {'_text', '_stext'} in /proc/kallsyms; for s390, it's always 
'_stext', but actually, '_stext' is not the first symbol in s390, there 
are other symbols before '_stext', for example 'iplstart'.
2. In addition, when loading by parsing /proc/kallsyms, if the kernel 
map start value is a non-zero value getting from '_stext', because 
'_text' is zero, it will not be included in kernel map; but for vmlinux, 
it always add '_text' to kernel map whether '_text' is zero or not. So 
if '_text' is zero, whether adding '_text' to kernel map, it's 
non-consistent for loading by /proc/kallsyms and vmlinux.

I'll try to fix this bug later.

Thanks for your comments.

> I'll send few coments shortly
>
> thanks,
> jirka
>

-- 
SongShan Gong

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Songshan Gong]

I send this email to test the connection to linux-kernel maillist. Just 
ignore.

在 7/11/2016 4:11 PM, Songshan Gong 写道:
>
>
> 在 7/8/2016 11:18 PM, Jiri Olsa 写道:
>> On Thu, Jul 07, 2016 at 09:49:36AM +0800, Song Shan Gong wrote:
>>> At preset, when creating module's map, perf gets 'start' address by
>>> parsing
>>> 'proc/modules', but it's module base address, isn't the start address of
>>> '.text' section. In most archs, it's OK. But for s390, it places
>>> 'GOT' and
>>> 'PLT' relocations before '.text' section. So there exists an offset
>>> between
>>> module base address and '.text' section, which will incur wrong symbol
>>> resolution for modules.
>>>
>>> Fix this bug by getting 'start' address of module's map from parsing
>>> '/sys/module/[module name]/sections/.text', not from '/proc/modules'.
>>
>> cool, does this fix the 'perf test 1' for s390? that'd be great
>
> I've checked, 'perf test 1' is still failed. But the test is for testing
> whether 'vmlinux symtab matches kallsyms'. For vmlinux, it's built when
> compiling linux-kernel, so there's no any symbol info about module. For
> kallsyms, when loading, it also splits kernel symbols with module
> symbols. But this patch is intended to fix wrong symbol resolution for
> modules, so I think there's no relationship between this patch and the
> testcase 'perf test 1'.
>
> I also debuged the reason why 'perf test 1' fails, there are two reasons
> at least:
> 1. perf gets kernel start address by finding the first no-zero value of
> symbols {'_text', '_stext'} in /proc/kallsyms; for s390, it's always
> '_stext', but actually, '_stext' is not the first symbol in s390, there
> are other symbols before '_stext', for example 'iplstart'.
> 2. In addition, when loading by parsing /proc/kallsyms, if the kernel
> map start value is a non-zero value getting from '_stext', because
> '_text' is zero, it will not be included in kernel map; but for vmlinux,
> it always add '_text' to kernel map whether '_text' is zero or not. So
> if '_text' is zero, whether adding '_text' to kernel map, it's
> non-consistent for loading by /proc/kallsyms and vmlinux.
>
> I'll try to fix this bug later.
>
> Thanks for your comments.
>
>> I'll send few coments shortly
>>
>> thanks,
>> jirka
>>
>

-- 
SongShan Gong

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Jiri Olsa]

On Thu, Jul 07, 2016 at 09:49:36AM +0800, Song Shan Gong wrote:

SNIP

> +libperf-y += sym-handling.o
> diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c
> new file mode 100644
> index 0000000..efe2a50
> --- /dev/null
> +++ b/tools/perf/arch/s390/util/sym-handling.c
> @@ -0,0 +1,49 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include "symbol.h"
> +#include "map.h"
> +#include "util.h"
> +#include "machine.h"
> +
> +int arch__fix_module_baseaddr(struct machine *machine,
> +		u64 *start, const char *name)
> +{
> +	char path[PATH_MAX];
> +	char *module_name = strdup(name);

we have a rule to check on every allocated pointer,
pelase check module_name != NULL before using it

> +	int len = strlen(module_name);
> +	FILE *file;
> +	int err = 0;
> +	u64 text_start;
> +	char *line = NULL;
> +	size_t n;
> +	char *sep;
> +
> +	module_name[len - 1] = '\0';
> +	module_name += 1;

hum, why do increase the pointer?

thanks,
jirka

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Jiri Olsa]

On Thu, Jul 07, 2016 at 09:49:36AM +0800, Song Shan Gong wrote:

SNIP

> +	hex2u64(sep + 1, &text_start);
> +
> +	*start = text_start;
> +out:
> +	free(line);
> +	fclose(file);
> +	free(module_name - 1);
> +	return err;
> +}
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index b177218..e5c2721 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1091,12 +1091,18 @@ static int machine__set_modules_path(struct machine *machine)
>  
>  	return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
>  }
> +int __weak arch__fix_module_baseaddr(struct machine *machine __maybe_unused,
> +				u64 *start __maybe_unused, const char *name __maybe_unused)
> +{
> +	return 0;
> +}
>  
>  static int machine__create_module(void *arg, const char *name, u64 start)
>  {
>  	struct machine *machine = arg;
>  	struct map *map;
>  
> +	arch__fix_module_baseaddr(machine, &start, name);

you should check for its return value and fail if needed

jirka

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Jiri Olsa]

On Thu, Jul 07, 2016 at 09:49:36AM +0800, Song Shan Gong wrote:

SNIP

> +	char *line = NULL;
> +	size_t n;
> +	char *sep;
> +
> +	module_name[len - 1] = '\0';
> +	module_name += 1;
> +	snprintf(path, PATH_MAX, "%s/sys/module/%s/sections/.text",
> +				machine->root_dir, module_name);
> +	file = fopen(path, "r");
> +	if (file == NULL)
> +		return -1;
> +
> +	len = getline(&line, &n, file);
> +	if (len < 0) {
> +		err = -1;
> +		goto out;
> +	}
> +	line[--len] = '\0'; /* \n */
> +	sep = strrchr(line, 'x');
> +	if (sep == NULL) {
> +		err = -1;
> +		goto out;
> +	}
> +	hex2u64(sep + 1, &text_start);

we have following functions in tools/lib/api/fs to read
single number from file, which I assume you do above:

int sysfs__read_int(const char *entry, int *value);
int sysfs__read_ull(const char *entry, unsigned long long *value);

please check if you could use some of them,
we could add some more generic one if needed

thanks,
jirka

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Songshan Gong]

在 7/8/2016 11:21 PM, Jiri Olsa 写道:
> On Thu, Jul 07, 2016 at 09:49:36AM +0800, Song Shan Gong wrote:
>
> SNIP
>
>> +	char *line = NULL;
>> +	size_t n;
>> +	char *sep;
>> +
>> +	module_name[len - 1] = '\0';
>> +	module_name += 1;
>> +	snprintf(path, PATH_MAX, "%s/sys/module/%s/sections/.text",
>> +				machine->root_dir, module_name);
>> +	file = fopen(path, "r");
>> +	if (file == NULL)
>> +		return -1;
>> +
>> +	len = getline(&line, &n, file);
>> +	if (len < 0) {
>> +		err = -1;
>> +		goto out;
>> +	}
>> +	line[--len] = '\0'; /* \n */
>> +	sep = strrchr(line, 'x');
>> +	if (sep == NULL) {
>> +		err = -1;
>> +		goto out;
>> +	}
>> +	hex2u64(sep + 1, &text_start);
>
> we have following functions in tools/lib/api/fs to read
> single number from file, which I assume you do above:
>
> int sysfs__read_int(const char *entry, int *value);
> int sysfs__read_ull(const char *entry, unsigned long long *value);
>
> please check if you could use some of them,
> we could add some more generic one if needed

It seems infeasible.
Each value in /sys/module/[module name]/sections/.text is a string like 
"0x000003ff8130078\n".
But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is 
based on decimal.

Maybe you can introduce a new argument indicating the value is based on 
hex or decimal, or binary?

> thanks,
> jirka
>

-- 
SongShan Gong

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Jiri Olsa]

On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:

SNIP

> > 
> > we have following functions in tools/lib/api/fs to read
> > single number from file, which I assume you do above:
> > 
> > int sysfs__read_int(const char *entry, int *value);
> > int sysfs__read_ull(const char *entry, unsigned long long *value);
> > 
> > please check if you could use some of them,
> > we could add some more generic one if needed
> 
> It seems infeasible.
> Each value in /sys/module/[module name]/sections/.text is a string like
> "0x000003ff8130078\n".
> But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is based
> on decimal.
> 
> Maybe you can introduce a new argument indicating the value is based on hex
> or decimal, or binary?

yea we could specify it directly and add something like:

  int filename__read_ull(const char *filename, unsigned long long *value, int base)

plus some other higher layer helpers..

but I wonder if we could use the base 0 (like in the attached patch),
the man page says it should be able to detect the base

we'd need to check all the current usage to make sure nothing gets broken

jirka


---
diff --git a/tools/lib/api/fs/fs.c b/tools/lib/api/fs/fs.c
index 08556cf2c70d..d18ae548468a 100644
--- a/tools/lib/api/fs/fs.c
+++ b/tools/lib/api/fs/fs.c
@@ -292,7 +292,7 @@ int filename__read_ull(const char *filename, unsigned long long *value)
 		return -1;
 
 	if (read(fd, line, sizeof(line)) > 0) {
-		*value = strtoull(line, NULL, 10);
+		*value = strtoull(line, NULL, 0);
 		if (*value != ULLONG_MAX)
 			err = 0;
 	}

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Songshan Gong]

在 7/11/2016 8:01 PM, Jiri Olsa 写道:
> On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
>
> SNIP
>
>>>
>>> we have following functions in tools/lib/api/fs to read
>>> single number from file, which I assume you do above:
>>>
>>> int sysfs__read_int(const char *entry, int *value);
>>> int sysfs__read_ull(const char *entry, unsigned long long *value);
>>>
>>> please check if you could use some of them,
>>> we could add some more generic one if needed
>>
>> It seems infeasible.
>> Each value in /sys/module/[module name]/sections/.text is a string like
>> "0x000003ff8130078\n".
>> But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is based
>> on decimal.
>>
>> Maybe you can introduce a new argument indicating the value is based on hex
>> or decimal, or binary?
>
> yea we could specify it directly and add something like:
>
>   int filename__read_ull(const char *filename, unsigned long long *value, int base)
>
> plus some other higher layer helpers..
>
> but I wonder if we could use the base 0 (like in the attached patch),
> the man page says it should be able to detect the base
>
> we'd need to check all the current usage to make sure nothing gets broken
>
> jirka
>
>

Since your patch havn't pushed to devel branch, my next version patch 
will still use the origin method to parse value from /sys/.

Thanks.

> ---
> diff --git a/tools/lib/api/fs/fs.c b/tools/lib/api/fs/fs.c
> index 08556cf2c70d..d18ae548468a 100644
> --- a/tools/lib/api/fs/fs.c
> +++ b/tools/lib/api/fs/fs.c
> @@ -292,7 +292,7 @@ int filename__read_ull(const char *filename, unsigned long long *value)
>  		return -1;
>
>  	if (read(fd, line, sizeof(line)) > 0) {
> -		*value = strtoull(line, NULL, 10);
> +		*value = strtoull(line, NULL, 0);
>  		if (*value != ULLONG_MAX)
>  			err = 0;
>  	}
>

-- 
SongShan Gong

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Jiri Olsa]

On Wed, Jul 13, 2016 at 02:39:13PM +0800, Songshan Gong wrote:
> 
> 
> 在 7/11/2016 8:01 PM, Jiri Olsa 写道:
> > On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
> > 
> > SNIP
> > 
> > > > 
> > > > we have following functions in tools/lib/api/fs to read
> > > > single number from file, which I assume you do above:
> > > > 
> > > > int sysfs__read_int(const char *entry, int *value);
> > > > int sysfs__read_ull(const char *entry, unsigned long long *value);
> > > > 
> > > > please check if you could use some of them,
> > > > we could add some more generic one if needed
> > > 
> > > It seems infeasible.
> > > Each value in /sys/module/[module name]/sections/.text is a string like
> > > "0x000003ff8130078\n".
> > > But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is based
> > > on decimal.
> > > 
> > > Maybe you can introduce a new argument indicating the value is based on hex
> > > or decimal, or binary?
> > 
> > yea we could specify it directly and add something like:
> > 
> >   int filename__read_ull(const char *filename, unsigned long long *value, int base)
> > 
> > plus some other higher layer helpers..
> > 
> > but I wonder if we could use the base 0 (like in the attached patch),
> > the man page says it should be able to detect the base
> > 
> > we'd need to check all the current usage to make sure nothing gets broken
> > 
> > jirka
> > 
> > 
> 
> Since your patch havn't pushed to devel branch, my next version patch will
> still use the origin method to parse value from /sys/.

I'll make/send the change during this week,

jirka

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Songshan Gong]

在 7/13/2016 5:07 PM, Jiri Olsa 写道:
> On Wed, Jul 13, 2016 at 02:39:13PM +0800, Songshan Gong wrote:
>>
>>
>> 在 7/11/2016 8:01 PM, Jiri Olsa 写道:
>>> On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
>>>
>>> SNIP
>>>
>>>>>
>>>>> we have following functions in tools/lib/api/fs to read
>>>>> single number from file, which I assume you do above:
>>>>>
>>>>> int sysfs__read_int(const char *entry, int *value);
>>>>> int sysfs__read_ull(const char *entry, unsigned long long *value);
>>>>>
>>>>> please check if you could use some of them,
>>>>> we could add some more generic one if needed
>>>>
>>>> It seems infeasible.
>>>> Each value in /sys/module/[module name]/sections/.text is a string like
>>>> "0x000003ff8130078\n".
>>>> But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is based
>>>> on decimal.
>>>>
>>>> Maybe you can introduce a new argument indicating the value is based on hex
>>>> or decimal, or binary?
>>>
>>> yea we could specify it directly and add something like:
>>>
>>>   int filename__read_ull(const char *filename, unsigned long long *value, int base)
>>>
>>> plus some other higher layer helpers..
>>>
>>> but I wonder if we could use the base 0 (like in the attached patch),
>>> the man page says it should be able to detect the base
>>>
>>> we'd need to check all the current usage to make sure nothing gets broken
>>>
>>> jirka
>>>
>>>
>>
>> Since your patch havn't pushed to devel branch, my next version patch will
>> still use the origin method to parse value from /sys/.
>
> I'll make/send the change during this week,
>

Oh, could you remind me after you've done?
Thanks a lot.

Song Shan Gong

> jirka
>

-- 
SongShan Gong

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Jiri Olsa]

On Fri, Jul 15, 2016 at 03:45:24PM +0800, Songshan Gong wrote:
> 
> 
> 在 7/13/2016 5:07 PM, Jiri Olsa 写道:
> > On Wed, Jul 13, 2016 at 02:39:13PM +0800, Songshan Gong wrote:
> > > 
> > > 
> > > 在 7/11/2016 8:01 PM, Jiri Olsa 写道:
> > > > On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
> > > > 
> > > > SNIP
> > > > 
> > > > > > 
> > > > > > we have following functions in tools/lib/api/fs to read
> > > > > > single number from file, which I assume you do above:
> > > > > > 
> > > > > > int sysfs__read_int(const char *entry, int *value);
> > > > > > int sysfs__read_ull(const char *entry, unsigned long long *value);
> > > > > > 
> > > > > > please check if you could use some of them,
> > > > > > we could add some more generic one if needed
> > > > > 
> > > > > It seems infeasible.
> > > > > Each value in /sys/module/[module name]/sections/.text is a string like
> > > > > "0x000003ff8130078\n".
> > > > > But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is based
> > > > > on decimal.
> > > > > 
> > > > > Maybe you can introduce a new argument indicating the value is based on hex
> > > > > or decimal, or binary?
> > > > 
> > > > yea we could specify it directly and add something like:
> > > > 
> > > >   int filename__read_ull(const char *filename, unsigned long long *value, int base)
> > > > 
> > > > plus some other higher layer helpers..
> > > > 
> > > > but I wonder if we could use the base 0 (like in the attached patch),
> > > > the man page says it should be able to detect the base
> > > > 
> > > > we'd need to check all the current usage to make sure nothing gets broken
> > > > 
> > > > jirka
> > > > 
> > > > 
> > > 
> > > Since your patch havn't pushed to devel branch, my next version patch will
> > > still use the origin method to parse value from /sys/.
> > 
> > I'll make/send the change during this week,
> > 
> 
> Oh, could you remind me after you've done?
> Thanks a lot.
> 

I posted it this morning, you're CC-ed

jirka

Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

[Arnaldo Carvalho de Melo]

Em Fri, Jul 15, 2016 at 10:27:43AM +0200, Jiri Olsa escreveu:
> On Fri, Jul 15, 2016 at 03:45:24PM +0800, Songshan Gong wrote:
> > 在 7/13/2016 5:07 PM, Jiri Olsa 写道:
> > > On Wed, Jul 13, 2016 at 02:39:13PM +0800, Songshan Gong wrote:
> > > > 在 7/11/2016 8:01 PM, Jiri Olsa 写道:
> > > > > On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
> > > > Since your patch havn't pushed to devel branch, my next version patch will
> > > > still use the origin method to parse value from /sys/.
> > > 
> > > I'll make/send the change during this week,
> > > 
> > 
> > Oh, could you remind me after you've done?
> > Thanks a lot.
> > 
> 
> I posted it this morning, you're CC-ed

I have just merged it, pushing now to acme/perf/core, thanks!

- Arnaldo