* [RFC PATCH V2]s390/perf:fix 'start' address of module's map
@ 2016-07-15 10:15 Song Shan Gong
2016-07-15 10:15 ` [RFC PATCH V2] s390/perf: fix " Song Shan Gong
0 siblings, 1 reply; 6+ messages in thread
From: Song Shan Gong @ 2016-07-15 10:15 UTC (permalink / raw)
To: acme, jolsa; +Cc: dsahern, linux-kernel, borntraeger, Song Shan Gong
Change log:
>From V1:
1.change func name from 'fix__arch_module_baseaddr' to
'fix__arch_module_text_start';
2.Parse '.text' start addr from 'sys' through 'sysfs__read_ull', not
'hex2u64()';
2.Perfect code: check return value and allocated pointer by 'strdup'.
Song Shan Gong (1):
s390/perf: fix 'start' address of module's map
tools/perf/arch/s390/util/Build | 2 ++
tools/perf/arch/s390/util/sym-handling.c | 27 +++++++++++++++++++++++++++
tools/perf/util/machine.c | 8 ++++++++
tools/perf/util/machine.h | 1 +
4 files changed, 38 insertions(+)
create mode 100644 tools/perf/arch/s390/util/sym-handling.c
--
2.3.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH V2] s390/perf: fix 'start' address of module's map
2016-07-15 10:15 [RFC PATCH V2]s390/perf:fix 'start' address of module's map Song Shan Gong
@ 2016-07-15 10:15 ` Song Shan Gong
2016-07-18 1:52 ` Songshan Gong
2016-07-18 14:07 ` Jiri Olsa
0 siblings, 2 replies; 6+ messages in thread
From: Song Shan Gong @ 2016-07-15 10:15 UTC (permalink / raw)
To: acme, jolsa; +Cc: dsahern, linux-kernel, borntraeger, 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 | 27 +++++++++++++++++++++++++++
tools/perf/util/machine.c | 8 ++++++++
tools/perf/util/machine.h | 1 +
4 files changed, 38 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..ff51336
--- /dev/null
+++ b/tools/perf/arch/s390/util/sym-handling.c
@@ -0,0 +1,27 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include "util.h"
+#include "machine.h"
+#include "api/fs/fs.h"
+
+int arch__fix_module_text_start(u64 *start, const char *name)
+{
+ char path[PATH_MAX];
+ char *module_name = NULL;
+ int len;
+
+ if (!(module_name = strdup(name)))
+ return -1;
+
+ len = strlen(module_name);
+ module_name[len - 1] = '\0';
+ snprintf(path, PATH_MAX, "module/%s/sections/.text",
+ module_name + 1);
+
+ if (sysfs__read_ull(path, (unsigned long long *)start) < 0) {
+ free(module_name);
+ return -1;
+ }
+ return 0;
+}
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index b177218..97cc9f0 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1091,12 +1091,20 @@ 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_text_start(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;
+ if (arch__fix_module_text_start(&start, name) < 0)
+ return -1;
+
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..20739f7 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -216,6 +216,7 @@ 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_text_start(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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH V2] s390/perf: fix 'start' address of module's map
2016-07-15 10:15 ` [RFC PATCH V2] s390/perf: fix " Song Shan Gong
@ 2016-07-18 1:52 ` Songshan Gong
2016-07-18 14:07 ` Jiri Olsa
1 sibling, 0 replies; 6+ messages in thread
From: Songshan Gong @ 2016-07-18 1:52 UTC (permalink / raw)
To: acme, jolsa; +Cc: dsahern, linux-kernel, borntraeger
在 7/15/2016 6:15 PM, 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 | 27 +++++++++++++++++++++++++++
> tools/perf/util/machine.c | 8 ++++++++
> tools/perf/util/machine.h | 1 +
> 4 files changed, 38 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..ff51336
> --- /dev/null
> +++ b/tools/perf/arch/s390/util/sym-handling.c
> @@ -0,0 +1,27 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include "util.h"
> +#include "machine.h"
> +#include "api/fs/fs.h"
> +
> +int arch__fix_module_text_start(u64 *start, const char *name)
> +{
> + char path[PATH_MAX];
> + char *module_name = NULL;
> + int len;
> +
> + if (!(module_name = strdup(name)))
> + return -1;
'checkpatch.pl' don't allow to use assignment in if condition.I'll split
this sentence then.
Sorry for careless. And others is OK for 'checkpatch.pl'.
> +
> + len = strlen(module_name);
> + module_name[len - 1] = '\0';
> + snprintf(path, PATH_MAX, "module/%s/sections/.text",
> + module_name + 1);
> +
> + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) {
> + free(module_name);
> + return -1;
> + }
> + return 0;
> +}
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index b177218..97cc9f0 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1091,12 +1091,20 @@ 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_text_start(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;
>
> + if (arch__fix_module_text_start(&start, name) < 0)
> + return -1;
> +
> 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..20739f7 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -216,6 +216,7 @@ 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_text_start(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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH V2] s390/perf: fix 'start' address of module's map
2016-07-15 10:15 ` [RFC PATCH V2] s390/perf: fix " Song Shan Gong
2016-07-18 1:52 ` Songshan Gong
@ 2016-07-18 14:07 ` Jiri Olsa
[not found] ` <d32bfb65-a998-ebec-6dbf-348c4dd32149@linux.vnet.ibm.com>
1 sibling, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2016-07-18 14:07 UTC (permalink / raw)
To: Song Shan Gong; +Cc: acme, jolsa, dsahern, linux-kernel, borntraeger
On Fri, Jul 15, 2016 at 06:15:11PM +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'.
>
> 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 | 27 +++++++++++++++++++++++++++
> tools/perf/util/machine.c | 8 ++++++++
> tools/perf/util/machine.h | 1 +
> 4 files changed, 38 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..ff51336
> --- /dev/null
> +++ b/tools/perf/arch/s390/util/sym-handling.c
> @@ -0,0 +1,27 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include "util.h"
> +#include "machine.h"
> +#include "api/fs/fs.h"
> +
> +int arch__fix_module_text_start(u64 *start, const char *name)
> +{
> + char path[PATH_MAX];
> + char *module_name = NULL;
> + int len;
> +
> + if (!(module_name = strdup(name)))
> + return -1;
> +
> + len = strlen(module_name);
> + module_name[len - 1] = '\0';
> + snprintf(path, PATH_MAX, "module/%s/sections/.text",
> + module_name + 1);
why can't you use 'name' in here? I can't see the reason
you allocated module_name..
> +
> + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) {
> + free(module_name);
> + return -1;
> + }
leaking module_name in here
> + return 0;
> +}
SNIP
thanks,
jirka
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH V2] s390/perf: fix 'start' address of module's map
[not found] ` <d32bfb65-a998-ebec-6dbf-348c4dd32149@linux.vnet.ibm.com>
@ 2016-07-19 1:50 ` Songshan Gong
2016-07-19 3:35 ` Songshan Gong
0 siblings, 1 reply; 6+ messages in thread
From: Songshan Gong @ 2016-07-19 1:50 UTC (permalink / raw)
To: Jiri Olsa; +Cc: acme, jolsa, dsahern, linux-kernel, borntraeger
在 7/19/2016 9:37 AM, Songshan Gong 写道:
>
>
> 在 7/18/2016 10:07 PM, Jiri Olsa 写道:
>> On Fri, Jul 15, 2016 at 06:15:11PM +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'.
>>>
>>> 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 | 27
>>> +++++++++++++++++++++++++++
>>> tools/perf/util/machine.c | 8 ++++++++
>>> tools/perf/util/machine.h | 1 +
>>> 4 files changed, 38 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..ff51336
>>> --- /dev/null
>>> +++ b/tools/perf/arch/s390/util/sym-handling.c
>>> @@ -0,0 +1,27 @@
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <unistd.h>
>>> +#include "util.h"
>>> +#include "machine.h"
>>> +#include "api/fs/fs.h"
>>> +
>>> +int arch__fix_module_text_start(u64 *start, const char *name)
>>> +{
>>> + char path[PATH_MAX];
>>> + char *module_name = NULL;
>>> + int len;
>>> +
>>> + if (!(module_name = strdup(name)))
>>> + return -1;
>>> +
>>> + len = strlen(module_name);
>>> + module_name[len - 1] = '\0';
>>> + snprintf(path, PATH_MAX, "module/%s/sections/.text",
>>> + module_name + 1);
>>
>> why can't you use 'name' in here? I can't see the reason
>> you allocated module_name..
>
> Oh, yes.
>
Sorry, just ignore the upper reply.
I use the 'module_name' here instead of 'name', because 'name' is
something like '[module_name]', I need strip the '[' and ']', so I have
to set the tail ']' to '\0' to indicate the string end, which will need
change the constant variable 'name'.
>
>>> +
>>> + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) {
>>> + free(module_name);
>>> + return -1;
>>> + }
>>
>> leaking module_name in here
>
> yes, thanks.
>>
>>> + return 0;
>>> +}
>>
>> SNIP
>>
>> thanks,
>> jirka
>>
>
--
SongShan Gong
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH V2] s390/perf: fix 'start' address of module's map
2016-07-19 1:50 ` Songshan Gong
@ 2016-07-19 3:35 ` Songshan Gong
0 siblings, 0 replies; 6+ messages in thread
From: Songshan Gong @ 2016-07-19 3:35 UTC (permalink / raw)
To: Jiri Olsa; +Cc: acme, jolsa, dsahern, linux-kernel, borntraeger
在 7/19/2016 9:50 AM, Songshan Gong 写道:
>
>
> 在 7/19/2016 9:37 AM, Songshan Gong 写道:
>>
>>
>> 在 7/18/2016 10:07 PM, Jiri Olsa 写道:
>>> On Fri, Jul 15, 2016 at 06:15:11PM +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'.
>>>>
>>>> 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 | 27
>>>> +++++++++++++++++++++++++++
>>>> tools/perf/util/machine.c | 8 ++++++++
>>>> tools/perf/util/machine.h | 1 +
>>>> 4 files changed, 38 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..ff51336
>>>> --- /dev/null
>>>> +++ b/tools/perf/arch/s390/util/sym-handling.c
>>>> @@ -0,0 +1,27 @@
>>>> +#include <stdio.h>
>>>> +#include <stdlib.h>
>>>> +#include <unistd.h>
>>>> +#include "util.h"
>>>> +#include "machine.h"
>>>> +#include "api/fs/fs.h"
>>>> +
>>>> +int arch__fix_module_text_start(u64 *start, const char *name)
>>>> +{
>>>> + char path[PATH_MAX];
>>>> + char *module_name = NULL;
>>>> + int len;
>>>> +
>>>> + if (!(module_name = strdup(name)))
>>>> + return -1;
>>>> +
>>>> + len = strlen(module_name);
>>>> + module_name[len - 1] = '\0';
>>>> + snprintf(path, PATH_MAX, "module/%s/sections/.text",
>>>> + module_name + 1);
>>>
>>> why can't you use 'name' in here? I can't see the reason
>>> you allocated module_name..
>>
>> Oh, yes.
>>
> Sorry, just ignore the upper reply.
>
> I use the 'module_name' here instead of 'name', because 'name' is
> something like '[module_name]', I need strip the '[' and ']', so I have
> to set the tail ']' to '\0' to indicate the string end, which will need
> change the constant variable 'name'.
>
eeee, I found a new way to implement it.
I could use '%.*s' to format the 'path', 'name' is enough indeed.
It's so concise now. Appreciate.
>
>>
>>>> +
>>>> + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) {
>>>> + free(module_name);
>>>> + return -1;
>>>> + }
>>>
>>> leaking module_name in here
>>
>> yes, thanks.
>>>
>>>> + return 0;
>>>> +}
>>>
>>> SNIP
>>>
>>> thanks,
>>> jirka
>>>
>>
>
--
SongShan Gong
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-19 3:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-15 10:15 [RFC PATCH V2]s390/perf:fix 'start' address of module's map Song Shan Gong
2016-07-15 10:15 ` [RFC PATCH V2] s390/perf: fix " Song Shan Gong
2016-07-18 1:52 ` Songshan Gong
2016-07-18 14:07 ` Jiri Olsa
[not found] ` <d32bfb65-a998-ebec-6dbf-348c4dd32149@linux.vnet.ibm.com>
2016-07-19 1:50 ` Songshan Gong
2016-07-19 3:35 ` Songshan Gong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).