Message ID | 20171012021950.GA21344@gmail.com |
---|---|
State | New |
Headers | show |
Series | Support profiling PIE [BZ #22284] | expand |
On 10/11/2017 07:19 PM, H.J. Lu wrote: > Since PIE can be loaded at any address, we need to subtract load address > from PCs. > > Tested on i686 and x86-64. OK for master? OK, with one added comment, see below. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > > H.J. > --- > [BZ #22284] > * gmon/Makefile [$(have-fpie)$(build-shared) == yesyes] (tests, > tests-pie): Add tst-gmon-pie. > (CFLAGS-tst-gmon-pie.c): New. > (CRT-tst-gmon-pie): Likewise. > (tst-gmon-pie-ENV): Likewise. > [$(have-fpie)$(build-shared) == yesyes] (tests-special): Likewise. > ($(objpfx)tst-gmon-pie.out): Likewise. > (clean-tst-gmon-pie-data): Likewise. > ($(objpfx)tst-gmon-pie-gprof.out): Likewise. > * gmon/gmon.c [PIC]: Include <link.h>. > [PIC] (callback): New function. > (write_hist): Add an argument for load address. Subtract load > address from PCs. > (write_call_graph): Likewise. > (write_gmon): Call __dl_iterate_phdr to get load address, pass > it to write_hist and write_call_graph. > --- > gmon/Makefile | 21 +++++++++++++++++++++ > gmon/gmon.c | 44 ++++++++++++++++++++++++++++++++++---------- > gmon/tst-gmon-pie.c | 1 + > 3 files changed, 56 insertions(+), 10 deletions(-) > create mode 100644 gmon/tst-gmon-pie.c > > diff --git a/gmon/Makefile b/gmon/Makefile > index 79e29d188f..2cd077dece 100644 > --- a/gmon/Makefile > +++ b/gmon/Makefile > @@ -33,6 +33,11 @@ tests-static += tst-profile-static > LDFLAGS-tst-profile-static = -profile > endif > > +ifeq (yesyes,$(have-fpie)$(build-shared)) > +tests += tst-gmon-pie > +tests-pie += tst-gmon-pie > +endif OK. > + > # The mcount code won't work without a frame pointer. > CFLAGS-mcount.c := -fno-omit-frame-pointer > > @@ -44,6 +49,14 @@ ifeq ($(run-built-tests),yes) > tests-special += $(objpfx)tst-gmon-gprof.out > endif > > +CFLAGS-tst-gmon-pie.c := $(PIE-ccflag) -fno-omit-frame-pointer -pg > +CRT-tst-gmon-pie := $(csu-objpfx)gcrt1.o > +tst-gmon-pie-ENV := GMON_OUT_PREFIX=$(objpfx)tst-gmon-pie.data > +ifeq ($(run-built-tests),yes) > +tests-special += $(objpfx)tst-gmon-pie-gprof.out > +endif OK. > + > + > include ../Rules > > # We cannot compile mcount.c with -pg because that would > @@ -69,3 +82,11 @@ clean-tst-gmon-data: > $(objpfx)tst-gmon-gprof.out: tst-gmon-gprof.sh $(objpfx)tst-gmon.out > $(SHELL) $< $(GPROF) $(objpfx)tst-gmon $(objpfx)tst-gmon.data.* > $@; \ > $(evaluate-test) > + > +$(objpfx)tst-gmon-pie.out: clean-tst-gmon-pie-data > +clean-tst-gmon-pie-data: > + rm -f $(objpfx)tst-gmon-pie.data.* > + > +$(objpfx)tst-gmon-pie-gprof.out: tst-gmon-gprof.sh $(objpfx)tst-gmon-pie.out > + $(SHELL) $< $(GPROF) $(objpfx)tst-gmon-pie $(objpfx)tst-gmon-pie.data.* > $@; \ > + $(evaluate-test) OK. > diff --git a/gmon/gmon.c b/gmon/gmon.c > index f1aa3b776c..5f569cc994 100644 > --- a/gmon/gmon.c > +++ b/gmon/gmon.c > @@ -46,6 +46,23 @@ > #include <libc-internal.h> > #include <not-cancel.h> > > +#ifdef PIC > +# include <link.h> > + > +static int > +callback (struct dl_phdr_info *info, size_t size, void *data) > +{ > + u_long *load_address = data; > + Does the dl_iterate_phdr ABI mandate taht the application istelf is always an empty string? You should add a comment to that effect here if you believe that to be true. > + if (info->dlpi_name[0] == '\0') > + { > + *load_address = (u_long) info->dlpi_addr; > + return 1; > + } > + > + return 0; > +} > +#endif > > /* Head of basic-block list or NULL. */ > struct __bb *__bb_head attribute_hidden; > @@ -63,8 +80,8 @@ static int s_scale; > void moncontrol (int mode); > void __moncontrol (int mode); > libc_hidden_proto (__moncontrol) > -static void write_hist (int fd); > -static void write_call_graph (int fd); > +static void write_hist (int fd, u_long load_address); > +static void write_call_graph (int fd, u_long load_address); OK. > static void write_bb_counts (int fd); > > /* > @@ -173,7 +190,7 @@ weak_alias (__monstartup, monstartup) > > > static void > -write_hist (int fd) > +write_hist (int fd, u_long load_address) > { > u_char tag = GMON_TAG_TIME_HIST; > > @@ -210,8 +227,8 @@ write_hist (int fd) > != offsetof (struct gmon_hist_hdr, dimen_abbrev))) > abort (); > > - thdr.low_pc = (char *) _gmonparam.lowpc; > - thdr.high_pc = (char *) _gmonparam.highpc; > + thdr.low_pc = (char *) _gmonparam.lowpc - load_address; > + thdr.high_pc = (char *) _gmonparam.highpc - load_address; OK. > thdr.hist_size = _gmonparam.kcountsize / sizeof (HISTCOUNTER); > thdr.prof_rate = __profile_frequency (); > strncpy (thdr.dimen, "seconds", sizeof (thdr.dimen)); > @@ -223,7 +240,7 @@ write_hist (int fd) > > > static void > -write_call_graph (int fd) > +write_call_graph (int fd, u_long load_address) > { > #define NARCS_PER_WRITEV 32 > u_char tag = GMON_TAG_CG_ARC; > @@ -266,8 +283,9 @@ write_call_graph (int fd) > } > arc; > > - arc.frompc = (char *) frompc; > - arc.selfpc = (char *) _gmonparam.tos[to_index].selfpc; > + arc.frompc = (char *) frompc - load_address; > + arc.selfpc = ((char *) _gmonparam.tos[to_index].selfpc > + - load_address); OK. > arc.count = _gmonparam.tos[to_index].count; > memcpy (raw_arc + nfilled, &arc, sizeof (raw_arc [0])); > > @@ -376,11 +394,17 @@ write_gmon (void) > memset (ghdr.spare, '\0', sizeof (ghdr.spare)); > __write_nocancel (fd, &ghdr, sizeof (struct gmon_hdr)); > > + /* Get load_address to profile PIE. */ > + u_long load_address = 0; > +#ifdef PIC > + __dl_iterate_phdr (callback, &load_address); > +#endif OK. > + > /* write PC histogram: */ > - write_hist (fd); > + write_hist (fd, load_address); OK. > > /* write call-graph: */ > - write_call_graph (fd); > + write_call_graph (fd, load_address); OK. > > /* write basic-block execution counts: */ > write_bb_counts (fd); > diff --git a/gmon/tst-gmon-pie.c b/gmon/tst-gmon-pie.c > new file mode 100644 > index 0000000000..1eef2583b6 > --- /dev/null > +++ b/gmon/tst-gmon-pie.c > @@ -0,0 +1 @@ > +#include "tst-gmon.c" > -- 2.13.6
On 10/11/17, Carlos O'Donell <carlos@redhat.com> wrote: > On 10/11/2017 07:19 PM, H.J. Lu wrote: >> Since PIE can be loaded at any address, we need to subtract load address >> from PCs. >> >> Tested on i686 and x86-64. OK for master? > > OK, with one added comment, see below. > > Reviewed-by: Carlos O'Donell <carlos@redhat.com> > >> >> H.J. >> --- >> [BZ #22284] >> * gmon/Makefile [$(have-fpie)$(build-shared) == yesyes] (tests, >> tests-pie): Add tst-gmon-pie. >> (CFLAGS-tst-gmon-pie.c): New. >> (CRT-tst-gmon-pie): Likewise. >> (tst-gmon-pie-ENV): Likewise. >> [$(have-fpie)$(build-shared) == yesyes] (tests-special): Likewise. >> ($(objpfx)tst-gmon-pie.out): Likewise. >> (clean-tst-gmon-pie-data): Likewise. >> ($(objpfx)tst-gmon-pie-gprof.out): Likewise. >> * gmon/gmon.c [PIC]: Include <link.h>. >> [PIC] (callback): New function. >> (write_hist): Add an argument for load address. Subtract load >> address from PCs. >> (write_call_graph): Likewise. >> (write_gmon): Call __dl_iterate_phdr to get load address, pass >> it to write_hist and write_call_graph. >> --- >> gmon/Makefile | 21 +++++++++++++++++++++ >> gmon/gmon.c | 44 ++++++++++++++++++++++++++++++++++---------- >> gmon/tst-gmon-pie.c | 1 + >> 3 files changed, 56 insertions(+), 10 deletions(-) >> create mode 100644 gmon/tst-gmon-pie.c >> >> diff --git a/gmon/Makefile b/gmon/Makefile >> index 79e29d188f..2cd077dece 100644 >> --- a/gmon/Makefile >> +++ b/gmon/Makefile >> @@ -33,6 +33,11 @@ tests-static += tst-profile-static >> LDFLAGS-tst-profile-static = -profile >> endif >> >> +ifeq (yesyes,$(have-fpie)$(build-shared)) >> +tests += tst-gmon-pie >> +tests-pie += tst-gmon-pie >> +endif > > OK. > >> + >> # The mcount code won't work without a frame pointer. >> CFLAGS-mcount.c := -fno-omit-frame-pointer >> >> @@ -44,6 +49,14 @@ ifeq ($(run-built-tests),yes) >> tests-special += $(objpfx)tst-gmon-gprof.out >> endif >> >> +CFLAGS-tst-gmon-pie.c := $(PIE-ccflag) -fno-omit-frame-pointer -pg >> +CRT-tst-gmon-pie := $(csu-objpfx)gcrt1.o >> +tst-gmon-pie-ENV := GMON_OUT_PREFIX=$(objpfx)tst-gmon-pie.data >> +ifeq ($(run-built-tests),yes) >> +tests-special += $(objpfx)tst-gmon-pie-gprof.out >> +endif > > > OK. > >> + >> + >> include ../Rules >> >> # We cannot compile mcount.c with -pg because that would >> @@ -69,3 +82,11 @@ clean-tst-gmon-data: >> $(objpfx)tst-gmon-gprof.out: tst-gmon-gprof.sh $(objpfx)tst-gmon.out >> $(SHELL) $< $(GPROF) $(objpfx)tst-gmon $(objpfx)tst-gmon.data.* > $@; \ >> $(evaluate-test) >> + >> +$(objpfx)tst-gmon-pie.out: clean-tst-gmon-pie-data >> +clean-tst-gmon-pie-data: >> + rm -f $(objpfx)tst-gmon-pie.data.* >> + >> +$(objpfx)tst-gmon-pie-gprof.out: tst-gmon-gprof.sh >> $(objpfx)tst-gmon-pie.out >> + $(SHELL) $< $(GPROF) $(objpfx)tst-gmon-pie $(objpfx)tst-gmon-pie.data.* >> > $@; \ >> + $(evaluate-test) > > OK. > >> diff --git a/gmon/gmon.c b/gmon/gmon.c >> index f1aa3b776c..5f569cc994 100644 >> --- a/gmon/gmon.c >> +++ b/gmon/gmon.c >> @@ -46,6 +46,23 @@ >> #include <libc-internal.h> >> #include <not-cancel.h> >> >> +#ifdef PIC >> +# include <link.h> >> + >> +static int >> +callback (struct dl_phdr_info *info, size_t size, void *data) >> +{ >> + u_long *load_address = data; >> + > > Does the dl_iterate_phdr ABI mandate taht the application istelf > is always an empty string? > > You should add a comment to that effect here if you believe that > to be true. This is what I checked in. Thanks. >> + if (info->dlpi_name[0] == '\0') >> + { >> + *load_address = (u_long) info->dlpi_addr; >> + return 1; >> + } >> + >> + return 0; >> +} >> +#endif >> >> /* Head of basic-block list or NULL. */ >> struct __bb *__bb_head attribute_hidden; >> @@ -63,8 +80,8 @@ static int s_scale; >> void moncontrol (int mode); >> void __moncontrol (int mode); >> libc_hidden_proto (__moncontrol) >> -static void write_hist (int fd); >> -static void write_call_graph (int fd); >> +static void write_hist (int fd, u_long load_address); >> +static void write_call_graph (int fd, u_long load_address); > > OK. > >> static void write_bb_counts (int fd); >> >> /* >> @@ -173,7 +190,7 @@ weak_alias (__monstartup, monstartup) >> >> >> static void >> -write_hist (int fd) >> +write_hist (int fd, u_long load_address) >> { >> u_char tag = GMON_TAG_TIME_HIST; >> >> @@ -210,8 +227,8 @@ write_hist (int fd) >> != offsetof (struct gmon_hist_hdr, dimen_abbrev))) >> abort (); >> >> - thdr.low_pc = (char *) _gmonparam.lowpc; >> - thdr.high_pc = (char *) _gmonparam.highpc; >> + thdr.low_pc = (char *) _gmonparam.lowpc - load_address; >> + thdr.high_pc = (char *) _gmonparam.highpc - load_address; > > OK. > >> thdr.hist_size = _gmonparam.kcountsize / sizeof (HISTCOUNTER); >> thdr.prof_rate = __profile_frequency (); >> strncpy (thdr.dimen, "seconds", sizeof (thdr.dimen)); >> @@ -223,7 +240,7 @@ write_hist (int fd) >> >> >> static void >> -write_call_graph (int fd) >> +write_call_graph (int fd, u_long load_address) >> { >> #define NARCS_PER_WRITEV 32 >> u_char tag = GMON_TAG_CG_ARC; >> @@ -266,8 +283,9 @@ write_call_graph (int fd) >> } >> arc; >> >> - arc.frompc = (char *) frompc; >> - arc.selfpc = (char *) _gmonparam.tos[to_index].selfpc; >> + arc.frompc = (char *) frompc - load_address; >> + arc.selfpc = ((char *) _gmonparam.tos[to_index].selfpc >> + - load_address); > > OK. > >> arc.count = _gmonparam.tos[to_index].count; >> memcpy (raw_arc + nfilled, &arc, sizeof (raw_arc [0])); >> >> @@ -376,11 +394,17 @@ write_gmon (void) >> memset (ghdr.spare, '\0', sizeof (ghdr.spare)); >> __write_nocancel (fd, &ghdr, sizeof (struct gmon_hdr)); >> >> + /* Get load_address to profile PIE. */ >> + u_long load_address = 0; >> +#ifdef PIC >> + __dl_iterate_phdr (callback, &load_address); >> +#endif > > OK. > >> + >> /* write PC histogram: */ >> - write_hist (fd); >> + write_hist (fd, load_address); > > OK. > >> >> /* write call-graph: */ >> - write_call_graph (fd); >> + write_call_graph (fd, load_address); > > OK. > > >> >> /* write basic-block execution counts: */ >> write_bb_counts (fd); >> diff --git a/gmon/tst-gmon-pie.c b/gmon/tst-gmon-pie.c >> new file mode 100644 >> index 0000000000..1eef2583b6 >> --- /dev/null >> +++ b/gmon/tst-gmon-pie.c >> @@ -0,0 +1 @@ >> +#include "tst-gmon.c" >> -- 2.13.6 > > > -- > Cheers, > Carlos. >
diff --git a/gmon/Makefile b/gmon/Makefile index 79e29d188f..2cd077dece 100644 --- a/gmon/Makefile +++ b/gmon/Makefile @@ -33,6 +33,11 @@ tests-static += tst-profile-static LDFLAGS-tst-profile-static = -profile endif +ifeq (yesyes,$(have-fpie)$(build-shared)) +tests += tst-gmon-pie +tests-pie += tst-gmon-pie +endif + # The mcount code won't work without a frame pointer. CFLAGS-mcount.c := -fno-omit-frame-pointer @@ -44,6 +49,14 @@ ifeq ($(run-built-tests),yes) tests-special += $(objpfx)tst-gmon-gprof.out endif +CFLAGS-tst-gmon-pie.c := $(PIE-ccflag) -fno-omit-frame-pointer -pg +CRT-tst-gmon-pie := $(csu-objpfx)gcrt1.o +tst-gmon-pie-ENV := GMON_OUT_PREFIX=$(objpfx)tst-gmon-pie.data +ifeq ($(run-built-tests),yes) +tests-special += $(objpfx)tst-gmon-pie-gprof.out +endif + + include ../Rules # We cannot compile mcount.c with -pg because that would @@ -69,3 +82,11 @@ clean-tst-gmon-data: $(objpfx)tst-gmon-gprof.out: tst-gmon-gprof.sh $(objpfx)tst-gmon.out $(SHELL) $< $(GPROF) $(objpfx)tst-gmon $(objpfx)tst-gmon.data.* > $@; \ $(evaluate-test) + +$(objpfx)tst-gmon-pie.out: clean-tst-gmon-pie-data +clean-tst-gmon-pie-data: + rm -f $(objpfx)tst-gmon-pie.data.* + +$(objpfx)tst-gmon-pie-gprof.out: tst-gmon-gprof.sh $(objpfx)tst-gmon-pie.out + $(SHELL) $< $(GPROF) $(objpfx)tst-gmon-pie $(objpfx)tst-gmon-pie.data.* > $@; \ + $(evaluate-test) diff --git a/gmon/gmon.c b/gmon/gmon.c index f1aa3b776c..5f569cc994 100644 --- a/gmon/gmon.c +++ b/gmon/gmon.c @@ -46,6 +46,23 @@ #include <libc-internal.h> #include <not-cancel.h> +#ifdef PIC +# include <link.h> + +static int +callback (struct dl_phdr_info *info, size_t size, void *data) +{ + u_long *load_address = data; + + if (info->dlpi_name[0] == '\0') + { + *load_address = (u_long) info->dlpi_addr; + return 1; + } + + return 0; +} +#endif /* Head of basic-block list or NULL. */ struct __bb *__bb_head attribute_hidden; @@ -63,8 +80,8 @@ static int s_scale; void moncontrol (int mode); void __moncontrol (int mode); libc_hidden_proto (__moncontrol) -static void write_hist (int fd); -static void write_call_graph (int fd); +static void write_hist (int fd, u_long load_address); +static void write_call_graph (int fd, u_long load_address); static void write_bb_counts (int fd); /* @@ -173,7 +190,7 @@ weak_alias (__monstartup, monstartup) static void -write_hist (int fd) +write_hist (int fd, u_long load_address) { u_char tag = GMON_TAG_TIME_HIST; @@ -210,8 +227,8 @@ write_hist (int fd) != offsetof (struct gmon_hist_hdr, dimen_abbrev))) abort (); - thdr.low_pc = (char *) _gmonparam.lowpc; - thdr.high_pc = (char *) _gmonparam.highpc; + thdr.low_pc = (char *) _gmonparam.lowpc - load_address; + thdr.high_pc = (char *) _gmonparam.highpc - load_address; thdr.hist_size = _gmonparam.kcountsize / sizeof (HISTCOUNTER); thdr.prof_rate = __profile_frequency (); strncpy (thdr.dimen, "seconds", sizeof (thdr.dimen)); @@ -223,7 +240,7 @@ write_hist (int fd) static void -write_call_graph (int fd) +write_call_graph (int fd, u_long load_address) { #define NARCS_PER_WRITEV 32 u_char tag = GMON_TAG_CG_ARC; @@ -266,8 +283,9 @@ write_call_graph (int fd) } arc; - arc.frompc = (char *) frompc; - arc.selfpc = (char *) _gmonparam.tos[to_index].selfpc; + arc.frompc = (char *) frompc - load_address; + arc.selfpc = ((char *) _gmonparam.tos[to_index].selfpc + - load_address); arc.count = _gmonparam.tos[to_index].count; memcpy (raw_arc + nfilled, &arc, sizeof (raw_arc [0])); @@ -376,11 +394,17 @@ write_gmon (void) memset (ghdr.spare, '\0', sizeof (ghdr.spare)); __write_nocancel (fd, &ghdr, sizeof (struct gmon_hdr)); + /* Get load_address to profile PIE. */ + u_long load_address = 0; +#ifdef PIC + __dl_iterate_phdr (callback, &load_address); +#endif + /* write PC histogram: */ - write_hist (fd); + write_hist (fd, load_address); /* write call-graph: */ - write_call_graph (fd); + write_call_graph (fd, load_address); /* write basic-block execution counts: */ write_bb_counts (fd); diff --git a/gmon/tst-gmon-pie.c b/gmon/tst-gmon-pie.c new file mode 100644 index 0000000000..1eef2583b6 --- /dev/null +++ b/gmon/tst-gmon-pie.c @@ -0,0 +1 @@ +#include "tst-gmon.c"