| Message ID | ee7a0b4944093db90a589040be39ef3e7e747f63.1724820993.git.scclevenger@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset | expand |
On 8/28/2024 6:09 AM, Steve Clevenger wrote: > > Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for > the DF_1_PIE flag. This identifies position executable code. > > Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com> > --- > tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++ > tools/perf/util/symbol.h | 1 + > 2 files changed, 61 insertions(+) > > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c > index e398abfd13a0..0e49c1345a67 100644 > --- a/tools/perf/util/symbol-elf.c > +++ b/tools/perf/util/symbol-elf.c > @@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, > return err; > } > > +/* > + * Check dynamic section DT_FLAGS_1 for a Position Independent > + * Executable (PIE). > + */ > +bool dso__is_pie(struct dso *dso) > +{ > + Elf *elf = NULL; > + Elf_Scn *scn = NULL; > + GElf_Ehdr ehdr; > + GElf_Shdr shdr; > + bool is_pie = false; > + char dso_path[PATH_MAX]; > + int fd = -1; > + > + if (!dso || (elf_version(EV_CURRENT) == EV_NONE)) > + goto exit; // false > + > + dso__build_id_filename(dso, dso_path, sizeof(dso_path), false); > + > + fd = open(dso_path, O_RDONLY); > + > + if (fd < 0) { > + pr_debug("%s: cannot read cached %s.\n", __func__, dso_path); > + goto exit; // false > + } > + > + elf = elf_begin(fd, ELF_C_READ, NULL); > + gelf_getehdr(elf, &ehdr); > + > + if (ehdr.e_type == ET_DYN) { > + Elf_Data *data; > + GElf_Dyn *entry; > + int n_entries = shdr.sh_size / sizeof(GElf_Dyn); > + > + scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL); > + if (!scn) > + goto exit; // false It should goto a new tag (e.g. exit_failed) so can invoke elf_end(elf) and close(fd). > + > + data = (Elf_Data *) elf_getdata(scn, NULL); > + if (!data || !data->d_buf) > + goto exit; // false Ditto. > + > + // check DT_FLAGS_1 > + for (int i = 0; i < n_entries; i++) { > + entry = ((GElf_Dyn *) data->d_buf) + i; > + if (entry->d_tag == DT_FLAGS_1) { > + if ((entry->d_un.d_val & DF_1_PIE) != 0) { > + is_pie = true; > + break; > + } > + } > + } // end for > + } > + Put 'exit_failed' tag at here. With the changes: Reviewed-by: Leo Yan <leo.yan@arm.com> > + elf_end(elf); > + close(fd); > +exit: > + return is_pie; > +} > + > /* > * We need to check if we have a .dynsym, so that we can handle the > * .plt, synthesizing its symbols, that aren't on the symtabs (be it > diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h > index 3fb5d146d9b1..33ea2596ce31 100644 > --- a/tools/perf/util/symbol.h > +++ b/tools/perf/util/symbol.h > @@ -127,6 +127,7 @@ void dso__insert_symbol(struct dso *dso, > struct symbol *sym); > void dso__delete_symbol(struct dso *dso, > struct symbol *sym); > +bool dso__is_pie(struct dso *dso); > > struct symbol *dso__find_symbol(struct dso *dso, u64 addr); > struct symbol *dso__find_symbol_nocache(struct dso *dso, u64 addr); > -- > 2.25.1 >
On 8/28/2024 1:23 AM, Leo Yan wrote: > On 8/28/2024 6:09 AM, Steve Clevenger wrote: >> >> Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for >> the DF_1_PIE flag. This identifies position executable code. >> >> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com> >> --- >> tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++ >> tools/perf/util/symbol.h | 1 + >> 2 files changed, 61 insertions(+) >> >> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c >> index e398abfd13a0..0e49c1345a67 100644 >> --- a/tools/perf/util/symbol-elf.c >> +++ b/tools/perf/util/symbol-elf.c >> @@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, >> return err; >> } >> >> +/* >> + * Check dynamic section DT_FLAGS_1 for a Position Independent >> + * Executable (PIE). >> + */ >> +bool dso__is_pie(struct dso *dso) >> +{ >> + Elf *elf = NULL; >> + Elf_Scn *scn = NULL; >> + GElf_Ehdr ehdr; >> + GElf_Shdr shdr; >> + bool is_pie = false; >> + char dso_path[PATH_MAX]; >> + int fd = -1; >> + >> + if (!dso || (elf_version(EV_CURRENT) == EV_NONE)) >> + goto exit; // false >> + >> + dso__build_id_filename(dso, dso_path, sizeof(dso_path), false); >> + >> + fd = open(dso_path, O_RDONLY); >> + >> + if (fd < 0) { >> + pr_debug("%s: cannot read cached %s.\n", __func__, dso_path); >> + goto exit; // false >> + } >> + >> + elf = elf_begin(fd, ELF_C_READ, NULL); >> + gelf_getehdr(elf, &ehdr); >> + >> + if (ehdr.e_type == ET_DYN) { >> + Elf_Data *data; >> + GElf_Dyn *entry; >> + int n_entries = shdr.sh_size / sizeof(GElf_Dyn); >> + >> + scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL); >> + if (!scn) >> + goto exit; // false > > It should goto a new tag (e.g. exit_failed) so can invoke elf_end(elf) and > close(fd). > >> + >> + data = (Elf_Data *) elf_getdata(scn, NULL); >> + if (!data || !data->d_buf) >> + goto exit; // false > > Ditto. > >> + >> + // check DT_FLAGS_1 >> + for (int i = 0; i < n_entries; i++) { >> + entry = ((GElf_Dyn *) data->d_buf) + i; >> + if (entry->d_tag == DT_FLAGS_1) { >> + if ((entry->d_un.d_val & DF_1_PIE) != 0) { >> + is_pie = true; >> + break; >> + } >> + } >> + } // end for >> + } >> + > > Put 'exit_failed' tag at here. > > With the changes: > > Reviewed-by: Leo Yan <leo.yan@arm.com> > >> + elf_end(elf); >> + close(fd); >> +exit: >> + return is_pie; >> +} >> + Hi Leo, It's been a long time since I've seen goto's used as a rule rather than an exception. I see it introduced the problem you've mentioned. Is it ok to simply update the cover letter (patch 0), and this one (patch 1) as V5? If I don't hear from you, I'll resubmit all. Thanks, Steve >> /* >> * We need to check if we have a .dynsym, so that we can handle the >> * .plt, synthesizing its symbols, that aren't on the symtabs (be it >> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h >> index 3fb5d146d9b1..33ea2596ce31 100644 >> --- a/tools/perf/util/symbol.h >> +++ b/tools/perf/util/symbol.h >> @@ -127,6 +127,7 @@ void dso__insert_symbol(struct dso *dso, >> struct symbol *sym); >> void dso__delete_symbol(struct dso *dso, >> struct symbol *sym); >> +bool dso__is_pie(struct dso *dso); >> >> struct symbol *dso__find_symbol(struct dso *dso, u64 addr); >> struct symbol *dso__find_symbol_nocache(struct dso *dso, u64 addr); >> -- >> 2.25.1 >>
On 8/28/2024 4:48 PM, Steve Clevenger wrote: > > On 8/28/2024 1:23 AM, Leo Yan wrote: >> On 8/28/2024 6:09 AM, Steve Clevenger wrote: >>> >>> Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for >>> the DF_1_PIE flag. This identifies position executable code. >>> >>> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com> >>> --- >>> tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++ >>> tools/perf/util/symbol.h | 1 + >>> 2 files changed, 61 insertions(+) >>> >>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c >>> index e398abfd13a0..0e49c1345a67 100644 >>> --- a/tools/perf/util/symbol-elf.c >>> +++ b/tools/perf/util/symbol-elf.c >>> @@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, >>> return err; >>> } >>> >>> +/* >>> + * Check dynamic section DT_FLAGS_1 for a Position Independent >>> + * Executable (PIE). >>> + */ >>> +bool dso__is_pie(struct dso *dso) >>> +{ >>> + Elf *elf = NULL; >>> + Elf_Scn *scn = NULL; >>> + GElf_Ehdr ehdr; >>> + GElf_Shdr shdr; >>> + bool is_pie = false; >>> + char dso_path[PATH_MAX]; >>> + int fd = -1; >>> + >>> + if (!dso || (elf_version(EV_CURRENT) == EV_NONE)) >>> + goto exit; // false >>> + >>> + dso__build_id_filename(dso, dso_path, sizeof(dso_path), false); >>> + >>> + fd = open(dso_path, O_RDONLY); >>> + >>> + if (fd < 0) { >>> + pr_debug("%s: cannot read cached %s.\n", __func__, dso_path); >>> + goto exit; // false >>> + } >>> + >>> + elf = elf_begin(fd, ELF_C_READ, NULL); >>> + gelf_getehdr(elf, &ehdr); >>> + >>> + if (ehdr.e_type == ET_DYN) { >>> + Elf_Data *data; >>> + GElf_Dyn *entry; >>> + int n_entries = shdr.sh_size / sizeof(GElf_Dyn); >>> + >>> + scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL); >>> + if (!scn) >>> + goto exit; // false >> >> It should goto a new tag (e.g. exit_failed) so can invoke elf_end(elf) and >> close(fd). >> >>> + >>> + data = (Elf_Data *) elf_getdata(scn, NULL); >>> + if (!data || !data->d_buf) >>> + goto exit; // false >> >> Ditto. >> >>> + >>> + // check DT_FLAGS_1 >>> + for (int i = 0; i < n_entries; i++) { >>> + entry = ((GElf_Dyn *) data->d_buf) + i; >>> + if (entry->d_tag == DT_FLAGS_1) { >>> + if ((entry->d_un.d_val & DF_1_PIE) != 0) { >>> + is_pie = true; >>> + break; >>> + } >>> + } >>> + } // end for >>> + } >>> + >> >> Put 'exit_failed' tag at here. >> >> With the changes: >> >> Reviewed-by: Leo Yan <leo.yan@arm.com> >> >>> + elf_end(elf); >>> + close(fd); >>> +exit: >>> + return is_pie; >>> +} >>> + > > Hi Leo, > > It's been a long time since I've seen goto's used as a rule rather than > an exception. I see it introduced the problem you've mentioned. I am not sure if I understand this. In the Linux code, it is quite common to use goto to handle failure cases (for releasing resources). > Is it ok to simply update the cover letter (patch 0), and this one (patch 1) > as V5? If I don't hear from you, I'll resubmit all. I assume you also need to address patch 03? It is good to send the all patches in one go, this would be easier for maintainers to pick up the whole series (e.g. using b4). Thanks, Leo
On 8/28/2024 4:48 PM, Steve Clevenger wrote: > > On 8/28/2024 1:23 AM, Leo Yan wrote: >> On 8/28/2024 6:09 AM, Steve Clevenger wrote: >>> >>> Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for >>> the DF_1_PIE flag. This identifies position executable code. >>> >>> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com> >>> --- >>> tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++ >>> tools/perf/util/symbol.h | 1 + >>> 2 files changed, 61 insertions(+) >>> >>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c >>> index e398abfd13a0..0e49c1345a67 100644 >>> --- a/tools/perf/util/symbol-elf.c >>> +++ b/tools/perf/util/symbol-elf.c >>> @@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, >>> return err; >>> } >>> >>> +/* >>> + * Check dynamic section DT_FLAGS_1 for a Position Independent >>> + * Executable (PIE). >>> + */ >>> +bool dso__is_pie(struct dso *dso) >>> +{ >>> + Elf *elf = NULL; >>> + Elf_Scn *scn = NULL; >>> + GElf_Ehdr ehdr; >>> + GElf_Shdr shdr; >>> + bool is_pie = false; >>> + char dso_path[PATH_MAX]; >>> + int fd = -1; >>> + >>> + if (!dso || (elf_version(EV_CURRENT) == EV_NONE)) >>> + goto exit; // false >>> + >>> + dso__build_id_filename(dso, dso_path, sizeof(dso_path), false); >>> + >>> + fd = open(dso_path, O_RDONLY); >>> + >>> + if (fd < 0) { >>> + pr_debug("%s: cannot read cached %s.\n", __func__, dso_path); >>> + goto exit; // false >>> + } >>> + >>> + elf = elf_begin(fd, ELF_C_READ, NULL); >>> + gelf_getehdr(elf, &ehdr); >>> + >>> + if (ehdr.e_type == ET_DYN) { >>> + Elf_Data *data; >>> + GElf_Dyn *entry; >>> + int n_entries = shdr.sh_size / sizeof(GElf_Dyn); >>> + >>> + scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL); >>> + if (!scn) >>> + goto exit; // false >> >> It should goto a new tag (e.g. exit_failed) so can invoke elf_end(elf) and >> close(fd). >> >>> + >>> + data = (Elf_Data *) elf_getdata(scn, NULL); >>> + if (!data || !data->d_buf) >>> + goto exit; // false >> >> Ditto. >> >>> + >>> + // check DT_FLAGS_1 >>> + for (int i = 0; i < n_entries; i++) { >>> + entry = ((GElf_Dyn *) data->d_buf) + i; >>> + if (entry->d_tag == DT_FLAGS_1) { >>> + if ((entry->d_un.d_val & DF_1_PIE) != 0) { >>> + is_pie = true; >>> + break; >>> + } >>> + } >>> + } // end for >>> + } >>> + >> >> Put 'exit_failed' tag at here. >> >> With the changes: >> >> Reviewed-by: Leo Yan <leo.yan@arm.com> >> >>> + elf_end(elf); >>> + close(fd); >>> +exit: >>> + return is_pie; >>> +} >>> + > > Hi Leo, > > It's been a long time since I've seen goto's used as a rule rather than > an exception. I see it introduced the problem you've mentioned. I am not sure if I understand this. In the Linux code, it is quite common to use goto to handle failure cases (for releasing resources). > Is it ok to simply update the cover letter (patch 0), and this one (patch 1) > as V5? If I don't hear from you, I'll resubmit all. I assume you also need to address patch 03? It is good to send the all patches in one go, this would be easier for maintainers to pick up the whole series (e.g. using b4). Thanks, Leo
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c index e398abfd13a0..0e49c1345a67 100644 --- a/tools/perf/util/symbol-elf.c +++ b/tools/perf/util/symbol-elf.c @@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, return err; } +/* + * Check dynamic section DT_FLAGS_1 for a Position Independent + * Executable (PIE). + */ +bool dso__is_pie(struct dso *dso) +{ + Elf *elf = NULL; + Elf_Scn *scn = NULL; + GElf_Ehdr ehdr; + GElf_Shdr shdr; + bool is_pie = false; + char dso_path[PATH_MAX]; + int fd = -1; + + if (!dso || (elf_version(EV_CURRENT) == EV_NONE)) + goto exit; // false + + dso__build_id_filename(dso, dso_path, sizeof(dso_path), false); + + fd = open(dso_path, O_RDONLY); + + if (fd < 0) { + pr_debug("%s: cannot read cached %s.\n", __func__, dso_path); + goto exit; // false + } + + elf = elf_begin(fd, ELF_C_READ, NULL); + gelf_getehdr(elf, &ehdr); + + if (ehdr.e_type == ET_DYN) { + Elf_Data *data; + GElf_Dyn *entry; + int n_entries = shdr.sh_size / sizeof(GElf_Dyn); + + scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL); + if (!scn) + goto exit; // false + + data = (Elf_Data *) elf_getdata(scn, NULL); + if (!data || !data->d_buf) + goto exit; // false + + // check DT_FLAGS_1 + for (int i = 0; i < n_entries; i++) { + entry = ((GElf_Dyn *) data->d_buf) + i; + if (entry->d_tag == DT_FLAGS_1) { + if ((entry->d_un.d_val & DF_1_PIE) != 0) { + is_pie = true; + break; + } + } + } // end for + } + + elf_end(elf); + close(fd); +exit: + return is_pie; +} + /* * We need to check if we have a .dynsym, so that we can handle the * .plt, synthesizing its symbols, that aren't on the symtabs (be it diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h index 3fb5d146d9b1..33ea2596ce31 100644 --- a/tools/perf/util/symbol.h +++ b/tools/perf/util/symbol.h @@ -127,6 +127,7 @@ void dso__insert_symbol(struct dso *dso, struct symbol *sym); void dso__delete_symbol(struct dso *dso, struct symbol *sym); +bool dso__is_pie(struct dso *dso); struct symbol *dso__find_symbol(struct dso *dso, u64 addr); struct symbol *dso__find_symbol_nocache(struct dso *dso, u64 addr);
Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for the DF_1_PIE flag. This identifies position executable code. Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com> --- tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++ tools/perf/util/symbol.h | 1 + 2 files changed, 61 insertions(+)