[PATCH] perf tools: don't adjust symbols in vDSO

[PATCH] perf tools: don't adjust symbols in vDSO

[Will Deacon]

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 causes objdump, which expects relative addresses, not to produce
any output in conjunction with perf annotate, which cheerfully passes
absolute addresses when trying to disassemble vDSO functions.

This patch avoids marking the vDSO as requiring adjustment of symbol
addresses, allowing the relative program counter to be used instead.

Cc: Vladimir Nikulichev <nvs@tbricks.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---

Not sure why I've just started seeing this, but it appears to affect
both x86 and arm64. Also, if I revert the patch above then the issue
it supposedly fixed doesn't resurface. Maybe it was just masking another
bug that has since been addressed?

 tools/perf/util/symbol-elf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index a7ab6063e038..ba4f9bf2765d 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);
-- 
2.1.4

Re: [PATCH] perf tools: don't adjust symbols in vDSO

[Adrian Hunter]

On 24/06/15 19:17, 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 causes objdump, which expects relative addresses, not to produce
> any output in conjunction with perf annotate, which cheerfully passes
> absolute addresses when trying to disassemble vDSO functions.
> 
> This patch avoids marking the vDSO as requiring adjustment of symbol
> addresses, allowing the relative program counter to be used instead.
> 
> Cc: Vladimir Nikulichev <nvs@tbricks.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> 
> Not sure why I've just started seeing this, but it appears to affect
> both x86 and arm64. Also, if I revert the patch above then the issue
> it supposedly fixed doesn't resurface. Maybe it was just masking another
> bug that has since been addressed?

No the problem still appears on older kernels.

Probably could look at the vdso section/program headers to decide if it
needs adjustment or not.

> 
>  tools/perf/util/symbol-elf.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index a7ab6063e038..ba4f9bf2765d 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);
> 

Re: [PATCH] perf tools: don't adjust symbols in vDSO

[Will Deacon]

On Thu, Jun 25, 2015 at 02:32:13PM +0100, Adrian Hunter wrote:
> On 24/06/15 19:17, 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 causes objdump, which expects relative addresses, not to produce
> > any output in conjunction with perf annotate, which cheerfully passes
> > absolute addresses when trying to disassemble vDSO functions.
> > 
> > This patch avoids marking the vDSO as requiring adjustment of symbol
> > addresses, allowing the relative program counter to be used instead.
> > 
> > Cc: Vladimir Nikulichev <nvs@tbricks.com>
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> > 
> > Not sure why I've just started seeing this, but it appears to affect
> > both x86 and arm64. Also, if I revert the patch above then the issue
> > it supposedly fixed doesn't resurface. Maybe it was just masking another
> > bug that has since been addressed?
> 
> No the problem still appears on older kernels.

Can you be more specific, please? I tried with a 3.16 kernel (that I happen
to be running on my box) but perf doesn't even detect the vdso there,
regardless of this patch.

> Probably could look at the vdso section/program headers to decide if it
> needs adjustment or not.

Did the x86 kernel change in this regard? Why isn't the vDSO always ET_DYN?

Will

Re: [PATCH] perf tools: don't adjust symbols in vDSO

[Adrian Hunter]

On 26/06/15 15:23, Will Deacon wrote:
> On Thu, Jun 25, 2015 at 02:32:13PM +0100, Adrian Hunter wrote:
>> On 24/06/15 19:17, 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 causes objdump, which expects relative addresses, not to produce
>>> any output in conjunction with perf annotate, which cheerfully passes
>>> absolute addresses when trying to disassemble vDSO functions.
>>>
>>> This patch avoids marking the vDSO as requiring adjustment of symbol
>>> addresses, allowing the relative program counter to be used instead.
>>>
>>> Cc: Vladimir Nikulichev <nvs@tbricks.com>
>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>> Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
>>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>>> ---
>>>
>>> Not sure why I've just started seeing this, but it appears to affect
>>> both x86 and arm64. Also, if I revert the patch above then the issue
>>> it supposedly fixed doesn't resurface. Maybe it was just masking another
>>> bug that has since been addressed?
>>
>> No the problem still appears on older kernels.
> 
> Can you be more specific, please? I tried with a 3.16 kernel (that I happen

3.13 Ubuntu kernel

> to be running on my box) but perf doesn't even detect the vdso there,
> regardless of this patch.

