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 <[email protected]> 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