Re: [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN

* Re: [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN
       [not found]   ` <CAPp4yYzRwoTBzH1_TwcdvAOryZppV9-BwnX2bOtN1qrEqrdBww@mail.gmail.com>
@ 2014-09-11 20:38     ` Arnaldo Carvalho de Melo
  2014-09-15 18:06       ` Tong Shen
  0 siblings, 1 reply; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2014-09-11 20:38 UTC (permalink / raw)
  To: Tong Shen; +Cc: linux-perf-users, Linux Kernel Mailing List

Em Thu, Sep 11, 2014 at 11:21:40AM -0700, Tong Shen escreveu:
> Hi Arnaldo,
> 
> Thanks a lot for the comment!
> 
> I fixed the coding style and added a helper function with comments, as
> you suggested :-)
> See attachment.

That is much better, I'm adding lkml to the CC list so that we can give
other people the opportunity to chime in.

- Arnaldo

> On Thu, Sep 11, 2014 at 8:01 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> > Em Fri, Sep 05, 2014 at 11:38:02AM -0700, Tong Shen escreveu:
> >> Hi linux-perf-users,
> >>
> >> It's actually a patch, and I didn't find a development mailing list
> >> for perf, so I just post it here. If this is not the right place to
> >
> > You can post it here, but please add the maintainers to the CC list, so
> > that we can more quickly see your message.
> >
> > Comments below:
> >
> >> post patches, could someone direct me to it? Thanks in advance!
> >>
> >> In tools/perf/util/symbol-elf.c, we don't adjust symbol address for
> >> ET_DYN type of ELF files because we think ET_DYN is always
> >> relocatable.
> >>
> >> But that's not necessarily true; there are some non-relocatable ET_DYN
> >> ELF files. For those files, we should still adjust symbol address.
> >
> >> Suggested patch: (HTML subpart now allowed in this mailing list...)
> >
> >> +++ b/perf-3.12.0/tools/perf/util/symbol-elf.c
> >> @@ -915,7 +915,10 @@ int dso__load_sym(struct dso *dso, struct map *map,
> >
> >>               if ((used_opd && runtime_ss->adjust_symbols)
> >> -                             || (!used_opd && syms_ss->adjust_symbols)) {
> >> +                             || (!used_opd && syms_ss->adjust_symbols)
> >> +                             || (syms_ss->ehdr.e_type == et_dyn
> >> +                                     && shdr.sh_addr != shdr.sh_offset
> >> +                                     && elf_sec__is_text(&shdr, secstrs))) {
> >
> > Please try to follow kernel coding style in these tools, i.e. the &&
> > should be at the end, the expression on each line should be aligned just
> > after the opening parens.
> >
> > If, like in this case, you find something not following that style, take
> > the opportunity to fix it :-)
> >
> > But back to your change, I'll have to look at this code further, just
> > looking at the excerpt above I find it way too confusing, and your patch
> > makes it even more so...
> >
> > Can you introduce some suitably named helper that has those extra
> > conditions you're adding now and then add it to this conditional?
> >
> > I.e. make it somethine like:
> >
> > -               if ((used_opd && runtime_ss->adjust_symbols)
> > -                               || (!used_opd && syms_ss->adjust_symbols)) {
> > +               if ((used_opd && runtime_ss->adjust_symbols) ||
> > +                   (!used_opd && syms_ss->adjust_symbols)   ||
> > +                   syms__some_suitable_name(syms_ss, &shdr)) {
> >
> > And then add your explanation of these kinds of files as a comment over
> > this new function, please?
> >
> >>                       pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " "
> >>                                 "sh_addr: %#" PRIx64 " sh_offset: %#" PRIx64 "\n", __func__,
> >>                                 (u64)sym.st_value, (u64)shdr.sh_addr,
> >
> > Best regards,
> >
> > - Arnaldo
> 
> 
> 
> -- 
> Best Regards, Tong Shen

> diff --git a/perf-3.12.0/tools/perf/util/symbol-elf.c b/perf-3.12.0/tools/perf/util/symbol-elf.c
> index a9c829b..f9bd488 100644
> --- a/perf-3.12.0/tools/perf/util/symbol-elf.c
> +++ b/perf-3.12.0/tools/perf/util/symbol-elf.c
> @@ -673,6 +673,27 @@ static u64 ref_reloc(struct kmap *kmap)
>  	return 0;
>  }
>  
> +/**
> + * is_non_relocatable_dyn - check if ELF file is ET_DYN, but not relocatable
> + * @ehdr: ELF header of the ELF file
> + * @shdr: a section header in the ELF file
> + *
> + * For ELF files with type ET_EXEC and ET_REL, we adjust symbol addresses in
> + * symbol table in dso__load_sym() so that later we can always locate symbols
> + * by sym.st_value + map_address_of_ELF_file.
> + *
> + * But ELF files with type ET_DYN may need adjusting as well, if they are
> + * not relocatable. There's no standard way to tell if an ELF file with type
> + * ET_DYN is relocatable. One possible way to do that is checking if
> + * sh_addr == sh_offset for .text section.
> + */
> +static bool is_non_relocatable_dyn(GElf_Ehdr *ehdr, GElf_Shdr *shdr,
> +                                   Elf_Data *secstrs) {
> +	return ehdr->e_type == ET_DYN &&
> +	       elf_sec__is_text(shdr, secstrs) &&
> +	       shdr->sh_offset != shdr->sh_addr;
> +}
> +
>  int dso__load_sym(struct dso *dso, struct map *map,
>  		  struct symsrc *syms_ss, struct symsrc *runtime_ss,
>  		  symbol_filter_t filter, int kmodule)
> @@ -914,8 +935,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
>  			goto new_symbol;
>  		}
>  
> -		if ((used_opd && runtime_ss->adjust_symbols)
> -				|| (!used_opd && syms_ss->adjust_symbols)) {
> +		if ((used_opd && runtime_ss->adjust_symbols) ||
> +		    (!used_opd && syms_ss->adjust_symbols) ||
> +		    is_non_relocatable_dyn(&ehdr, &shdr, secstrs)) {
>  			pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " "
>  				  "sh_addr: %#" PRIx64 " sh_offset: %#" PRIx64 "\n", __func__,
>  				  (u64)sym.st_value, (u64)shdr.sh_addr,

^ permalink raw reply	[flat|nested] 7+ messages in thread