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

[Aditya Gupta]

Hello Namhyung,
Sorry for the late reply, was busy last week.

On Thu, Sep 07, 2023 at 02:31:29PM -0700, Namhyung Kim wrote:
> Hello,
> 
> On Sun, Sep 3, 2023 at 4:47 AM Aditya Gupta <adityag@xxxxxxxxxxxxx> wrote:
> >
> > ...
> >
> > +/**
> > + * check whether "feature" is built-in with perf
> > + * returns:./perf annotate --data-type --type-stat -k vmlinux -d '[kernel.kallsyms]' --objdump=llvm-objdump
> > + *   -1: Feature not known
> > + *    0: Built-in
> > + *    1: NOT Built in
> > + */
> > +static int has_support(const char *feature)
> > +{
> > +       int res = -1;
> > +
> > +       for (int i = 0; supported_features[i].name; ++i) {
> > +               if ((strcmp(feature, supported_features[i].name) == 0) ||
> > +                       (strcmp(feature, supported_features[i].macro) == 0)) {
> > +                       res = supported_features[i].is_builtin;
> > +                       STATUS(supported_features[i]);
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (res == -1) {
> > +               color_fprintf(stdout, PERF_COLOR_RED, "Feature not known: %s", feature);
> > +               return -2;
> 
> return -1 ??  It doesn't match with the comment.
> 

Thanks for pointing this. Yes, it should have returned -1, I made a mistake in
this.

> 
> > +       }
> > +
> > +       return !res;
> > +}
> > +
> > +int cmd_check(int argc, const char **argv)
> > +{
> > +       argc = parse_options(argc, argv, check_options, check_usage,
> > +                            PARSE_OPT_STOP_AT_NON_OPTION);
> > +
> > +       printf("perf check %s\n", perf_version_string);
> > +
> > +       if (check.feature)
> > +               return has_support(check.feature);
> > +
> > +       return 0;
> > +}
> > diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> > index f2ab5bae2150..6683ea6d3b60 100644
> > --- a/tools/perf/builtin.h
> > +++ b/tools/perf/builtin.h
> > @@ -2,6 +2,52 @@
> >  #ifndef BUILTIN_H
> >  #define BUILTIN_H
> >
> > +#include <stddef.h>
> > +#include <linux/compiler.h>
> > +#include <tools/config.h>
> > +
> > +struct feature_support {
> > +       const char *name;
> > +       const char *macro;
> > +       int is_builtin;
> > +};
> > +
> > +#define FEATURE_SUPPORT(name_, macro_) { \
> > +       .name = name_,                       \
> > +       .macro = #macro_,                    \
> > +       .is_builtin = IS_BUILTIN(macro_) }
> > +
> > +static struct feature_support supported_features[] __maybe_unused = {
> 
> Hmm.. do you want it in a header file?
> I'm afraid it'd duplicate the entire array for any .c files that
> include this header.
> 

Hmm.. I agree that it duplicates the array in all those c files.

Should I define 'supported_features' same way in perf.c, and then use
'extern struct feature_support supported_features[]' in builtin.h ?

> 
> > +       FEATURE_SUPPORT("dwarf", HAVE_DWARF_SUPPORT),
> > +       FEATURE_SUPPORT("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
> > +#ifndef HAVE_SYSCALL_TABLE_SUPPORT
> 
> Do we really need this #ifndef?

Not really, will remove this in next version. It was this way in
'perf version --build-options' so I just kept it that way.

Thanks,
Aditya Gupta