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

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

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;

Although shdr is ".dynsym" whereas ".text" is what we typically care about.
Don't know if they could be inconsistent.