Commit 922d0e4d9f04 ("perf tools: Adjust symbols in VDSO") changed the
ELF symbol parsing so that the vDSO is treated the same as ET_EXEC and
ET_REL binaries despite being an ET_DYN. This was a partial workaround
to deal with older x86 vDSOs being prelinked at a high address that
didn't correspond to the map, so using object-relative offsets and
adding the base of the map allowed symbol resolution to succeed.
Unfortunately, this causes objdump not to produce any output in
conjunction with perf annotate, which cheerfully passes the absolute
address of the map symbol.
This patch fixes the problem by avoiding adjustment of vDSO symbols and
instead setting the map->pgoff field to correspond to the virtual load
address specified in the vDSO ELF header.
Cc: Vladimir Nikulichev <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Reported-by: Kristina Martsenko <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
v1->v2: Adjust map->pgoff in ELF loader to avoid breaking symbol lookup
on older kernels.
tools/perf/util/map.c | 5 ++---
tools/perf/util/symbol-elf.c | 9 ++++++++-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index a14f08f41686..6ba38293be88 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -173,10 +173,9 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
filename = newfilename;
}
- if (vdso) {
- pgoff = 0;
+ if (vdso)
dso = vdso__dso_findnew(machine, thread);
- } else
+ else
dso = __dsos__findnew(&machine->user_dsos, filename);
if (dso == NULL)
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index a7ab6063e038..83f8ba232575 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -706,7 +706,6 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
GElf_Shdr shdr;
ss->adjust_symbols = (ehdr.e_type == ET_EXEC ||
ehdr.e_type == ET_REL ||
- dso__is_vdso(dso) ||
elf_section_by_name(elf, &ehdr, &shdr,
".gnu.prelink_undo",
NULL) != NULL);
@@ -824,6 +823,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
sec = syms_ss->symtab;
shdr = syms_ss->symshdr;
+ /*
+ * Older x86 kernels prelink the vDSO at a high address, so
+ * we need to reflect that in map->pgoff in order to talk to
+ * objdump.
+ */
+ if (dso__is_vdso(dso))
+ map->pgoff = shdr.sh_addr - shdr.sh_offset;
+
if (runtime_ss->opdsec)
opddata = elf_rawdata(runtime_ss->opdsec, NULL);
--
2.1.4
On 27/06/15 12:10, Will Deacon wrote:
> Commit 922d0e4d9f04 ("perf tools: Adjust symbols in VDSO") changed the
> ELF symbol parsing so that the vDSO is treated the same as ET_EXEC and
> ET_REL binaries despite being an ET_DYN. This was a partial workaround
> to deal with older x86 vDSOs being prelinked at a high address that
> didn't correspond to the map, so using object-relative offsets and
> adding the base of the map allowed symbol resolution to succeed.
>
> Unfortunately, this causes objdump not to produce any output in
> conjunction with perf annotate, which cheerfully passes the absolute
> address of the map symbol.
>
> This patch fixes the problem by avoiding adjustment of vDSO symbols and
> instead setting the map->pgoff field to correspond to the virtual load
> address specified in the vDSO ELF header.
>
> Cc: Vladimir Nikulichev <[email protected]>
> Cc: Adrian Hunter <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Reported-by: Kristina Martsenko <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
> ---
>
> v1->v2: Adjust map->pgoff in ELF loader to avoid breaking symbol lookup
> on older kernels.
>
> tools/perf/util/map.c | 5 ++---
> tools/perf/util/symbol-elf.c | 9 ++++++++-
> 2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index a14f08f41686..6ba38293be88 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -173,10 +173,9 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
> filename = newfilename;
> }
>
> - if (vdso) {
> - pgoff = 0;
> + if (vdso)
> dso = vdso__dso_findnew(machine, thread);
> - } else
> + else
> dso = __dsos__findnew(&machine->user_dsos, filename);
>
> if (dso == NULL)
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index a7ab6063e038..83f8ba232575 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -706,7 +706,6 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
> GElf_Shdr shdr;
> ss->adjust_symbols = (ehdr.e_type == ET_EXEC ||
> ehdr.e_type == ET_REL ||
> - dso__is_vdso(dso) ||
> elf_section_by_name(elf, &ehdr, &shdr,
> ".gnu.prelink_undo",
> NULL) != NULL);
> @@ -824,6 +823,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
> sec = syms_ss->symtab;
> shdr = syms_ss->symshdr;
>
> + /*
> + * Older x86 kernels prelink the vDSO at a high address, so
> + * we need to reflect that in map->pgoff in order to talk to
> + * objdump.
> + */
> + if (dso__is_vdso(dso))
> + map->pgoff = shdr.sh_addr - shdr.sh_offset;
In the case of perf tools, maps map memory addresses to file offsets.
That is used to read from the object file, so you can't change the map.
On 29/06/15 12:02, Adrian Hunter wrote:
> On 27/06/15 12:10, Will Deacon wrote:
>> Commit 922d0e4d9f04 ("perf tools: Adjust symbols in VDSO") changed the
>> ELF symbol parsing so that the vDSO is treated the same as ET_EXEC and
>> ET_REL binaries despite being an ET_DYN. This was a partial workaround
>> to deal with older x86 vDSOs being prelinked at a high address that
>> didn't correspond to the map, so using object-relative offsets and
>> adding the base of the map allowed symbol resolution to succeed.
>>
>> Unfortunately, this causes objdump not to produce any output in
>> conjunction with perf annotate, which cheerfully passes the absolute
>> address of the map symbol.
>>
>> This patch fixes the problem by avoiding adjustment of vDSO symbols and
>> instead setting the map->pgoff field to correspond to the virtual load
>> address specified in the vDSO ELF header.
>>
>> Cc: Vladimir Nikulichev <[email protected]>
>> Cc: Adrian Hunter <[email protected]>
>> Cc: Namhyung Kim <[email protected]>
>> Cc: Andy Lutomirski <[email protected]>
>> Reported-by: Kristina Martsenko <[email protected]>
>> Signed-off-by: Will Deacon <[email protected]>
>> ---
>>
>> v1->v2: Adjust map->pgoff in ELF loader to avoid breaking symbol lookup
>> on older kernels.
>>
>> tools/perf/util/map.c | 5 ++---
>> tools/perf/util/symbol-elf.c | 9 ++++++++-
>> 2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
>> index a14f08f41686..6ba38293be88 100644
>> --- a/tools/perf/util/map.c
>> +++ b/tools/perf/util/map.c
>> @@ -173,10 +173,9 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
>> filename = newfilename;
>> }
>>
>> - if (vdso) {
>> - pgoff = 0;
>> + if (vdso)
>> dso = vdso__dso_findnew(machine, thread);
>> - } else
>> + else
>> dso = __dsos__findnew(&machine->user_dsos, filename);
>>
>> if (dso == NULL)
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index a7ab6063e038..83f8ba232575 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -706,7 +706,6 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
>> GElf_Shdr shdr;
>> ss->adjust_symbols = (ehdr.e_type == ET_EXEC ||
>> ehdr.e_type == ET_REL ||
>> - dso__is_vdso(dso) ||
>> elf_section_by_name(elf, &ehdr, &shdr,
>> ".gnu.prelink_undo",
>> NULL) != NULL);
>> @@ -824,6 +823,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
>> sec = syms_ss->symtab;
>> shdr = syms_ss->symshdr;
>>
>> + /*
>> + * Older x86 kernels prelink the vDSO at a high address, so
>> + * we need to reflect that in map->pgoff in order to talk to
>> + * objdump.
>> + */
>> + if (dso__is_vdso(dso))
>> + map->pgoff = shdr.sh_addr - shdr.sh_offset;
>
> In the case of perf tools, maps map memory addresses to file offsets.
> That is used to read from the object file, so you can't change the map.
So what about just this instead:
if (dso__is_vdso(dso))
map->reloc = shdr.sh_addr - shdr.sh_offset;
Although shdr is ".dynsym" whereas ".text" is what we typically care about.
Don't know if they could be inconsistent.
On 29/06/15 15:30, Adrian Hunter wrote:
> On 29/06/15 12:02, Adrian Hunter wrote:
>> On 27/06/15 12:10, Will Deacon wrote:
>>> Commit 922d0e4d9f04 ("perf tools: Adjust symbols in VDSO") changed the
>>> ELF symbol parsing so that the vDSO is treated the same as ET_EXEC and
>>> ET_REL binaries despite being an ET_DYN. This was a partial workaround
>>> to deal with older x86 vDSOs being prelinked at a high address that
>>> didn't correspond to the map, so using object-relative offsets and
>>> adding the base of the map allowed symbol resolution to succeed.
>>>
>>> Unfortunately, this causes objdump not to produce any output in
>>> conjunction with perf annotate, which cheerfully passes the absolute
>>> address of the map symbol.
>>>
>>> This patch fixes the problem by avoiding adjustment of vDSO symbols and
>>> instead setting the map->pgoff field to correspond to the virtual load
>>> address specified in the vDSO ELF header.
>>>
>>> Cc: Vladimir Nikulichev <[email protected]>
>>> Cc: Adrian Hunter <[email protected]>
>>> Cc: Namhyung Kim <[email protected]>
>>> Cc: Andy Lutomirski <[email protected]>
>>> Reported-by: Kristina Martsenko <[email protected]>
>>> Signed-off-by: Will Deacon <[email protected]>
>>> ---
>>>
>>> v1->v2: Adjust map->pgoff in ELF loader to avoid breaking symbol lookup
>>> on older kernels.
>>>
>>> tools/perf/util/map.c | 5 ++---
>>> tools/perf/util/symbol-elf.c | 9 ++++++++-
>>> 2 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
>>> index a14f08f41686..6ba38293be88 100644
>>> --- a/tools/perf/util/map.c
>>> +++ b/tools/perf/util/map.c
>>> @@ -173,10 +173,9 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
>>> filename = newfilename;
>>> }
>>>
>>> - if (vdso) {
>>> - pgoff = 0;
>>> + if (vdso)
>>> dso = vdso__dso_findnew(machine, thread);
>>> - } else
>>> + else
>>> dso = __dsos__findnew(&machine->user_dsos, filename);
>>>
>>> if (dso == NULL)
>>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>>> index a7ab6063e038..83f8ba232575 100644
>>> --- a/tools/perf/util/symbol-elf.c
>>> +++ b/tools/perf/util/symbol-elf.c
>>> @@ -706,7 +706,6 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
>>> GElf_Shdr shdr;
>>> ss->adjust_symbols = (ehdr.e_type == ET_EXEC ||
>>> ehdr.e_type == ET_REL ||
>>> - dso__is_vdso(dso) ||
>>> elf_section_by_name(elf, &ehdr, &shdr,
>>> ".gnu.prelink_undo",
>>> NULL) != NULL);
>>> @@ -824,6 +823,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
>>> sec = syms_ss->symtab;
>>> shdr = syms_ss->symshdr;
>>>
>>> + /*
>>> + * Older x86 kernels prelink the vDSO at a high address, so
>>> + * we need to reflect that in map->pgoff in order to talk to
>>> + * objdump.
>>> + */
>>> + if (dso__is_vdso(dso))
>>> + map->pgoff = shdr.sh_addr - shdr.sh_offset;
>>
>> In the case of perf tools, maps map memory addresses to file offsets.
>> That is used to read from the object file, so you can't change the map.
>
> So what about just this instead:
>
> if (dso__is_vdso(dso))
> map->reloc = shdr.sh_addr - shdr.sh_offset;
>
No that's no good either :-(
On Mon, Jun 29, 2015 at 02:23:51PM +0100, Adrian Hunter wrote:
> On 29/06/15 15:30, Adrian Hunter wrote:
> > On 29/06/15 12:02, Adrian Hunter wrote:
> >> On 27/06/15 12:10, Will Deacon wrote:
> >>> @@ -824,6 +823,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
> >>> sec = syms_ss->symtab;
> >>> shdr = syms_ss->symshdr;
> >>>
> >>> + /*
> >>> + * Older x86 kernels prelink the vDSO at a high address, so
> >>> + * we need to reflect that in map->pgoff in order to talk to
> >>> + * objdump.
> >>> + */
> >>> + if (dso__is_vdso(dso))
> >>> + map->pgoff = shdr.sh_addr - shdr.sh_offset;
> >>
> >> In the case of perf tools, maps map memory addresses to file offsets.
> >> That is used to read from the object file, so you can't change the map.
> >
> > So what about just this instead:
> >
> > if (dso__is_vdso(dso))
> > map->reloc = shdr.sh_addr - shdr.sh_offset;
> >
>
> No that's no good either :-(
Yeah... we're fighting against symbol lookup wanting ->map_ip to give the
address of the ELF symbol but unwinding wanting ->map_ip to give a relative
offset for file reads.
Also, the reloc is a bit weird and I think needs to be the other way around
(i.e. we convert from a map-relative address to an objdump address by
*subtracting* the reloc). Even with that change, I run into problems with
annotate comparing ELF symbol addresses with map addresses. Urgh.
Will
Em Mon, Jun 29, 2015 at 02:52:39PM +0100, Will Deacon escreveu:
> On Mon, Jun 29, 2015 at 02:23:51PM +0100, Adrian Hunter wrote:
> > On 29/06/15 15:30, Adrian Hunter wrote:
> > > On 29/06/15 12:02, Adrian Hunter wrote:
> > >> On 27/06/15 12:10, Will Deacon wrote:
> > >>> @@ -824,6 +823,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
> > >>> sec = syms_ss->symtab;
> > >>> shdr = syms_ss->symshdr;
> > >>>
> > >>> + /*
> > >>> + * Older x86 kernels prelink the vDSO at a high address, so
> > >>> + * we need to reflect that in map->pgoff in order to talk to
> > >>> + * objdump.
> > >>> + */
> > >>> + if (dso__is_vdso(dso))
> > >>> + map->pgoff = shdr.sh_addr - shdr.sh_offset;
> > >>
> > >> In the case of perf tools, maps map memory addresses to file offsets.
> > >> That is used to read from the object file, so you can't change the map.
> > >
> > > So what about just this instead:
> > >
> > > if (dso__is_vdso(dso))
> > > map->reloc = shdr.sh_addr - shdr.sh_offset;
> > >
> >
> > No that's no good either :-(
>
> Yeah... we're fighting against symbol lookup wanting ->map_ip to give the
> address of the ELF symbol but unwinding wanting ->map_ip to give a relative
> offset for file reads.
>
> Also, the reloc is a bit weird and I think needs to be the other way around
> (i.e. we convert from a map-relative address to an objdump address by
> *subtracting* the reloc). Even with that change, I run into problems with
> annotate comparing ELF symbol addresses with map addresses. Urgh.
Its been a long time that I looked at this, but perhaps these can help:
/* rip/ip <-> addr suitable for passing to `objdump --start-address=` */
u64 map__rip_2objdump(struct map *map, u64 rip);
/* objdump address -> memory address */
u64 map__objdump_2mem(struct map *map, u64 ip);
The comments in the changeset that introduced it have more info:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7a2b6209863626cf8362e5ff4653491558f91e67
- Arnaldo
- Arnaldo
On 29/06/15 16:52, Will Deacon wrote:
> On Mon, Jun 29, 2015 at 02:23:51PM +0100, Adrian Hunter wrote:
>> On 29/06/15 15:30, Adrian Hunter wrote:
>>> On 29/06/15 12:02, Adrian Hunter wrote:
>>>> On 27/06/15 12:10, Will Deacon wrote:
>>>>> @@ -824,6 +823,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
>>>>> sec = syms_ss->symtab;
>>>>> shdr = syms_ss->symshdr;
>>>>>
>>>>> + /*
>>>>> + * Older x86 kernels prelink the vDSO at a high address, so
>>>>> + * we need to reflect that in map->pgoff in order to talk to
>>>>> + * objdump.
>>>>> + */
>>>>> + if (dso__is_vdso(dso))
>>>>> + map->pgoff = shdr.sh_addr - shdr.sh_offset;
>>>>
>>>> In the case of perf tools, maps map memory addresses to file offsets.
>>>> That is used to read from the object file, so you can't change the map.
>>>
>>> So what about just this instead:
>>>
>>> if (dso__is_vdso(dso))
>>> map->reloc = shdr.sh_addr - shdr.sh_offset;
>>>
>>
>> No that's no good either :-(
>
> Yeah... we're fighting against symbol lookup wanting ->map_ip to give the
> address of the ELF symbol but unwinding wanting ->map_ip to give a relative
> offset for file reads.
>
> Also, the reloc is a bit weird and I think needs to be the other way around
> (i.e. we convert from a map-relative address to an objdump address by
> *subtracting* the reloc). Even with that change, I run into problems with
> annotate comparing ELF symbol addresses with map addresses. Urgh.
Adding or subtracting shouldn't matter.
This seems to work (without any other changes):
if (dso__is_vdso(dso))
map->reloc = map->start - shdr.sh_addr + shdr.sh_offset;
Haven't tested 32-bit yet though.
On Tue, Jun 30, 2015 at 10:57:04AM +0100, Adrian Hunter wrote:
> On 29/06/15 16:52, Will Deacon wrote:
> > On Mon, Jun 29, 2015 at 02:23:51PM +0100, Adrian Hunter wrote:
> >> On 29/06/15 15:30, Adrian Hunter wrote:
> >>> On 29/06/15 12:02, Adrian Hunter wrote:
> >>>> On 27/06/15 12:10, Will Deacon wrote:
> >>>>> @@ -824,6 +823,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
> >>>>> sec = syms_ss->symtab;
> >>>>> shdr = syms_ss->symshdr;
> >>>>>
> >>>>> + /*
> >>>>> + * Older x86 kernels prelink the vDSO at a high address, so
> >>>>> + * we need to reflect that in map->pgoff in order to talk to
> >>>>> + * objdump.
> >>>>> + */
> >>>>> + if (dso__is_vdso(dso))
> >>>>> + map->pgoff = shdr.sh_addr - shdr.sh_offset;
> >>>>
> >>>> In the case of perf tools, maps map memory addresses to file offsets.
> >>>> That is used to read from the object file, so you can't change the map.
> >>>
> >>> So what about just this instead:
> >>>
> >>> if (dso__is_vdso(dso))
> >>> map->reloc = shdr.sh_addr - shdr.sh_offset;
> >>>
> >>
> >> No that's no good either :-(
> >
> > Yeah... we're fighting against symbol lookup wanting ->map_ip to give the
> > address of the ELF symbol but unwinding wanting ->map_ip to give a relative
> > offset for file reads.
> >
> > Also, the reloc is a bit weird and I think needs to be the other way around
> > (i.e. we convert from a map-relative address to an objdump address by
> > *subtracting* the reloc). Even with that change, I run into problems with
> > annotate comparing ELF symbol addresses with map addresses. Urgh.
>
> Adding or subtracting shouldn't matter.
I was referring to the subtraction in map__rip_2objdump, but you've
addressed that below.
> This seems to work (without any other changes):
>
> if (dso__is_vdso(dso))
> map->reloc = map->start - shdr.sh_addr + shdr.sh_offset;
>
> Haven't tested 32-bit yet though.
FWIW, this works on arm64 and x86_64 with my testing:
Tested-by: Will Deacon <[email protected]>
Cheers,
Will
Hi Adrian,
On Tue, Jun 30, 2015 at 10:57:04AM +0100, Adrian Hunter wrote:
> On 29/06/15 16:52, Will Deacon wrote:
> > On Mon, Jun 29, 2015 at 02:23:51PM +0100, Adrian Hunter wrote:
> >> On 29/06/15 15:30, Adrian Hunter wrote:
> >>> On 29/06/15 12:02, Adrian Hunter wrote:
> >>>> On 27/06/15 12:10, Will Deacon wrote:
> >>>>> @@ -824,6 +823,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
> >>>>> sec = syms_ss->symtab;
> >>>>> shdr = syms_ss->symshdr;
> >>>>>
> >>>>> + /*
> >>>>> + * Older x86 kernels prelink the vDSO at a high address, so
> >>>>> + * we need to reflect that in map->pgoff in order to talk to
> >>>>> + * objdump.
> >>>>> + */
> >>>>> + if (dso__is_vdso(dso))
> >>>>> + map->pgoff = shdr.sh_addr - shdr.sh_offset;
> >>>>
> >>>> In the case of perf tools, maps map memory addresses to file offsets.
> >>>> That is used to read from the object file, so you can't change the map.
> >>>
> >>> So what about just this instead:
> >>>
> >>> if (dso__is_vdso(dso))
> >>> map->reloc = shdr.sh_addr - shdr.sh_offset;
> >>>
> >>
> >> No that's no good either :-(
> >
> > Yeah... we're fighting against symbol lookup wanting ->map_ip to give the
> > address of the ELF symbol but unwinding wanting ->map_ip to give a relative
> > offset for file reads.
> >
> > Also, the reloc is a bit weird and I think needs to be the other way around
> > (i.e. we convert from a map-relative address to an objdump address by
> > *subtracting* the reloc). Even with that change, I run into problems with
> > annotate comparing ELF symbol addresses with map addresses. Urgh.
>
> Adding or subtracting shouldn't matter.
>
> This seems to work (without any other changes):
>
> if (dso__is_vdso(dso))
> map->reloc = map->start - shdr.sh_addr + shdr.sh_offset;
>
> Haven't tested 32-bit yet though.
Any chance of writing up a patch for this? I don't see it in -next.
Will