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

[Adrian Hunter <adrian.hunter@intel.com>]

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 <nvs@tbricks.com>
>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>> Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
>>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>>> ---
>>>
>>> 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 :-(