Don't know what you mean about not detecting vdso

> 
>> Probably could look at the vdso section/program headers to decide if it
>> needs adjustment or not.
> 
> Did the x86 kernel change in this regard? Why isn't the vDSO always ET_DYN?

I guess, but it is ET_DYN

Re: [PATCH] perf tools: don't adjust symbols in vDSO

[Will Deacon]

(CC'ing Andy, since the removal of VDSO_PRELINK is user-visible here)

Hi Adrian,

Thanks for your helpful responses.

On Fri, Jun 26, 2015 at 02:55:29PM +0100, Adrian Hunter wrote:
> On 26/06/15 15:23, Will Deacon wrote:
> > On Thu, Jun 25, 2015 at 02:32:13PM +0100, Adrian Hunter wrote:
> >> On 24/06/15 19:17, 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 causes objdump, which expects relative addresses, not to produce
> >>> any output in conjunction with perf annotate, which cheerfully passes
> >>> absolute addresses when trying to disassemble vDSO functions.
> >>>
> >>> This patch avoids marking the vDSO as requiring adjustment of symbol
> >>> addresses, allowing the relative program counter to be used instead.
> >>>
> >>> Cc: Vladimir Nikulichev <nvs@tbricks.com>
> >>> Cc: Adrian Hunter <adrian.hunter@intel.com>
> >>> Cc: Namhyung Kim <namhyung@kernel.org>
> >>> Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
> >>> Signed-off-by: Will Deacon <will.deacon@arm.com>
> >>> ---
> >>>
> >>> Not sure why I've just started seeing this, but it appears to affect
> >>> both x86 and arm64. Also, if I revert the patch above then the issue
> >>> it supposedly fixed doesn't resurface. Maybe it was just masking another
> >>> bug that has since been addressed?
> >>
> >> No the problem still appears on older kernels.
> > 
> > Can you be more specific, please? I tried with a 3.16 kernel (that I happen
> 
> 3.13 Ubuntu kernel

So I just tried that on a core2 box and it's broken even without my patch:

# ./perf record -e cycles:u  ./vdso-test
[...]
# ./perf report
[...]
# Overhead  Command    Shared Object      Symbol
# ........  .........  .................  ........................
#
    99.70%  vdso-test  [vdso]             [.] __vdso_clock_gettime
[...]
# ./perf annotate -v
[...]
annotating [0x2178d10]                         [vdso] : [0x258bbd0]           __vdso_clock_gettime
Executing: objdump  --start-address=0x00007fff5eb9a870 --stop-address=0x00007fff5eb9ab4c -l -d --no-show-raw -S -C /home/nanook/.debug/.build-id/51/54a08e7153b1deecc09259d1e970f9b67e1664
[...]
 Percent |      Source code & Disassembly of [vdso] for cycles:u
----------------------------------------------------------------
                             870: 4
                             876: 2
                             87f: 2
                             920: 2
                             93a: 2
                             967: 2
                             985: 1
                             988: 3
                             9a2: 2
                             9bb: 1
                             9c8: 6
                             9d6: 2
                             9d9: 4
                             a9d: 4
                             ab4: 2
                             ab8: 13
                             ac0: 1
                             ac7: 3763
                             ac9: 5
                          h->sum: 3821
         :
         :
[...]

So there's no disassembly output for the vdso using a perf tool build
from a vanilla 4.1 kernel.

If I run readelf on the generated ELF object, I see:

ELF Header:
  Magic:   7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00 
  Class:                             ELF64
  Data:                              2's complement, little endian
  Version:                           1 (current)
  OS/ABI:                            UNIX - System V
  ABI Version:                       0
  Type:                              DYN (Shared object file)
  Machine:                           Advanced Micro Devices X86-64
  Version:                           0x1
  Entry point address:               0xffffffffff700700
  Start of program headers:          64 (bytes into file)
  Start of section headers:          3600 (bytes into file)
  Flags:                             0x0
  Size of this header:               64 (bytes)
  Size of program headers:           56 (bytes)
  Number of program headers:         4
  Size of section headers:           64 (bytes)
  Number of section headers:         18
  Section header string table index: 17

Text section:

  [15] .text             PROGBITS         ffffffffff700700  00000700
       000000000000062d  0000000000000000  AX       0     0     16


So somehow the Image claims to be loaded at 0xffffffffff700000,
whilst perf's map is offset by 0x00007fff5eb9a000. This seems to be down
to the #define VDSO_PRELINK 0xffffffffff700000 in older kernels, but your
patch doesn't take that fixed address into account, so I don't see how
it's ever the right thing to do.

At least with my patch applied, recent kernels work. Without it, vdso
disassembly appears to be broken on all kernels for all architectures.

> > to be running on my box) but perf doesn't even detect the vdso there,
> > regardless of this patch.
> 
> Don't know what you mean about not detecting vdso

It just prints something like [perf.data.XXXX] instead of [vdso] -- probably
a separate bug.

Will

Re: [PATCH] perf tools: don't adjust symbols in vDSO

[Andy Lutomirski]

On Fri, Jun 26, 2015 at 7:58 AM, Will Deacon <will.deacon@arm.com> wrote:
> (CC'ing Andy, since the removal of VDSO_PRELINK is user-visible here)

What arch is this?  I removed VDSO_PRELINK entirely from x86 a while
back, and now x86's vdso has a base address of 0 before relocations,
and everything works just fine.

(Except one ancient glibc, which fails if the vdso is relocated at
all.  We no longer support that version of glibc unless you turn off
the vdso entirely.)

--Andy

Re: [PATCH] perf tools: don't adjust symbols in vDSO

[Will Deacon]

On Fri, Jun 26, 2015 at 04:18:59PM +0100, Andy Lutomirski wrote:
> On Fri, Jun 26, 2015 at 7:58 AM, Will Deacon <will.deacon@arm.com> wrote:
> > (CC'ing Andy, since the removal of VDSO_PRELINK is user-visible here)
> 
> What arch is this?  I removed VDSO_PRELINK entirely from x86 a while
> back, and now x86's vdso has a base address of 0 before relocations,
> and everything works just fine.

I think this is only x86, since it's the removal of VDSO_PRELINK that
has changed things.

> (Except one ancient glibc, which fails if the vdso is relocated at
> all.  We no longer support that version of glibc unless you turn off
> the vdso entirely.)

The problem is that perf expects to objdump portions of the vdso using
--start-address=foo and --stop-address=bar, but these addresses have changed
from being offset by VDSO_PRELINK to 0x0.

Thankfully, it looks like perf tool was always broken in this regard, but
I figured you might like to be aware of the issue. I guess perf just needs
to add on the load address of the vdso .text section to its relative
addresses before passing them to objdump.

Will

Re: [PATCH] perf tools: don't adjust symbols in vDSO

[Adrian Hunter]

On 26/06/2015 5:58 p.m., Will Deacon wrote:
> (CC'ing Andy, since the removal of VDSO_PRELINK is user-visible here)
> On Fri, Jun 26, 2015 at 02:55:29PM +0100, Adrian Hunter wrote:
>> On 26/06/15 15:23, Will Deacon wrote:
>>> On Thu, Jun 25, 2015 at 02:32:13PM +0100, Adrian Hunter wrote:
>>>> On 24/06/15 19:17, 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 causes objdump, which expects relative addresses, not to produce
>>>>> any output in conjunction with perf annotate, which cheerfully passes
>>>>> absolute addresses when trying to disassemble vDSO functions.
>>>>>
>>>>> This patch avoids marking the vDSO as requiring adjustment of symbol
>>>>> addresses, allowing the relative program counter to be used instead.
>>>>>
>>>>> Cc: Vladimir Nikulichev <nvs@tbricks.com>
>>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>>> Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
>>>>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>>>>> ---
>>>>>
>>>>> Not sure why I've just started seeing this, but it appears to affect
>>>>> both x86 and arm64. Also, if I revert the patch above then the issue
>>>>> it supposedly fixed doesn't resurface. Maybe it was just masking another
>>>>> bug that has since been addressed?
>>>>
>>>> No the problem still appears on older kernels.
>>>
>>> Can you be more specific, please? I tried with a 3.16 kernel (that I happen
>>
>> 3.13 Ubuntu kernel
>
> So I just tried that on a core2 box and it's broken even without my patch:
>
> # ./perf record -e cycles:u  ./vdso-test
> [...]
> # ./perf report
> [...]
> # Overhead  Command    Shared Object      Symbol
> # ........  .........  .................  ........................
> #
>      99.70%  vdso-test  [vdso]             [.] __vdso_clock_gettime

Annotation is broken but the symbol gets resolved.

Re: [PATCH] perf tools: don't adjust symbols in vDSO

[Andy Lutomirski]

On Fri, Jun 26, 2015 at 8:29 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Jun 26, 2015 at 04:18:59PM +0100, Andy Lutomirski wrote:
>> On Fri, Jun 26, 2015 at 7:58 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > (CC'ing Andy, since the removal of VDSO_PRELINK is user-visible here)
>>
>> What arch is this?  I removed VDSO_PRELINK entirely from x86 a while
>> back, and now x86's vdso has a base address of 0 before relocations,
>> and everything works just fine.
>
> I think this is only x86, since it's the removal of VDSO_PRELINK that
> has changed things.
>
>> (Except one ancient glibc, which fails if the vdso is relocated at
>> all.  We no longer support that version of glibc unless you turn off
>> the vdso entirely.)
>
> The problem is that perf expects to objdump portions of the vdso using
> --start-address=foo and --stop-address=bar, but these addresses have changed
> from being offset by VDSO_PRELINK to 0x0.
>
> Thankfully, it looks like perf tool was always broken in this regard, but
> I figured you might like to be aware of the issue. I guess perf just needs
> to add on the load address of the vdso .text section to its relative
> addresses before passing them to objdump.
>

Given that we've randomized the vdso load address on x86 for years, I
don't see how perf ever worked here.  There was a brief period during
which we actually loaded the vdso at the address VDSO_PRELINK, but
that's long gone.

Is there a patch I should be looking at?

--Andy

Re: [PATCH] perf tools: don't adjust symbols in vDSO

[Will Deacon]

On Fri, Jun 26, 2015 at 04:44:14PM +0100, Andy Lutomirski wrote:
> On Fri, Jun 26, 2015 at 8:29 AM, Will Deacon <will.deacon@arm.com> wrote:
> > On Fri, Jun 26, 2015 at 04:18:59PM +0100, Andy Lutomirski wrote:
> >> On Fri, Jun 26, 2015 at 7:58 AM, Will Deacon <will.deacon@arm.com> wrote:
> >> > (CC'ing Andy, since the removal of VDSO_PRELINK is user-visible here)
> >>
> >> What arch is this?  I removed VDSO_PRELINK entirely from x86 a while
> >> back, and now x86's vdso has a base address of 0 before relocations,
> >> and everything works just fine.
> >
> > I think this is only x86, since it's the removal of VDSO_PRELINK that
> > has changed things.
> >
> >> (Except one ancient glibc, which fails if the vdso is relocated at
> >> all.  We no longer support that version of glibc unless you turn off
> >> the vdso entirely.)
> >
> > The problem is that perf expects to objdump portions of the vdso using
> > --start-address=foo and --stop-address=bar, but these addresses have changed
> > from being offset by VDSO_PRELINK to 0x0.
> >
> > Thankfully, it looks like perf tool was always broken in this regard, but
> > I figured you might like to be aware of the issue. I guess perf just needs
> > to add on the load address of the vdso .text section to its relative
> > addresses before passing them to objdump.
> >
> 
> Given that we've randomized the vdso load address on x86 for years, I
> don't see how perf ever worked here.  There was a brief period during
> which we actually loaded the vdso at the address VDSO_PRELINK, but
> that's long gone.

The symbol resolution works by applying the vdso symbol offset to the
[vdso] vma base, but perf annotate had indeed been broken the whole time.

> Is there a patch I should be looking at?

I've cooked something, so I'll send it out now.

Will