[PATCH 7/9] perf tools: Add size to struct perf_record_header_build_id
We do not store size with build ids in perf data, but there's enough space to do it. Adding misc bit PERF_RECORD_MISC_BUILD_ID_SIZE to mark build id event with size. With this fix the dso with md5 build id will have correct build id data and will be usable for debuginfod processing if needed (coming in following patches). Acked-by: Ian Rogers Signed-off-by: Jiri Olsa --- tools/lib/perf/include/perf/event.h | 12 +++- tools/perf/util/build-id.c | 8 +--- tools/perf/util/header.c| 10 +++--- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h index a6dbba6b9073..988c539bedb6 100644 --- a/tools/lib/perf/include/perf/event.h +++ b/tools/lib/perf/include/perf/event.h @@ -201,10 +201,20 @@ struct perf_record_header_tracing_data { __u32size; }; +#define PERF_RECORD_MISC_BUILD_ID_SIZE (1 << 15) + struct perf_record_header_build_id { struct perf_event_header header; pid_tpid; - __u8 build_id[24]; + union { + __u8 build_id[24]; + struct { + __u8 data[20]; + __u8 size; + __u8 reserved1__; + __u16reserved2__; + }; + }; char filename[]; }; diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c index b5648735f01f..8763772f1095 100644 --- a/tools/perf/util/build-id.c +++ b/tools/perf/util/build-id.c @@ -296,7 +296,7 @@ char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size, continue; \ else -static int write_buildid(const char *name, size_t name_len, u8 *build_id, +static int write_buildid(const char *name, size_t name_len, struct build_id *bid, pid_t pid, u16 misc, struct feat_fd *fd) { int err; @@ -307,7 +307,9 @@ static int write_buildid(const char *name, size_t name_len, u8 *build_id, len = PERF_ALIGN(len, NAME_ALIGN); memset(&b, 0, sizeof(b)); - memcpy(&b.build_id, build_id, BUILD_ID_SIZE); + memcpy(&b.data, bid->data, bid->size); + b.size = (u8) bid->size; + misc |= PERF_RECORD_MISC_BUILD_ID_SIZE; b.pid = pid; b.header.misc = misc; b.header.size = sizeof(b) + len; @@ -354,7 +356,7 @@ static int machine__write_buildid_table(struct machine *machine, in_kernel = pos->kernel || is_kernel_module(name, PERF_RECORD_MISC_CPUMODE_UNKNOWN); - err = write_buildid(name, name_len, pos->bid.data, machine->pid, + err = write_buildid(name, name_len, &pos->bid, machine->pid, in_kernel ? kmisc : umisc, fd); if (err) break; diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c index 21243adbb9fd..8da3886f10a8 100644 --- a/tools/perf/util/header.c +++ b/tools/perf/util/header.c @@ -2083,8 +2083,12 @@ static int __event_process_build_id(struct perf_record_header_build_id *bev, if (dso != NULL) { char sbuild_id[SBUILD_ID_SIZE]; struct build_id bid; + size_t size = BUILD_ID_SIZE; - build_id__init(&bid, bev->build_id, BUILD_ID_SIZE); + if (bev->header.misc & PERF_RECORD_MISC_BUILD_ID_SIZE) + size = bev->size; + + build_id__init(&bid, bev->data, size); dso__set_build_id(dso, &bid); if (dso_space != DSO_SPACE__USER) { @@ -2098,8 +2102,8 @@ static int __event_process_build_id(struct perf_record_header_build_id *bev, } build_id__sprintf(&dso->bid, sbuild_id); - pr_debug("build id event received for %s: %s\n", -dso->long_name, sbuild_id); + pr_debug("build id event received for %s: %s [%lu]\n", +dso->long_name, sbuild_id, size); dso__put(dso); } -- 2.26.2
Re: [PATCH 7/9] perf tools: Add size to struct perf_record_header_build_id
Em Tue, Oct 13, 2020 at 09:24:39PM +0200, Jiri Olsa escreveu: > We do not store size with build ids in perf data, > but there's enough space to do it. Adding misc bit > PERF_RECORD_MISC_BUILD_ID_SIZE to mark build id event > with size. > > With this fix the dso with md5 build id will have correct > build id data and will be usable for debuginfod processing > if needed (coming in following patches). > > Acked-by: Ian Rogers > Signed-off-by: Jiri Olsa > --- > tools/lib/perf/include/perf/event.h | 12 +++- > tools/perf/util/build-id.c | 8 +--- > tools/perf/util/header.c| 10 +++--- > 3 files changed, 23 insertions(+), 7 deletions(-) > > diff --git a/tools/lib/perf/include/perf/event.h > b/tools/lib/perf/include/perf/event.h > index a6dbba6b9073..988c539bedb6 100644 > --- a/tools/lib/perf/include/perf/event.h > +++ b/tools/lib/perf/include/perf/event.h > @@ -201,10 +201,20 @@ struct perf_record_header_tracing_data { > __u32size; > }; > > +#define PERF_RECORD_MISC_BUILD_ID_SIZE (1 << 15) > + > struct perf_record_header_build_id { > struct perf_event_header header; > pid_tpid; > - __u8 build_id[24]; > + union { > + __u8 build_id[24]; > + struct { > + __u8 data[20]; > + __u8 size; > + __u8 reserved1__; > + __u16reserved2__; > + }; > + }; > char filename[]; > }; Hey, shouldn't we just append the extra info at the end, i.e. keep it like: struct perf_record_header_build_id { struct perf_event_header header; pid_tpid; __u8 build_id[24]; char filename[]; __u8 size; }; No need for PERF_RECORD_MISC_BUILD_ID_SIZE, older tools will continue working with new perf data files. OTOH BUILD_ID_SIZE is 20 and the space on this header is 24, so the last 4 bytes were not being used, so older tools don't look into it, they should continue working, have you tested this case? I.e. getting the perf binary in, say, fedora and check that it works with this new perf_record_header_build_id layout? - Arnaldo > diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c > index b5648735f01f..8763772f1095 100644 > --- a/tools/perf/util/build-id.c > +++ b/tools/perf/util/build-id.c > @@ -296,7 +296,7 @@ char *dso__build_id_filename(const struct dso *dso, char > *bf, size_t size, > continue; \ > else > > -static int write_buildid(const char *name, size_t name_len, u8 *build_id, > +static int write_buildid(const char *name, size_t name_len, struct build_id > *bid, >pid_t pid, u16 misc, struct feat_fd *fd) > { > int err; > @@ -307,7 +307,9 @@ static int write_buildid(const char *name, size_t > name_len, u8 *build_id, > len = PERF_ALIGN(len, NAME_ALIGN); > > memset(&b, 0, sizeof(b)); > - memcpy(&b.build_id, build_id, BUILD_ID_SIZE); > + memcpy(&b.data, bid->data, bid->size); > + b.size = (u8) bid->size; > + misc |= PERF_RECORD_MISC_BUILD_ID_SIZE; > b.pid = pid; > b.header.misc = misc; > b.header.size = sizeof(b) + len; > @@ -354,7 +356,7 @@ static int machine__write_buildid_table(struct machine > *machine, > in_kernel = pos->kernel || > is_kernel_module(name, > PERF_RECORD_MISC_CPUMODE_UNKNOWN); > - err = write_buildid(name, name_len, pos->bid.data, machine->pid, > + err = write_buildid(name, name_len, &pos->bid, machine->pid, > in_kernel ? kmisc : umisc, fd); > if (err) > break; > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c > index 21243adbb9fd..8da3886f10a8 100644 > --- a/tools/perf/util/header.c > +++ b/tools/perf/util/header.c > @@ -2083,8 +2083,12 @@ static int __event_process_build_id(struct > perf_record_header_build_id *bev, > if (dso != NULL) { > char sbuild_id[SBUILD_ID_SIZE]; > struct build_id bid; > + size_t size = BUILD_ID_SIZE; > > - build_id__init(&bid, bev->build_id, BUILD_ID_SIZE); > + if (bev->header.misc & PERF_RECORD_MISC_BUILD_ID_SIZE) > + size = bev->size; > + > + build_id__init(&bid, bev->data, size); > dso__set_build_id(dso, &bid); > > if (dso_space != DSO_SPACE__USER) { > @@ -2098,8 +2102,8 @@ static int __event_process_build_id(struct > perf_record_header_build_id *bev, > } > > build_id__sprintf(&dso->bid, sbuild_id); > - pr_debug("build id event received for %s: %s\n", > -
Re: [PATCH 7/9] perf tools: Add size to struct perf_record_header_build_id
On Wed, Oct 14, 2020 at 08:59:08AM -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Oct 13, 2020 at 09:24:39PM +0200, Jiri Olsa escreveu: > > We do not store size with build ids in perf data, > > but there's enough space to do it. Adding misc bit > > PERF_RECORD_MISC_BUILD_ID_SIZE to mark build id event > > with size. > > > > With this fix the dso with md5 build id will have correct > > build id data and will be usable for debuginfod processing > > if needed (coming in following patches). > > > > Acked-by: Ian Rogers > > Signed-off-by: Jiri Olsa > > --- > > tools/lib/perf/include/perf/event.h | 12 +++- > > tools/perf/util/build-id.c | 8 +--- > > tools/perf/util/header.c| 10 +++--- > > 3 files changed, 23 insertions(+), 7 deletions(-) > > > > diff --git a/tools/lib/perf/include/perf/event.h > > b/tools/lib/perf/include/perf/event.h > > index a6dbba6b9073..988c539bedb6 100644 > > --- a/tools/lib/perf/include/perf/event.h > > +++ b/tools/lib/perf/include/perf/event.h > > @@ -201,10 +201,20 @@ struct perf_record_header_tracing_data { > > __u32size; > > }; > > > > +#define PERF_RECORD_MISC_BUILD_ID_SIZE (1 << 15) > > + > > struct perf_record_header_build_id { > > struct perf_event_header header; > > pid_tpid; > > - __u8 build_id[24]; > > + union { > > + __u8 build_id[24]; > > + struct { > > + __u8 data[20]; > > + __u8 size; > > + __u8 reserved1__; > > + __u16reserved2__; > > + }; > > + }; > > char filename[]; > > }; > > Hey, shouldn't we just append the extra info at the end, i.e. keep it > like: > > struct perf_record_header_build_id { > struct perf_event_header header; > pid_tpid; > __u8 build_id[24]; > char filename[]; > __u8 size; > }; > > > No need for PERF_RECORD_MISC_BUILD_ID_SIZE, older tools will continue > working with new perf data files. hum, then how would we tell if the last byte (size) is present or not? > > OTOH BUILD_ID_SIZE is 20 and the space on this header is 24, so the last > 4 bytes were not being used, so older tools don't look into it, they > should continue working, have you tested this case? I.e. getting the > perf binary in, say, fedora and check that it works with this new > perf_record_header_build_id layout? yes, that still works (tested), because we copied only 20 bytes of the build_id[24] and did not care about the rest jirka
Re: [PATCH 7/9] perf tools: Add size to struct perf_record_header_build_id
Em Tue, Oct 13, 2020 at 09:24:39PM +0200, Jiri Olsa escreveu: > We do not store size with build ids in perf data, > but there's enough space to do it. Adding misc bit > PERF_RECORD_MISC_BUILD_ID_SIZE to mark build id event > with size. > > With this fix the dso with md5 build id will have correct > build id data and will be usable for debuginfod processing > if needed (coming in following patches). > > Acked-by: Ian Rogers > Signed-off-by: Jiri Olsa > --- > tools/lib/perf/include/perf/event.h | 12 +++- > tools/perf/util/build-id.c | 8 +--- > tools/perf/util/header.c| 10 +++--- > 3 files changed, 23 insertions(+), 7 deletions(-) > > diff --git a/tools/lib/perf/include/perf/event.h > b/tools/lib/perf/include/perf/event.h > index a6dbba6b9073..988c539bedb6 100644 > --- a/tools/lib/perf/include/perf/event.h > +++ b/tools/lib/perf/include/perf/event.h > @@ -201,10 +201,20 @@ struct perf_record_header_tracing_data { > __u32size; > }; > > +#define PERF_RECORD_MISC_BUILD_ID_SIZE (1 << 15) > + > struct perf_record_header_build_id { > struct perf_event_header header; > pid_tpid; > - __u8 build_id[24]; > + union { > + __u8 build_id[24]; > + struct { > + __u8 data[20]; > + __u8 size; > + __u8 reserved1__; > + __u16reserved2__; > + }; > + }; > char filename[]; > }; > > diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c > index b5648735f01f..8763772f1095 100644 > --- a/tools/perf/util/build-id.c > +++ b/tools/perf/util/build-id.c > @@ -296,7 +296,7 @@ char *dso__build_id_filename(const struct dso *dso, char > *bf, size_t size, > continue; \ > else > > -static int write_buildid(const char *name, size_t name_len, u8 *build_id, > +static int write_buildid(const char *name, size_t name_len, struct build_id > *bid, >pid_t pid, u16 misc, struct feat_fd *fd) > { > int err; > @@ -307,7 +307,9 @@ static int write_buildid(const char *name, size_t > name_len, u8 *build_id, > len = PERF_ALIGN(len, NAME_ALIGN); > > memset(&b, 0, sizeof(b)); > - memcpy(&b.build_id, build_id, BUILD_ID_SIZE); > + memcpy(&b.data, bid->data, bid->size); > + b.size = (u8) bid->size; > + misc |= PERF_RECORD_MISC_BUILD_ID_SIZE; > b.pid = pid; > b.header.misc = misc; > b.header.size = sizeof(b) + len; > @@ -354,7 +356,7 @@ static int machine__write_buildid_table(struct machine > *machine, > in_kernel = pos->kernel || > is_kernel_module(name, > PERF_RECORD_MISC_CPUMODE_UNKNOWN); > - err = write_buildid(name, name_len, pos->bid.data, machine->pid, > + err = write_buildid(name, name_len, &pos->bid, machine->pid, > in_kernel ? kmisc : umisc, fd); > if (err) > break; > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c > index 21243adbb9fd..8da3886f10a8 100644 > --- a/tools/perf/util/header.c > +++ b/tools/perf/util/header.c > @@ -2083,8 +2083,12 @@ static int __event_process_build_id(struct > perf_record_header_build_id *bev, > if (dso != NULL) { > char sbuild_id[SBUILD_ID_SIZE]; > struct build_id bid; > + size_t size = BUILD_ID_SIZE; > > - build_id__init(&bid, bev->build_id, BUILD_ID_SIZE); > + if (bev->header.misc & PERF_RECORD_MISC_BUILD_ID_SIZE) > + size = bev->size; > + > + build_id__init(&bid, bev->data, size); > dso__set_build_id(dso, &bid); > > if (dso_space != DSO_SPACE__USER) { > @@ -2098,8 +2102,8 @@ static int __event_process_build_id(struct > perf_record_header_build_id *bev, > } > > build_id__sprintf(&dso->bid, sbuild_id); > - pr_debug("build id event received for %s: %s\n", > - dso->long_name, sbuild_id); > + pr_debug("build id event received for %s: %s [%lu]\n", > + dso->long_name, sbuild_id, size); util/header.c: In function '__event_process_build_id': util/header.c:2105:3: error: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'size_t' [-Werror=format=] pr_debug("build id event received for %s: %s [%lu]\n", ^ Fixing this to '%zd' - Arnaldo > dso__put(dso); > } > > -- > 2.26.2 > -- - Arnaldo
Re: [PATCH 7/9] perf tools: Add size to struct perf_record_header_build_id
Em Wed, Oct 14, 2020 at 03:21:46PM +0200, Jiri Olsa escreveu: > On Wed, Oct 14, 2020 at 08:59:08AM -0300, Arnaldo Carvalho de Melo wrote: > > Em Tue, Oct 13, 2020 at 09:24:39PM +0200, Jiri Olsa escreveu: > > > We do not store size with build ids in perf data, > > > but there's enough space to do it. Adding misc bit > > > PERF_RECORD_MISC_BUILD_ID_SIZE to mark build id event > > > with size. > > > > > > With this fix the dso with md5 build id will have correct > > > build id data and will be usable for debuginfod processing > > > if needed (coming in following patches). > > > > > > Acked-by: Ian Rogers > > > Signed-off-by: Jiri Olsa > > > --- > > > tools/lib/perf/include/perf/event.h | 12 +++- > > > tools/perf/util/build-id.c | 8 +--- > > > tools/perf/util/header.c| 10 +++--- > > > 3 files changed, 23 insertions(+), 7 deletions(-) > > > > > > diff --git a/tools/lib/perf/include/perf/event.h > > > b/tools/lib/perf/include/perf/event.h > > > index a6dbba6b9073..988c539bedb6 100644 > > > --- a/tools/lib/perf/include/perf/event.h > > > +++ b/tools/lib/perf/include/perf/event.h > > > @@ -201,10 +201,20 @@ struct perf_record_header_tracing_data { > > > __u32size; > > > }; > > > > > > +#define PERF_RECORD_MISC_BUILD_ID_SIZE (1 << 15) > > > + > > > struct perf_record_header_build_id { > > > struct perf_event_header header; > > > pid_tpid; > > > - __u8 build_id[24]; > > > + union { > > > + __u8 build_id[24]; > > > + struct { > > > + __u8 data[20]; > > > + __u8 size; > > > + __u8 reserved1__; > > > + __u16reserved2__; > > > + }; > > > + }; > > > char filename[]; > > > }; > > Hey, shouldn't we just append the extra info at the end, i.e. keep it > > like: > > struct perf_record_header_build_id { > > struct perf_event_header header; > > pid_tpid; > > __u8 build_id[24]; > > char filename[]; > > __u8 size; > > }; > > No need for PERF_RECORD_MISC_BUILD_ID_SIZE, older tools will continue > > working with new perf data files. > hum, then how would we tell if the last byte (size) is present or not? IT would be different than '\0' ;-) - Arnaldo > > > > OTOH BUILD_ID_SIZE is 20 and the space on this header is 24, so the last > > 4 bytes were not being used, so older tools don't look into it, they > > should continue working, have you tested this case? I.e. getting the > > perf binary in, say, fedora and check that it works with this new > > perf_record_header_build_id layout? > > yes, that still works (tested), because we copied only 20 bytes > of the build_id[24] and did not care about the rest Great that you actually tested it. - Arnaldo