* 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
* Re: [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN 2014-09-11 20:38 ` [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN Arnaldo Carvalho de Melo @ 2014-09-15 18:06 ` Tong Shen 2014-09-25 0:28 ` Tong Shen 0 siblings, 1 reply; 7+ messages in thread From: Tong Shen @ 2014-09-15 18:06 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: linux-perf-users, Linux Kernel Mailing List Gentle ping :-) On Thu, Sep 11, 2014 at 1:38 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > 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, > -- Best Regards, Tong Shen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN 2014-09-15 18:06 ` Tong Shen @ 2014-09-25 0:28 ` Tong Shen 2014-09-26 5:45 ` Namhyung Kim 0 siblings, 1 reply; 7+ messages in thread From: Tong Shen @ 2014-09-25 0:28 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: linux-perf-users, Linux Kernel Mailing List Gentle Ping :-) On Mon, Sep 15, 2014 at 11:06 AM, Tong Shen <endlessroad@google.com> wrote: > Gentle ping :-) > > On Thu, Sep 11, 2014 at 1:38 PM, Arnaldo Carvalho de Melo > <acme@kernel.org> wrote: >> 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, >> > > > > -- > Best Regards, Tong Shen -- Best Regards, Tong Shen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN 2014-09-25 0:28 ` Tong Shen @ 2014-09-26 5:45 ` Namhyung Kim 2014-09-26 5:53 ` Tong Shen 0 siblings, 1 reply; 7+ messages in thread From: Namhyung Kim @ 2014-09-26 5:45 UTC (permalink / raw) To: Tong Shen Cc: Arnaldo Carvalho de Melo, linux-perf-users, Linux Kernel Mailing List Hi Tong, Does a non-relocatable-dyn mean a prelink-ed dso? Thanks, Namhyung On Wed, 24 Sep 2014 17:28:24 -0700, Tong Shen wrote: > Gentle Ping :-) > > On Mon, Sep 15, 2014 at 11:06 AM, Tong Shen <endlessroad@google.com> wrote: >> Gentle ping :-) >> >> On Thu, Sep 11, 2014 at 1:38 PM, Arnaldo Carvalho de Melo >> <acme@kernel.org> wrote: >>> 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, >>> >> >> >> >> -- >> Best Regards, Tong Shen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN 2014-09-26 5:45 ` Namhyung Kim @ 2014-09-26 5:53 ` Tong Shen 2014-09-29 2:33 ` Namhyung Kim 0 siblings, 1 reply; 7+ messages in thread From: Tong Shen @ 2014-09-26 5:53 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, linux-perf-users, Linux Kernel Mailing List In my case it's not a prelink-ed dso; but I believe the effect is the same. Specifically: - Program headers and section headers contain absolute address; - Symbol addresses are also absolute; - There's no relocation in my case. On Thu, Sep 25, 2014 at 10:45 PM, Namhyung Kim <namhyung@kernel.org> wrote: > Hi Tong, > > Does a non-relocatable-dyn mean a prelink-ed dso? > > Thanks, > Namhyung > > > On Wed, 24 Sep 2014 17:28:24 -0700, Tong Shen wrote: >> Gentle Ping :-) >> >> On Mon, Sep 15, 2014 at 11:06 AM, Tong Shen <endlessroad@google.com> wrote: >>> Gentle ping :-) >>> >>> On Thu, Sep 11, 2014 at 1:38 PM, Arnaldo Carvalho de Melo >>> <acme@kernel.org> wrote: >>>> 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, >>>> >>> >>> >>> >>> -- >>> Best Regards, Tong Shen -- Best Regards, Tong Shen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN 2014-09-26 5:53 ` Tong Shen @ 2014-09-29 2:33 ` Namhyung Kim 2014-09-29 18:13 ` Tong Shen 0 siblings, 1 reply; 7+ messages in thread From: Namhyung Kim @ 2014-09-29 2:33 UTC (permalink / raw) To: Tong Shen Cc: Arnaldo Carvalho de Melo, linux-perf-users, Linux Kernel Mailing List Hi Tong, On Thu, 25 Sep 2014 22:53:22 -0700, Tong Shen wrote: >>>>>> +/** >>>>>> + * 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, Hmm.. IIUC for normal dso (ET_DYN), shdr->offset == shdr->sh_addr for text section right? And we always adjust ET_EXEC and ET_REL.. What about always trying to adjust symbol address then? We may precalculate adjust offset and subtracting it from symbol values. And the offset of 0 effectively means no adjust. This way we can simplify the logic IMHO. Thanks, Namhyung ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN 2014-09-29 2:33 ` Namhyung Kim @ 2014-09-29 18:13 ` Tong Shen 0 siblings, 0 replies; 7+ messages in thread From: Tong Shen @ 2014-09-29 18:13 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, linux-perf-users, Linux Kernel Mailing List On Sun, Sep 28, 2014 at 7:33 PM, Namhyung Kim <namhyung@kernel.org> wrote: > > Hmm.. IIUC for normal dso (ET_DYN), shdr->offset == shdr->sh_addr for > text section right? And we always adjust ET_EXEC and ET_REL.. What > about always trying to adjust symbol address then? We may precalculate > adjust offset and subtracting it from symbol values. And the offset of > 0 effectively means no adjust. This way we can simplify the logic IMHO. Yeah I'd like to do that too; but (shdr->offset == shdr->sh_addr for normal ET_DYN && shdr->offset != shdr->sh_addr for non-relocatable ET_DYN) is only true for .text section. It does not hold for .data section. I didn't dig deep into the code; if we only care about symbols in .text section (function names), we can do what you said. Otherwise we may want to keep it the way it is... -- Best Regards, Tong Shen ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-29 18:13 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CAPp4yYwTHbXiypxR_1WJGS9DYQNJ24t_Ykdo_sfQ48Ak-K3Z7g@mail.gmail.com>
[not found] ` <20140911150123.GH10158@kernel.org>
[not found] ` <CAPp4yYzRwoTBzH1_TwcdvAOryZppV9-BwnX2bOtN1qrEqrdBww@mail.gmail.com>
2014-09-11 20:38 ` [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN Arnaldo Carvalho de Melo
2014-09-15 18:06 ` Tong Shen
2014-09-25 0:28 ` Tong Shen
2014-09-26 5:45 ` Namhyung Kim
2014-09-26 5:53 ` Tong Shen
2014-09-29 2:33 ` Namhyung Kim
2014-09-29 18:13 ` Tong Shen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).