| Message ID | 20231005230851.3666908-7-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clang-tools support in tools | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote: > > The buildid filename is first determined and then from this the > buildid read. If getting the filename fails then the buildid will be > used for a later memcmp uninitialized. Detected by clang-tidy. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/builtin-buildid-cache.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c > index cd381693658b..e2a40f1d9225 100644 > --- a/tools/perf/builtin-buildid-cache.c > +++ b/tools/perf/builtin-buildid-cache.c > @@ -277,8 +277,10 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused) > char filename[PATH_MAX]; > struct build_id bid; > > - if (dso__build_id_filename(dso, filename, sizeof(filename), false) && > - filename__read_build_id(filename, &bid) == -1) { > + if (!dso__build_id_filename(dso, filename, sizeof(filename), false)) > + return true; This won't print anything and ignore the file which changes the existing behavior. But if it fails to read the build-id, I don't think there is not much we can do with it. IIUC the original intention of -M/--missing option is to list files that have a build-id but it's not in the build-id cache. So maybe it's ok to silently ignore it. Thanks, Namhyung > + > + if (filename__read_build_id(filename, &bid) == -1) { > if (errno == ENOENT) > return false; > > -- > 2.42.0.609.gbb76f46606-goog >
On Sun, Oct 8, 2023 at 11:06 PM Namhyung Kim <namhyung@kernel.org> wrote: > > On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote: > > > > The buildid filename is first determined and then from this the > > buildid read. If getting the filename fails then the buildid will be > > used for a later memcmp uninitialized. Detected by clang-tidy. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/builtin-buildid-cache.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c > > index cd381693658b..e2a40f1d9225 100644 > > --- a/tools/perf/builtin-buildid-cache.c > > +++ b/tools/perf/builtin-buildid-cache.c > > @@ -277,8 +277,10 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused) > > char filename[PATH_MAX]; > > struct build_id bid; > > > > - if (dso__build_id_filename(dso, filename, sizeof(filename), false) && > > - filename__read_build_id(filename, &bid) == -1) { > > + if (!dso__build_id_filename(dso, filename, sizeof(filename), false)) > > + return true; > > This won't print anything and ignore the file which changes > the existing behavior. But if it fails to read the build-id, I > don't think there is not much we can do with it. IIUC the > original intention of -M/--missing option is to list files that > have a build-id but it's not in the build-id cache. So maybe > it's ok to silently ignore it. If getting the build id filename fails then 'bid' is uninitialized and I don't think there is an expected behavior for what a memcmp on uninitialized memory should do - we may hope that it fails and get the pr_warning in the existing code, but that warning depends on reading the filename too. This was the smallest change to not change behavior but to avoid the undefined behavior (bugs) in the code. It could be a signal the code needs to be worked on more. Thanks, Ian > Thanks, > Namhyung > > > > + > > + if (filename__read_build_id(filename, &bid) == -1) { > > if (errno == ENOENT) > > return false; > > > > -- > > 2.42.0.609.gbb76f46606-goog > >
diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c index cd381693658b..e2a40f1d9225 100644 --- a/tools/perf/builtin-buildid-cache.c +++ b/tools/perf/builtin-buildid-cache.c @@ -277,8 +277,10 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused) char filename[PATH_MAX]; struct build_id bid; - if (dso__build_id_filename(dso, filename, sizeof(filename), false) && - filename__read_build_id(filename, &bid) == -1) { + if (!dso__build_id_filename(dso, filename, sizeof(filename), false)) + return true; + + if (filename__read_build_id(filename, &bid) == -1) { if (errno == ENOENT) return false;
The buildid filename is first determined and then from this the buildid read. If getting the filename fails then the buildid will be used for a later memcmp uninitialized. Detected by clang-tidy. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/builtin-buildid-cache.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)