Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Sun, Jun 30, 2024 at 07:38:44PM +0530, Aditya Gupta wrote:
> Hi Arnaldo,
> 
> 
> On 29/06/24 00:58, Arnaldo Carvalho de Melo wrote:
> > On Fri, Jun 28, 2024 at 11:28:14AM -0700, Namhyung Kim wrote:
> > > On Fri, Jun 28, 2024 at 11:24:53AM -0300, Arnaldo Carvalho de Melo wrote:
> > > 
> > > > And while looking at it:
> > > > 
> > > >               get_cpuid: [ on  ]  # HAVE_AUXTRACE_SUPPORT
> > > > 
> > > > This looks wrong, no? Or at least confusing, have to check the source
> > > > code...
> > > We have this in Makefile.config
> > > 
> > > ifndef NO_AUXTRACE
> > >    ifeq ($(SRCARCH),x86)
> > >      ifeq ($(feature-get_cpuid), 0)
> > >        $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
> > >        NO_AUXTRACE := 1
> > >      endif
> > >    endif
> > The complete sequence is:
> > 
> > ifndef NO_AUXTRACE
> >    ifeq ($(SRCARCH),x86)
> >      ifeq ($(feature-get_cpuid), 0)
> >        $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
> >        NO_AUXTRACE := 1
> >      endif
> >    endif
> >    ifndef NO_AUXTRACE
> >      $(call detected,CONFIG_AUXTRACE)
> >      CFLAGS += -DHAVE_AUXTRACE_SUPPORT
> >      ifeq ($(feature-reallocarray), 0)
> >        CFLAGS += -DCOMPAT_NEED_REALLOCARRAY
> >      endif
> >    endif
> > endif
> > 
> > The most descriptive would be to HAVE_GET_CPUID_SUPPORT and have it used
> > in the source code.
> > 
> > That or have:
> > 
> > diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> > index 44ffde6f8dbe51f3..ae4a686ff4f265be 100644
> > --- a/tools/perf/builtin-check.c
> > +++ b/tools/perf/builtin-check.c
> > @@ -33,7 +33,7 @@ struct feature_status supported_features[] = {
> >   	FEATURE_STATUS("dwarf", HAVE_DWARF_SUPPORT),
> >   	FEATURE_STATUS("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
> >   	FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
> > -	FEATURE_STATUS("get_cpuid", HAVE_AUXTRACE_SUPPORT),
> > +	FEATURE_STATUS("auxtrace", HAVE_AUXTRACE_SUPPORT),
> >   	FEATURE_STATUS("libaudit", HAVE_LIBAUDIT_SUPPORT),
> >   	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
> >   	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),
> 
> 
> Looks better. Went through all instances of 'HAVE_AUXTRACE_SUPPORT', it's
> mostly been used to conditionally define perf_*_auxtrace functions.
> 
> No one seems to depend on the feature 'name' 'get_cpuid'.
> 
> Any comments ?

So I think its better to go with HAVE_AUXTRACE_SUPPORT to cover this
feature, i.e. whatever is needed to have auxtrace support gets checked
and if all is in place, HAVE_AUXTRACE_SUPPORT is set up.

This in fact is what is being done, its just when reporting using
supported_features[] that we end up using the string "get_cpuid", right?

So it makes sense to use:

FEATURE_STATUS("auxtrace", HAVE_AUXTRACE_SUPPORT),
 
> > That:
> > 
> >          FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
> > 
> > Should also really be:
> > 
> >          FEATURE_STATUS("dwarf-unwind", HAVE_DWARF_UNWIND_SUPPORT),
> 
> Will do it in v13.

Thanks, put that in a separate patch, as it is only barely related to
the other patches, we only are correcting an inconsistency we found
while writing/reviewing your patch kit.

Thanks,

- Arnaldo
 
> 
> Thanks,
> 
> Aditya Gupta
> 
> > For consistency, the get_cpuid/auxtrace also for consistency, I think.
> > 
> > - Arnaldo