* Fix Symbol Address for ET_DYN @ 2014-09-05 18:38 Tong Shen 2014-09-05 18:40 ` Tong Shen 2014-09-11 15:01 ` Arnaldo Carvalho de Melo 0 siblings, 2 replies; 11+ messages in thread From: Tong Shen @ 2014-09-05 18:38 UTC (permalink / raw) To: linux-perf-users [-- Attachment #1: Type: text/plain, Size: 567 bytes --] 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 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 attached. Thanks. -- Best Regards, Tong Shen [-- Attachment #2: perf.patch --] [-- Type: text/x-patch, Size: 1328 bytes --] 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 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. Thanks. Suggested patch: (HTML subpart now allowed in this mailing list...) 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..0606c6f 100644 --- a/perf-3.12.0/tools/perf/util/symbol-elf.c +++ 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))) { 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 related [flat|nested] 11+ messages in thread
* Re: Fix Symbol Address for ET_DYN 2014-09-05 18:38 Fix Symbol Address for ET_DYN Tong Shen @ 2014-09-05 18:40 ` Tong Shen 2014-09-11 15:01 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 11+ messages in thread From: Tong Shen @ 2014-09-05 18:40 UTC (permalink / raw) To: linux-perf-users [-- Attachment #1: Type: text/plain, Size: 727 bytes --] Sorry, wrong patch... On Fri, Sep 5, 2014 at 11:38 AM, Tong Shen <endlessroad@google.com> wrote: > 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 > 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 attached. > > Thanks. > > -- > Best Regards, Tong Shen -- Best Regards, Tong Shen [-- Attachment #2: perf.patch --] [-- Type: text/x-patch, Size: 747 bytes --] 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..0606c6f 100644 --- a/perf-3.12.0/tools/perf/util/symbol-elf.c +++ 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))) { 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 related [flat|nested] 11+ messages in thread
* Re: Fix Symbol Address for ET_DYN 2014-09-05 18:38 Fix Symbol Address for ET_DYN Tong Shen 2014-09-05 18:40 ` Tong Shen @ 2014-09-11 15:01 ` Arnaldo Carvalho de Melo 2014-09-11 18:21 ` Tong Shen 1 sibling, 1 reply; 11+ messages in thread From: Arnaldo Carvalho de Melo @ 2014-09-11 15:01 UTC (permalink / raw) To: Tong Shen; +Cc: linux-perf-users 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Fix Symbol Address for ET_DYN 2014-09-11 15:01 ` Arnaldo Carvalho de Melo @ 2014-09-11 18:21 ` Tong Shen 2014-09-11 20:38 ` [PATCH 1/1] perf tools: " Arnaldo Carvalho de Melo 0 siblings, 1 reply; 11+ messages in thread From: Tong Shen @ 2014-09-11 18:21 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: linux-perf-users [-- Attachment #1: Type: text/plain, Size: 3013 bytes --] 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. 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 [-- Attachment #2: perf.patch --] [-- Type: text/x-patch, Size: 1882 bytes --] 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 related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN 2014-09-11 18:21 ` Tong Shen @ 2014-09-11 20:38 ` Arnaldo Carvalho de Melo 2014-09-15 18:06 ` Tong Shen 0 siblings, 1 reply; 11+ 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] 11+ 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: " Arnaldo Carvalho de Melo @ 2014-09-15 18:06 ` Tong Shen 2014-09-25 0:28 ` Tong Shen 0 siblings, 1 reply; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread
end of thread, other threads:[~2014-09-29 18:13 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-05 18:38 Fix Symbol Address for ET_DYN Tong Shen 2014-09-05 18:40 ` Tong Shen 2014-09-11 15:01 ` Arnaldo Carvalho de Melo 2014-09-11 18:21 ` Tong Shen 2014-09-11 20:38 ` [PATCH 1/1] perf tools: " 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).