| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 7 Jul 2022 11:33:19 -0700
From: Raul Silvera <rsilvera@...mo.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
linux-perf-users <linux-perf-users@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] perf inject: Add a command line option to specify build ids.
Thank you. Will apply these suggestions and send an updated version shortly.
Raúl E. Silvera
Software Engineer
waymo.com
Raúl E. Silvera
Software Engineer
waymo.com
On Wed, Jul 6, 2022 at 2:27 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> Hi Raul,
>
> On Wed, Jun 29, 2022 at 5:18 PM Raul Silvera <rsilvera@...gle.com> wrote:
> >
> > This commit adds the option --known-build-ids to perf inject.
> > It allows the user to explicitly specify the build id for a given
> > path, instead of retrieving it from the current system. This is
> > useful in cases where a perf.data file is processed on a different
> > system from where it was collected, or if some of the binaries are
> > no longer available.
> >
> > The build ids and paths are specified in pairs in the command line.
> > Using the file:// specifier, build ids can be loaded from a file
> > directly generated by perf buildid-list. This is convenient to copy
> > build ids from one perf.data file to another.
> >
> > ** Example: In this example we use perf record to create two
> > perf.data files, one with build ids and another without, and use
> > perf buildid-list and perf inject to copy the build ids from the
> > first file to the second.
> >
> > $ perf record ls /tmp # Create perf.data file
> > $ perf record --no-buildid ls /tmp -o perf.data.no-buildid
> > $ perf buildid-list > /tmp/build-ids.txt
> > $ perf inject -b --known-build-ids='file:///tmp/build-ids.txt' \
> > $ -i perf.data.no-buildid -o perf.data.buildid
>
> You'd better indent the block for the readability and to prevent
> possible interference from the shell (for comments usually).
>
> The last line is a continuation from the above line and it used
> to print other prompts like ">".
>
> >
> > Signed-off-by: Raul Silvera <rsilvera@...gle.com>
> > ---
> > tools/perf/builtin-inject.c | 57 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > index a75bf11585b5..667b942f870e 100644
> > --- a/tools/perf/builtin-inject.c
> > +++ b/tools/perf/builtin-inject.c
> > @@ -21,6 +21,7 @@
> > #include "util/data.h"
> > #include "util/auxtrace.h"
> > #include "util/jit.h"
> > +#include "util/string2.h"
> > #include "util/symbol.h"
> > #include "util/synthetic-events.h"
> > #include "util/thread.h"
> > @@ -35,6 +36,7 @@
> >
> > #include <linux/list.h>
> > #include <linux/string.h>
> > +#include <ctype.h>
> > #include <errno.h>
> > #include <signal.h>
> >
> > @@ -59,6 +61,8 @@ struct perf_inject {
> > struct itrace_synth_opts itrace_synth_opts;
> > char event_copy[PERF_SAMPLE_MAX_SIZE];
> > struct perf_file_section secs[HEADER_FEAT_BITS];
> > + const char *known_build_ids_source;
> > + struct strlist *known_build_ids;
> > };
> >
> > struct event_entry {
> > @@ -570,9 +574,43 @@ static int dso__read_build_id(struct dso *dso)
> > return dso->has_build_id ? 0 : -1;
> > }
> >
> > +static bool perf_inject__lookup_known_build_id(struct perf_inject *inject,
> > + struct dso *dso)
> > +{
> > + struct str_node *pos;
> > + int bid_len;
> > +
> > + strlist__for_each_entry(pos, inject->known_build_ids) {
> > + const char *space;
> > +
> > + pos->s = skip_spaces(pos->s);
> > + space = strstr(pos->s, " ");
> > + if (space == NULL ||
> > + !strcmp(dso->long_name, skip_spaces(space)))
> > + continue;
> > + bid_len = space - pos->s;
> > + if (bid_len == 0 || bid_len / 2 > BUILD_ID_SIZE)
>
> I'm not sure how bid_len can be 0. And you can compare to
> SBUILD_ID_SIZE (without dividing by 2) - but it'd be ">=".
> Also it could be worthwhile to check if bid_len is even.
>
>
> > + return false;
> > + for (int ix = 0; 2 * ix + 1 < bid_len; ++ix) {
> > + if (!isxdigit(pos->s[2 * ix]) ||
> > + !isxdigit(pos->s[2 * ix + 1]))
> > + return false;
> > +
> > + dso->bid.data[ix] = (hex(pos->s[2 * ix]) << 4 |
> > + hex(pos->s[2 * ix + 1]));
> > + }
> > + dso->bid.size = bid_len / 2;
> > + dso->has_build_id = 1;
> > + return true;
> > + }
> > + return false;
> > +}
> > +
> > static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
> > struct machine *machine, u8 cpumode, u32 flags)
> > {
> > + struct perf_inject *inject = container_of(tool, struct perf_inject,
> > + tool);
> > int err;
> >
> > if (is_anon_memory(dso->long_name) || flags & MAP_HUGETLB)
> > @@ -580,6 +618,10 @@ static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
> > if (is_no_dso_memory(dso->long_name))
> > return 0;
> >
> > + if (inject->known_build_ids != NULL &&
> > + perf_inject__lookup_known_build_id(inject, dso))
> > + return 1;
> > +
> > if (dso__read_build_id(dso) < 0) {
> > pr_debug("no build_id found for %s\n", dso->long_name);
> > return -1;
> > @@ -1082,6 +1124,9 @@ int cmd_inject(int argc, const char **argv)
> > "Inject build-ids into the output stream"),
> > OPT_BOOLEAN(0, "buildid-all", &inject.build_id_all,
> > "Inject build-ids of all DSOs into the output stream"),
> > + OPT_STRING(0, "known-build-ids", &inject.known_build_ids_source,
> > + "buildid path [buildid path...]",
> > + "build-ids to use for specific files"),
> > OPT_STRING('i', "input", &inject.input_name, "file",
> > "input file name"),
> > OPT_STRING('o', "output", &inject.output.path, "file",
> > @@ -1215,6 +1260,18 @@ int cmd_inject(int argc, const char **argv)
> > */
> > inject.tool.ordered_events = true;
> > inject.tool.ordering_requires_timestamps = true;
> > + if (inject.known_build_ids_source != NULL) {
> > + struct strlist *known_build_ids;
> > +
> > + known_build_ids = strlist__new(
> > + inject.known_build_ids_source, NULL);
> > +
> > + if (known_build_ids == NULL) {
> > + pr_err("Couldn't parse known build ids.\n");
> > + goto out_delete;
> > + }
> > + inject.known_build_ids = known_build_ids;
>
> You need to release it after use. Note that strlist__delete()
> accepts a NULL pointer.
>
> Thanks,
> Namhyung
>
>
> > + }
> > }
> >
> > if (inject.sched_stat) {
> > --
> > 2.37.0.rc0.161.g10f37bed90-goog
> >
Powered by blists - more mailing lists