Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term

Mathieu Poirier
2020-01-09T16:34:30Z

On Thu, Jan 09, 2020 at 01:08:06PM +0800, Leo Yan wrote:
> Hi Mathieu,
>
> On Wed, Jan 08, 2020 at 10:58:31AM -0700, Mathieu Poirier wrote:
>
> [...]
>
> > > diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> > > index 1f8d2fe0b66e..4e5b3ebf09cf 100644
> > > --- a/tools/perf/util/evsel_config.h
> > > +++ b/tools/perf/util/evsel_config.h
> > > @@ -33,21 +33,8 @@ struct perf_evsel_config_term {
> > > struct list_head list;
> > > enum evsel_term_type type;
> > > union {
> > > - u64 period;
> > > - u64 freq;
> > > - bool time;
> > > - char *callgraph;
> > > - char *drv_cfg;
> > > - u64 stack_user;
> > > - int max_stack;
> > > - bool inherit;
> > > - bool overwrite;
> > > - char *branch;
> > > - unsigned long max_events;
> > > - bool percore;
> > > - bool aux_output;
> > > - u32 aux_sample_size;
> > > - u64 cfg_chg;
> > > + u64 num;
> > > + char *str;
> >
> > That is a lot more than just dealing with the "char *" members. Given
> > the pervasiveness of the changes I would have been happy to leave
> > other members alone for the time being.
>
> I think actually you are suggesting like below which add general
> members and also keep the old members. If so, I prefer to add two
> general members 'num' and 'str'.

If we are to deal with all flields of the union, I think it should be as below:

union {
bool cfg_bool;
int cfg_int;
unsigned long cfg_ulong;
u32 cfg_u32;
char *cfg_str;
} val;

But just dealing with the "char *" as below would also be fine with me:

union {
u64 period;
u64 freq;
bool time;
u64 stack_user;
int max_stack;
bool inherit;
bool overwrite;
unsigned long max_events;
bool percore;
bool aux_output;
u32 aux_sample_size;
u64 cfg_chg;
u64 num;
char *str;
} val;

>
> struct perf_evsel_config_term {
> struct list_head list;
> enum evsel_term_type type;
> union {
> u64 period;
> u64 freq;
> bool time;
> char *callgraph;
> char *drv_cfg;
> u64 stack_user;
> int max_stack;
> bool inherit;
> bool overwrite;
> char *branch;
> unsigned long max_events;
> bool percore;
> bool aux_output;
> u32 aux_sample_size;
> u64 cfg_chg;
> + u64 num;
> + char *str;
> } val;
> bool weak;
> };
>
> > I will let Jiri make the
> > final call but if we are to proceed this way I think we should have a
> > member per type to avoid casting issues.
>
> Yeah, let's see what's Jiri thinking.
>
> Just note, with this change, I don't see any casting warning or errors
> when built perf on arm64/arm32.

At this time you may not, but they will happen and it will be very hard to
debug.

>
> Thanks,
> Leo Yan