Messages in this thread | | | From | Ian Rogers <> | Date | Tue, 23 May 2023 10:04:15 -0700 | Subject | Re: [PATCH v1 2/2] perf evsel: for_each_group fixes |
| |
On Tue, May 23, 2023 at 7:33 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 23/05/23 07:44, Ian Rogers wrote: > > Address/memory sanitizer was reporting issues in evsel__group_pmu_name > > because the for_each_group_evsel loop didn't terminate when the head > > was reached, the head would then be cast and accessed as an evsel > > leading to invalid memory accesses. Fix for_each_group_member and > > for_each_group_evsel to terminate at the list head. Note, > > evsel__group_pmu_name no longer iterates the group, but the problem is > > present regardless. > > > > Fixes: 717e263fc354 ("perf report: Show group description when event group is enabled") > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/evsel.h | 24 ++++++++++++++++-------- > > tools/perf/util/evsel_fprintf.c | 1 + > > 2 files changed, 17 insertions(+), 8 deletions(-) > > > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > > index 820771a649b2..6a64543c7612 100644 > > --- a/tools/perf/util/evsel.h > > +++ b/tools/perf/util/evsel.h > > @@ -462,16 +462,24 @@ static inline int evsel__group_idx(struct evsel *evsel) > > } > > > > /* Iterates group WITHOUT the leader. */ > > -#define for_each_group_member(_evsel, _leader) \ > > -for ((_evsel) = list_entry((_leader)->core.node.next, struct evsel, core.node); \ > > - (_evsel) && (_evsel)->core.leader == (&_leader->core); \ > > - (_evsel) = list_entry((_evsel)->core.node.next, struct evsel, core.node)) > > +#define for_each_group_member_head(_evsel, _leader, _head) \ > > +for ((_evsel) = list_entry((_leader)->core.node.next, struct evsel, core.node); \ > > + (_evsel) && (&(_evsel)->core.node != (_head)) && \ > > Extra parentheses perhaps not needed e.g. just > > &(_evsel)->core.node != (_head) > > > + (_evsel)->core.leader == (&_leader->core); \ > > Parentheses look odd, maybe should be: > > &(_leader)->core > > > + (_evsel) = list_entry((_evsel)->core.node.next, struct evsel, core.node)) > > + > > +#define for_each_group_member(_evsel, _leader) \ > > + for_each_group_member_head(_evsel, _leader, &(_leader)->evlist->core.entries) > > Did you consider using (_leader)->core.nr_members so that it is not > necessary to assume the evlist back pointer is populated.
Thanks! I'll address the other comments in v2. Wrt nr_members/evsel->evlist, I think we can use it to avoid the whole list scan in evsel__compute_group_pmu_name. We could use it here but in all the non-parse_events__sort_events_and_fix_groups cases we have the evsel->evlist and testing for the list head seems more consistent with other list_for_each cases.
Thanks, Ian
> > > > /* Iterates group WITH the leader. */ > > -#define for_each_group_evsel(_evsel, _leader) \ > > -for ((_evsel) = _leader; \ > > - (_evsel) && (_evsel)->core.leader == (&_leader->core); \ > > - (_evsel) = list_entry((_evsel)->core.node.next, struct evsel, core.node)) > > +#define for_each_group_evsel_head(_evsel, _leader, _head) \ > > +for ((_evsel) = _leader; \ > > + (_evsel) && (&(_evsel)->core.node != (_head)) && \ > > + (_evsel)->core.leader == (&_leader->core); \ > > + (_evsel) = list_entry((_evsel)->core.node.next, struct evsel, core.node)) > > + > > +#define for_each_group_evsel(_evsel, _leader) \ > > + for_each_group_evsel_head(_evsel, _leader, &(_leader)->evlist->core.entries) > > > > static inline bool evsel__has_branch_callstack(const struct evsel *evsel) > > { > > diff --git a/tools/perf/util/evsel_fprintf.c b/tools/perf/util/evsel_fprintf.c > > index cc80ec554c0a..036a2171dc1c 100644 > > --- a/tools/perf/util/evsel_fprintf.c > > +++ b/tools/perf/util/evsel_fprintf.c > > @@ -2,6 +2,7 @@ > > #include <inttypes.h> > > #include <stdio.h> > > #include <stdbool.h> > > +#include "util/evlist.h" > > #include "evsel.h" > > #include "util/evsel_fprintf.h" > > #include "util/event.h" >
| |