Re: [PATCH v3] perf parse: Copy string to perf_evsel_config_term

[Leo Yan]

Hi Mathieu, Jiri,

On Wed, Jan 08, 2020 at 11:22:12AM +0100, Jiri Olsa wrote:
> On Tue, Jan 07, 2020 at 02:45:27PM -0700, Mathieu Poirier wrote:

[...]

> > Many thanks for digging into this and stepping forward to provide a
> > solution - it is much appreciated.

My pleasure!

After think again, we do need to add CoreSight test into perf test
ASAP, thus we can pay attention for the failure at the early time.
This is another topic, let's fistly focus on resolve this issue.

> > > Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
> > > Suggested-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > > ---
> > >  tools/perf/util/evsel.c        |  2 ++
> > >  tools/perf/util/evsel_config.h |  2 ++
> > >  tools/perf/util/parse-events.c | 56 +++++++++++++++++++++-------------
> > >  3 files changed, 39 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index a69e64236120..ab9925cc1aa7 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
> > >
> > >         list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
> > >                 list_del_init(&term->list);
> > > +               if (term->free_str)
> > > +                       free(term->val.str);
> > 
> > This will do the trick but we can definitely do better.
> > 
> > Part of his comments on V2, Jiri hinted that we should move to a
> > common perf_evsel_config_term::str to replace {callgraph, drv_cfg,
> > branch}, something that will work because we have
> > perf_evsel_config_term::type.  That means  functions
> > apply_config_terms() and cs_etm_set_sink_attr() need to be modified
> > but the changes are quite small and well worth for the benefit they'll
> > carry.
> > 
> > With that the above becomes neat and clean.
> 
> I wonder if there was some reason for keeping the variables
> like that for every type and not just one per type as we did
> 'struct parse_events_term'
> 
> if the change is possible, the code would be cleaner, let's see ;-)

Thanks for the suggestion, I hope to do right thing in next spin :)

I tried to only use val.num and val.str in perf_evsel_config_term in
my local code, same with the struct parse_events_term, it does work and
the effort is not big.  Will send out patches soon (will use two
patches, one is for refactoring, another is for fixing regression).

Thanks,
Leo Yan