[PATCH v12 0/4] Introduce perf check subcommand

[PATCH v12 0/4] Introduce perf check subcommand

[Aditya Gupta]

The Problem
===========

Currently the presence of a feature is checked with a combination of
perf version --build-options and greps, such as:

    perf version --build-options | grep " on .* HAVE_FEATURE"

This relies on the output of perf version, and is a common pattern in tests.

Proposed solution
=================

As suggested by contributors in:
https://lore.kernel.org/linux-perf-users/ZMPWk5K63tadmDlU@kernel.org/

Introduce a subcommand "perf check feature", with which
scripts can test for presence of a feature or multiple features, such as:

    perf check feature HAVE_LIBTRACEEVENT  (feature macro)

    or

    perf check feature libtraceevent       (feature name)

    or

    perf check feature LibTraceEvent       (case-insensitive)

    or

    perf check feature libtraceevent,bpf   (multiple features)

The usage of "perf version --build-options | grep" has been replaced in two
tests, with "perf check feature" command

Also, to not duplicate the same feature list at multiple places, a new global
'supported_features' array has been introduced in builtin.h, so both commands
'perf check feature' and 'perf version --build-options' use the same array

'supported_features' feature is an array of 'struct feature_support', which
also has the name of the feature, macro used to test it's presence, and a
is_builtin member, which will be 0 if feature not built-in, and 1 if built-in

Architectures Tested
====================
* x86_64
* ppc64le

Commands ran for testing (Fedora & RHEL):

    sudo dnf install -y libtraceevent-devel
    make clean
    make -j$(nproc)
    ./perf check feature libtraceevent,bpf; echo Return Code: $?
    ./perf check feature libtraceevent; echo Return Code: $?
    
    sudo ./perf test -v "task-analyzer"
    sudo ./perf test -v "probe libc's inet_pton & backtrace it with ping"
    sudo ./perf test -v "Use vfs_getname probe to get syscall args filenames"
    
    sudo dnf remove -y libtraceevent-devel
    make clean
    make NO_LIBTRACEEVENT=1 -j$(nproc)
    ./perf check feature libtraceevent,bpf; echo Return Code: $?
    ./perf check feature libtraceevent; echo Return Code: $?
    
    sudo ./perf test -v "task-analyzer"
    sudo ./perf test -v "probe libc's inet_pton & backtrace it with ping"
    sudo ./perf test -v "Use vfs_getname probe to get syscall args filenames"

Git tree
========

Git tree with this patch series applied for testing:
https://github.com/adi-g15-ibm/linux/tree/perf-check-feature-v12

Changelog
=========

v12
+ patch #1: fix comment to mention argv[0] instead of argv[1]
+ patch #2: fix alignment

v11
+ patch #1: fix build error due to const *const instead of const*

v10
+ patch #1: use 'strdup' instead of 'malloc+memcpy'
+ patch #1: replace '-q' with '--quiet' in doc
+ patch #1: add usage for perf check

V9
+ make 'feature' a subcommand instead of an option
+ make feature name/macro check case-insensitive
+ rename 'FEATURE_SUPPORT' as 'FEATURE_STATUS'
+ rebase on upstream perf-tools-next

V8
+ handle return value of 'malloc' in patch #1
+ fix error due to strncpy depending on source string's length

V7
+ modified patch #1 to fix compile issue, and add feature to allow
  multiple comma-separated features

V6
+ rebased to perf-tools-next/perf-tools-next
+ modified patch #1 to include FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL)

V5
+ invert return value of 'has_support', but return value of perf check --feature
according to shell convention

V4
+ invert return value of perf check --feature

V3
+ simplified has_support code in builtin-check.c (patch #1)
+ modified patch #3 and patch #4 according to change in return value in patch #1

V2
+ improved the patch series with suggestions from Namhyung
+ fix incorrect return value, added -q option, and moved array definition to
  perf-check.c

V1
+ changed subcommand name to 'perf check --feature'
+ added documentation for perf check
+ support both macro (eg. HAVE_LIBTRACEEVENT), and name (eg. libtraceevent) as
  input to 'perf check --feature'
+ change subject and descriptions of all patch mentioning perf check instead of
  perf build

V0: Previous patch series: https://lore.kernel.org/linux-perf-users/20230825061125.24312-1-adityag@linux.ibm.com/


*** BLURB HERE ***

Aditya Gupta (3):
  perf check: introduce check subcommand
  perf version: update --build-options to use 'supported_features' array
  perf tests task_analyzer: use perf check for libtraceevent support

Athira Rajeev (1):
  tools/perf/tests: Update probe_vfs_getname.sh script to use perf check
    feature

 tools/perf/Build                              |   1 +
 tools/perf/Documentation/perf-check.txt       |  79 ++++++++
 tools/perf/builtin-check.c                    | 178 ++++++++++++++++++
 tools/perf/builtin-version.c                  |  43 +----
 tools/perf/builtin.h                          |  17 ++
 tools/perf/perf.c                             |   1 +
 .../perf/tests/shell/lib/probe_vfs_getname.sh |   4 +-
 .../shell/record+probe_libc_inet_pton.sh      |   5 +-
 .../shell/record+script_probe_vfs_getname.sh  |   5 +-
 tools/perf/tests/shell/test_task_analyzer.sh  |   4 +-
 10 files changed, 296 insertions(+), 41 deletions(-)
 create mode 100644 tools/perf/Documentation/perf-check.txt
 create mode 100644 tools/perf/builtin-check.c

-- 
2.45.2

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

[Aditya Gupta]

Currently the presence of a feature is checked with a combination of
perf version --build-options and greps, such as:

    perf version --build-options | grep " on .* HAVE_FEATURE"

Instead of this, introduce a subcommand "perf check feature", with which
scripts can test for presence of a feature, such as:

    perf check feature HAVE_FEATURE

'perf check feature' command is expected to have exit status of 0 if
feature is built-in, and 1 if it's not built-in or if feature is not known.

Multiple features can also be passed as a comma-separated list, in which
case the exit status will be 1 only if all of the passed features are
built-in. For example, with below command, it will have exit status of 0
only if both libtraceevent and bpf are enabled, else 1 in all other cases

    perf check feature libtraceevent,bpf

The arguments are case-insensitive.
An array 'supported_features' has also been introduced that can be used by
other commands like 'perf version --build-options', so that new features
can be added in one place, with the array

Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/Build                        |   1 +
 tools/perf/Documentation/perf-check.txt |  79 +++++++++++
 tools/perf/builtin-check.c              | 178 ++++++++++++++++++++++++
 tools/perf/builtin.h                    |  17 +++
 tools/perf/perf.c                       |   1 +
 5 files changed, 276 insertions(+)
 create mode 100644 tools/perf/Documentation/perf-check.txt
 create mode 100644 tools/perf/builtin-check.c

diff --git a/tools/perf/Build b/tools/perf/Build
index 1d4957957d75..3e486f7df94b 100644
--- a/tools/perf/Build
+++ b/tools/perf/Build
@@ -1,5 +1,6 @@
 perf-bench-y += builtin-bench.o
 perf-y += builtin-annotate.o
+perf-y += builtin-check.o
 perf-y += builtin-config.o
 perf-y += builtin-diff.o
 perf-y += builtin-evlist.o
diff --git a/tools/perf/Documentation/perf-check.txt b/tools/perf/Documentation/perf-check.txt
new file mode 100644
index 000000000000..88cfd41a76cc
--- /dev/null
+++ b/tools/perf/Documentation/perf-check.txt
@@ -0,0 +1,79 @@
+perf-check(1)
+===============
+
+NAME
+----
+perf-check - check features in perf
+
+SYNOPSIS
+--------
+[verse]
+'perf check' [<options>]
+'perf check' {feature <feature_list>} [<options>]
+
+DESCRIPTION
+-----------
+With no options given, the 'perf check' just prints the perf version
+on the standard output.
+
+If the subcommand 'feature' is used, then status of feature is printed
+on the standard output (unless '-q' is also passed), ie. whether it is
+compiled-in/built-in or not.
+Also, 'perf check feature' returns with exit status 0 if the feature
+is built-in, otherwise returns with exit status 1.
+
+SUBCOMMANDS
+-----------
+
+feature::
+
+        Print whether feature(s) is compiled-in or not, and also returns with an
+        exit status of 0, if passed feature(s) are compiled-in, else 1.
+
+        It expects a feature list as an argument. There can be a single feature
+        name/macro, or multiple features can also be passed as a comma-separated
+        list, in which case the exit status will be 0 only if all of the passed
+        features are compiled-in.
+
+        The feature names/macros are case-insensitive.
+
+        Example Usage:
+                perf check feature libtraceevent
+                perf check feature HAVE_LIBTRACEEVENT
+                perf check feature libtraceevent,bpf
+
+        Supported feature names/macro:
+                aio                     /  HAVE_AIO_SUPPORT
+                bpf                     /  HAVE_LIBBPF_SUPPORT
+                bpf_skeletons           /  HAVE_BPF_SKEL
+                debuginfod              /  HAVE_DEBUGINFOD_SUPPORT
+                dwarf                   /  HAVE_DWARF_SUPPORT
+                dwarf_getlocations      /  HAVE_DWARF_GETLOCATIONS_SUPPORT
+                get_cpuid               /  HAVE_AUXTRACE_SUPPORT
+                numa_num_possible_cpus  /  HAVE_LIBNUMA_SUPPORT
+                libaudit                /  HAVE_LIBAUDIT_SUPPORT
+                libbfd                  /  HAVE_LIBBFD_SUPPORT
+                libelf                  /  HAVE_LIBELF_SUPPORT
+                libcrypto               /  HAVE_LIBCRYPTO_SUPPORT
+                libdw-dwarf-unwind      /  HAVE_DWARF_SUPPORT
+                libnuma                 /  HAVE_LIBNUMA_SUPPORT
+                libperl                 /  HAVE_LIBPERL_SUPPORT
+                libpfm4                 /  HAVE_LIBPFM
+                libpython               /  HAVE_LIBPYTHON_SUPPORT
+                libslang                /  HAVE_SLANG_SUPPORT
+                libtraceevent           /  HAVE_LIBTRACEEVENT
+                libunwind               /  HAVE_LIBUNWIND_SUPPORT
+                lzma                    /  HAVE_LZMA_SUPPORT
+                syscall_table           /  HAVE_SYSCALL_TABLE_SUPPORT
+                zlib                    /  HAVE_ZLIB_SUPPORT
+                zstd                    /  HAVE_ZSTD_SUPPORT
+
+OPTIONS
+-------
+-q::
+--quiet::
+        Do not print any messages or warnings
+
+        This can be used along with subcommands such as 'perf check feature'
+        to hide unnecessary output in test scripts, eg.
+        'perf check feature --quiet libtraceevent'
diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
new file mode 100644
index 000000000000..44ffde6f8dbe
--- /dev/null
+++ b/tools/perf/builtin-check.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "builtin.h"
+#include "color.h"
+#include "util/debug.h"
+#include "util/header.h"
+#include <tools/config.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+#include <subcmd/parse-options.h>
+
+static const char * const check_subcommands[] = { "feature", NULL };
+static struct option check_options[] = {
+	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
+	OPT_END()
+};
+static struct option check_feature_options[] = { OPT_END() };
+
+static const char *check_usage[] = {
+	"perf check [<subcommand>] [<options>]",
+	NULL
+};
+static const char *check_feature_usage[] = {
+	"perf check feature <feature_list>",
+	NULL
+};
+
+struct feature_status supported_features[] = {
+	FEATURE_STATUS("aio", HAVE_AIO_SUPPORT),
+	FEATURE_STATUS("bpf", HAVE_LIBBPF_SUPPORT),
+	FEATURE_STATUS("bpf_skeletons", HAVE_BPF_SKEL),
+	FEATURE_STATUS("debuginfod", HAVE_DEBUGINFOD_SUPPORT),
+	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("libaudit", HAVE_LIBAUDIT_SUPPORT),
+	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
+	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),
+	FEATURE_STATUS("libcrypto", HAVE_LIBCRYPTO_SUPPORT),
+	FEATURE_STATUS("libdw-dwarf-unwind", HAVE_DWARF_SUPPORT),
+	FEATURE_STATUS("libelf", HAVE_LIBELF_SUPPORT),
+	FEATURE_STATUS("libnuma", HAVE_LIBNUMA_SUPPORT),
+	FEATURE_STATUS("libopencsd", HAVE_CSTRACE_SUPPORT),
+	FEATURE_STATUS("libperl", HAVE_LIBPERL_SUPPORT),
+	FEATURE_STATUS("libpfm4", HAVE_LIBPFM),
+	FEATURE_STATUS("libpython", HAVE_LIBPYTHON_SUPPORT),
+	FEATURE_STATUS("libslang", HAVE_SLANG_SUPPORT),
+	FEATURE_STATUS("libtraceevent", HAVE_LIBTRACEEVENT),
+	FEATURE_STATUS("libunwind", HAVE_LIBUNWIND_SUPPORT),
+	FEATURE_STATUS("lzma", HAVE_LZMA_SUPPORT),
+	FEATURE_STATUS("numa_num_possible_cpus", HAVE_LIBNUMA_SUPPORT),
+	FEATURE_STATUS("syscall_table", HAVE_SYSCALL_TABLE_SUPPORT),
+	FEATURE_STATUS("zlib", HAVE_ZLIB_SUPPORT),
+	FEATURE_STATUS("zstd", HAVE_ZSTD_SUPPORT),
+
+	/* this should remain at end, to know the array end */
+	FEATURE_STATUS(NULL, _)
+};
+
+static void on_off_print(const char *status)
+{
+	printf("[ ");
+
+	if (!strcmp(status, "OFF"))
+		color_fprintf(stdout, PERF_COLOR_RED, "%-3s", status);
+	else
+		color_fprintf(stdout, PERF_COLOR_GREEN, "%-3s", status);
+
+	printf(" ]");
+}
+
+/* Helper function to print status of a feature along with name/macro */
+static void status_print(const char *name, const char *macro,
+			 const char *status)
+{
+	printf("%22s: ", name);
+	on_off_print(status);
+	printf("  # %s\n", macro);
+}
+
+#define STATUS(feature)                                           \
+do {                                                              \
+	if (feature.is_builtin)                                   \
+		status_print(feature.name, feature.macro, "on");  \
+	else                                                      \
+		status_print(feature.name, feature.macro, "OFF"); \
+} while (0)
+
+/**
+ * check whether "feature" is built-in with perf
+ *
+ * returns:
+ *    0: NOT built-in or Feature not known
+ *    1: Built-in
+ */
+static int has_support(const char *feature)
+{
+	for (int i = 0; supported_features[i].name; ++i) {
+		if ((strcasecmp(feature, supported_features[i].name) == 0) ||
+		    (strcasecmp(feature, supported_features[i].macro) == 0)) {
+			if (!quiet)
+				STATUS(supported_features[i]);
+			return supported_features[i].is_builtin;
+		}
+	}
+
+	if (!quiet)
+		pr_err("Feature not known: '%s'\n", feature);
+
+	return 0;
+}
+
+
+/**
+ * Usage: 'perf check feature <feature_list>'
+ *
+ * <feature_list> can be a single feature name/macro, or a comma-separated list
+ * of feature names/macros
+ * eg. argument can be "libtraceevent" or "libtraceevent,bpf" etc
+ *
+ * In case of a comma-separated list, feature_enabled will be 1, only if
+ * all features passed in the string are supported
+ *
+ * Note that argv will get modified
+ */
+static int subcommand_feature(int argc, const char **argv)
+{
+	char *feature_list;
+	char *feature_name;
+	int feature_enabled;
+
+	argc = parse_options(argc, argv, check_feature_options,
+			check_feature_usage, 0);
+
+	if (!argc)
+		usage_with_options(check_feature_usage, check_feature_options);
+
+	if (argc > 1) {
+		pr_err("Too many arguments passed to 'perf check feature'\n");
+		return -1;
+	}
+
+	feature_enabled = 1;
+	/* feature_list is a non-const copy of 'argv[0]' */
+	feature_list = strdup(argv[0]);
+	if (!feature_list) {
+		pr_err("ERROR: failed to allocate memory for feature list\n");
+		return -1;
+	}
+
+	feature_name = strtok(feature_list, ",");
+
+	while (feature_name) {
+		feature_enabled &= has_support(feature_name);
+		feature_name = strtok(NULL, ",");
+	}
+
+	free(feature_list);
+
+	return !feature_enabled;
+}
+
+int cmd_check(int argc, const char **argv)
+{
+	argc = parse_options_subcommand(argc, argv, check_options,
+			check_subcommands, check_usage, 0);
+
+	if (!argc)
+		usage_with_options(check_usage, check_options);
+
+	if (strcmp(argv[0], "feature") == 0)
+		return subcommand_feature(argc, argv);
+
+	/* If no subcommand matched above, print usage help */
+	usage_with_options(check_usage, check_options);
+	return 0;
+}
diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
index f4375deabfa3..94f4b3769bf7 100644
--- a/tools/perf/builtin.h
+++ b/tools/perf/builtin.h
@@ -2,6 +2,22 @@
 #ifndef BUILTIN_H
 #define BUILTIN_H
 
+#include <stddef.h>
+#include <linux/compiler.h>
+#include <tools/config.h>
+
+struct feature_status {
+	const char *name;
+	const char *macro;
+	int is_builtin;
+};
+
+#define FEATURE_STATUS(name_, macro_) {    \
+	.name = name_,                     \
+	.macro = #macro_,                  \
+	.is_builtin = IS_BUILTIN(macro_) }
+
+extern struct feature_status supported_features[];
 struct cmdnames;
 
 void list_common_cmds_help(void);
@@ -11,6 +27,7 @@ int cmd_annotate(int argc, const char **argv);
 int cmd_bench(int argc, const char **argv);
 int cmd_buildid_cache(int argc, const char **argv);
 int cmd_buildid_list(int argc, const char **argv);
+int cmd_check(int argc, const char **argv);
 int cmd_config(int argc, const char **argv);
 int cmd_c2c(int argc, const char **argv);
 int cmd_diff(int argc, const char **argv);
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index bd3f80b5bb46..4def800f4089 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -52,6 +52,7 @@ static struct cmd_struct commands[] = {
 	{ "archive",	NULL,	0 },
 	{ "buildid-cache", cmd_buildid_cache, 0 },
 	{ "buildid-list", cmd_buildid_list, 0 },
+	{ "check",	cmd_check,	0 },
 	{ "config",	cmd_config,	0 },
 	{ "c2c",	cmd_c2c,	0 },
 	{ "diff",	cmd_diff,	0 },
-- 
2.45.2

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

[Namhyung Kim]

On Fri, Jun 28, 2024 at 12:12:33PM +0530, Aditya Gupta wrote:
> Currently the presence of a feature is checked with a combination of
> perf version --build-options and greps, such as:
> 
>     perf version --build-options | grep " on .* HAVE_FEATURE"
> 
> Instead of this, introduce a subcommand "perf check feature", with which
> scripts can test for presence of a feature, such as:
> 
>     perf check feature HAVE_FEATURE
> 
> 'perf check feature' command is expected to have exit status of 0 if
> feature is built-in, and 1 if it's not built-in or if feature is not known.
> 
> Multiple features can also be passed as a comma-separated list, in which
> case the exit status will be 1 only if all of the passed features are
> built-in. For example, with below command, it will have exit status of 0
> only if both libtraceevent and bpf are enabled, else 1 in all other cases
> 
>     perf check feature libtraceevent,bpf
> 
> The arguments are case-insensitive.
> An array 'supported_features' has also been introduced that can be used by
> other commands like 'perf version --build-options', so that new features
> can be added in one place, with the array
> 
> Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
>  tools/perf/Build                        |   1 +
>  tools/perf/Documentation/perf-check.txt |  79 +++++++++++
>  tools/perf/builtin-check.c              | 178 ++++++++++++++++++++++++
>  tools/perf/builtin.h                    |  17 +++
>  tools/perf/perf.c                       |   1 +
>  5 files changed, 276 insertions(+)
>  create mode 100644 tools/perf/Documentation/perf-check.txt
>  create mode 100644 tools/perf/builtin-check.c
> 
> diff --git a/tools/perf/Build b/tools/perf/Build
> index 1d4957957d75..3e486f7df94b 100644
> --- a/tools/perf/Build
> +++ b/tools/perf/Build
> @@ -1,5 +1,6 @@
>  perf-bench-y += builtin-bench.o
>  perf-y += builtin-annotate.o
> +perf-y += builtin-check.o
>  perf-y += builtin-config.o
>  perf-y += builtin-diff.o
>  perf-y += builtin-evlist.o
> diff --git a/tools/perf/Documentation/perf-check.txt b/tools/perf/Documentation/perf-check.txt
> new file mode 100644
> index 000000000000..88cfd41a76cc
> --- /dev/null
> +++ b/tools/perf/Documentation/perf-check.txt
> @@ -0,0 +1,79 @@
> +perf-check(1)
> +===============
> +
> +NAME
> +----
> +perf-check - check features in perf
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'perf check' [<options>]
> +'perf check' {feature <feature_list>} [<options>]
> +
> +DESCRIPTION
> +-----------
> +With no options given, the 'perf check' just prints the perf version
> +on the standard output.
> +
> +If the subcommand 'feature' is used, then status of feature is printed
> +on the standard output (unless '-q' is also passed), ie. whether it is
> +compiled-in/built-in or not.
> +Also, 'perf check feature' returns with exit status 0 if the feature
> +is built-in, otherwise returns with exit status 1.
> +
> +SUBCOMMANDS
> +-----------
> +
> +feature::
> +
> +        Print whether feature(s) is compiled-in or not, and also returns with an
> +        exit status of 0, if passed feature(s) are compiled-in, else 1.
> +
> +        It expects a feature list as an argument. There can be a single feature
> +        name/macro, or multiple features can also be passed as a comma-separated
> +        list, in which case the exit status will be 0 only if all of the passed
> +        features are compiled-in.
> +
> +        The feature names/macros are case-insensitive.
> +
> +        Example Usage:
> +                perf check feature libtraceevent
> +                perf check feature HAVE_LIBTRACEEVENT
> +                perf check feature libtraceevent,bpf
> +
> +        Supported feature names/macro:
> +                aio                     /  HAVE_AIO_SUPPORT
> +                bpf                     /  HAVE_LIBBPF_SUPPORT
> +                bpf_skeletons           /  HAVE_BPF_SKEL
> +                debuginfod              /  HAVE_DEBUGINFOD_SUPPORT
> +                dwarf                   /  HAVE_DWARF_SUPPORT
> +                dwarf_getlocations      /  HAVE_DWARF_GETLOCATIONS_SUPPORT
> +                get_cpuid               /  HAVE_AUXTRACE_SUPPORT
> +                numa_num_possible_cpus  /  HAVE_LIBNUMA_SUPPORT
> +                libaudit                /  HAVE_LIBAUDIT_SUPPORT
> +                libbfd                  /  HAVE_LIBBFD_SUPPORT
> +                libelf                  /  HAVE_LIBELF_SUPPORT
> +                libcrypto               /  HAVE_LIBCRYPTO_SUPPORT
> +                libdw-dwarf-unwind      /  HAVE_DWARF_SUPPORT
> +                libnuma                 /  HAVE_LIBNUMA_SUPPORT
> +                libperl                 /  HAVE_LIBPERL_SUPPORT
> +                libpfm4                 /  HAVE_LIBPFM
> +                libpython               /  HAVE_LIBPYTHON_SUPPORT
> +                libslang                /  HAVE_SLANG_SUPPORT
> +                libtraceevent           /  HAVE_LIBTRACEEVENT
> +                libunwind               /  HAVE_LIBUNWIND_SUPPORT
> +                lzma                    /  HAVE_LZMA_SUPPORT
> +                syscall_table           /  HAVE_SYSCALL_TABLE_SUPPORT
> +                zlib                    /  HAVE_ZLIB_SUPPORT
> +                zstd                    /  HAVE_ZSTD_SUPPORT
> +
> +OPTIONS
> +-------
> +-q::
> +--quiet::
> +        Do not print any messages or warnings
> +
> +        This can be used along with subcommands such as 'perf check feature'
> +        to hide unnecessary output in test scripts, eg.
> +        'perf check feature --quiet libtraceevent'
> diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> new file mode 100644
> index 000000000000..44ffde6f8dbe
> --- /dev/null
> +++ b/tools/perf/builtin-check.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "builtin.h"
> +#include "color.h"
> +#include "util/debug.h"
> +#include "util/header.h"
> +#include <tools/config.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <subcmd/parse-options.h>
> +
> +static const char * const check_subcommands[] = { "feature", NULL };
> +static struct option check_options[] = {
> +	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
> +	OPT_END()
> +};
> +static struct option check_feature_options[] = { OPT_END() };

It should be OPT_PARENT(check_options) instead of OPT_END().

> +
> +static const char *check_usage[] = {
> +	"perf check [<subcommand>] [<options>]",

You can leave this NULL and parse_options_subcommand() will fill the
first element automatically using check_subcommands[].

Please see other commands like 'perf sched' how to handle this.

Thanks,
Namhyung


> +	NULL
> +};
> +static const char *check_feature_usage[] = {
> +	"perf check feature <feature_list>",
> +	NULL
> +};
> +
> +struct feature_status supported_features[] = {
> +	FEATURE_STATUS("aio", HAVE_AIO_SUPPORT),
> +	FEATURE_STATUS("bpf", HAVE_LIBBPF_SUPPORT),
> +	FEATURE_STATUS("bpf_skeletons", HAVE_BPF_SKEL),
> +	FEATURE_STATUS("debuginfod", HAVE_DEBUGINFOD_SUPPORT),
> +	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("libaudit", HAVE_LIBAUDIT_SUPPORT),
> +	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
> +	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),
> +	FEATURE_STATUS("libcrypto", HAVE_LIBCRYPTO_SUPPORT),
> +	FEATURE_STATUS("libdw-dwarf-unwind", HAVE_DWARF_SUPPORT),
> +	FEATURE_STATUS("libelf", HAVE_LIBELF_SUPPORT),
> +	FEATURE_STATUS("libnuma", HAVE_LIBNUMA_SUPPORT),
> +	FEATURE_STATUS("libopencsd", HAVE_CSTRACE_SUPPORT),
> +	FEATURE_STATUS("libperl", HAVE_LIBPERL_SUPPORT),
> +	FEATURE_STATUS("libpfm4", HAVE_LIBPFM),
> +	FEATURE_STATUS("libpython", HAVE_LIBPYTHON_SUPPORT),
> +	FEATURE_STATUS("libslang", HAVE_SLANG_SUPPORT),
> +	FEATURE_STATUS("libtraceevent", HAVE_LIBTRACEEVENT),
> +	FEATURE_STATUS("libunwind", HAVE_LIBUNWIND_SUPPORT),
> +	FEATURE_STATUS("lzma", HAVE_LZMA_SUPPORT),
> +	FEATURE_STATUS("numa_num_possible_cpus", HAVE_LIBNUMA_SUPPORT),
> +	FEATURE_STATUS("syscall_table", HAVE_SYSCALL_TABLE_SUPPORT),
> +	FEATURE_STATUS("zlib", HAVE_ZLIB_SUPPORT),
> +	FEATURE_STATUS("zstd", HAVE_ZSTD_SUPPORT),
> +
> +	/* this should remain at end, to know the array end */
> +	FEATURE_STATUS(NULL, _)
> +};
> +
> +static void on_off_print(const char *status)
> +{
> +	printf("[ ");
> +
> +	if (!strcmp(status, "OFF"))
> +		color_fprintf(stdout, PERF_COLOR_RED, "%-3s", status);
> +	else
> +		color_fprintf(stdout, PERF_COLOR_GREEN, "%-3s", status);
> +
> +	printf(" ]");
> +}
> +
> +/* Helper function to print status of a feature along with name/macro */
> +static void status_print(const char *name, const char *macro,
> +			 const char *status)
> +{
> +	printf("%22s: ", name);
> +	on_off_print(status);
> +	printf("  # %s\n", macro);
> +}
> +
> +#define STATUS(feature)                                           \
> +do {                                                              \
> +	if (feature.is_builtin)                                   \
> +		status_print(feature.name, feature.macro, "on");  \
> +	else                                                      \
> +		status_print(feature.name, feature.macro, "OFF"); \
> +} while (0)
> +
> +/**
> + * check whether "feature" is built-in with perf
> + *
> + * returns:
> + *    0: NOT built-in or Feature not known
> + *    1: Built-in
> + */
> +static int has_support(const char *feature)
> +{
> +	for (int i = 0; supported_features[i].name; ++i) {
> +		if ((strcasecmp(feature, supported_features[i].name) == 0) ||
> +		    (strcasecmp(feature, supported_features[i].macro) == 0)) {
> +			if (!quiet)
> +				STATUS(supported_features[i]);
> +			return supported_features[i].is_builtin;
> +		}
> +	}
> +
> +	if (!quiet)
> +		pr_err("Feature not known: '%s'\n", feature);
> +
> +	return 0;
> +}
> +
> +
> +/**
> + * Usage: 'perf check feature <feature_list>'
> + *
> + * <feature_list> can be a single feature name/macro, or a comma-separated list
> + * of feature names/macros
> + * eg. argument can be "libtraceevent" or "libtraceevent,bpf" etc
> + *
> + * In case of a comma-separated list, feature_enabled will be 1, only if
> + * all features passed in the string are supported
> + *
> + * Note that argv will get modified
> + */
> +static int subcommand_feature(int argc, const char **argv)
> +{
> +	char *feature_list;
> +	char *feature_name;
> +	int feature_enabled;
> +
> +	argc = parse_options(argc, argv, check_feature_options,
> +			check_feature_usage, 0);
> +
> +	if (!argc)
> +		usage_with_options(check_feature_usage, check_feature_options);
> +
> +	if (argc > 1) {
> +		pr_err("Too many arguments passed to 'perf check feature'\n");
> +		return -1;
> +	}
> +
> +	feature_enabled = 1;
> +	/* feature_list is a non-const copy of 'argv[0]' */
> +	feature_list = strdup(argv[0]);
> +	if (!feature_list) {
> +		pr_err("ERROR: failed to allocate memory for feature list\n");
> +		return -1;
> +	}
> +
> +	feature_name = strtok(feature_list, ",");
> +
> +	while (feature_name) {
> +		feature_enabled &= has_support(feature_name);
> +		feature_name = strtok(NULL, ",");
> +	}
> +
> +	free(feature_list);
> +
> +	return !feature_enabled;
> +}
> +
> +int cmd_check(int argc, const char **argv)
> +{
> +	argc = parse_options_subcommand(argc, argv, check_options,
> +			check_subcommands, check_usage, 0);
> +
> +	if (!argc)
> +		usage_with_options(check_usage, check_options);
> +
> +	if (strcmp(argv[0], "feature") == 0)
> +		return subcommand_feature(argc, argv);
> +
> +	/* If no subcommand matched above, print usage help */
> +	usage_with_options(check_usage, check_options);
> +	return 0;
> +}
> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> index f4375deabfa3..94f4b3769bf7 100644
> --- a/tools/perf/builtin.h
> +++ b/tools/perf/builtin.h
> @@ -2,6 +2,22 @@
>  #ifndef BUILTIN_H
>  #define BUILTIN_H
>  
> +#include <stddef.h>
> +#include <linux/compiler.h>
> +#include <tools/config.h>
> +
> +struct feature_status {
> +	const char *name;
> +	const char *macro;
> +	int is_builtin;
> +};
> +
> +#define FEATURE_STATUS(name_, macro_) {    \
> +	.name = name_,                     \
> +	.macro = #macro_,                  \
> +	.is_builtin = IS_BUILTIN(macro_) }
> +
> +extern struct feature_status supported_features[];
>  struct cmdnames;
>  
>  void list_common_cmds_help(void);
> @@ -11,6 +27,7 @@ int cmd_annotate(int argc, const char **argv);
>  int cmd_bench(int argc, const char **argv);
>  int cmd_buildid_cache(int argc, const char **argv);
>  int cmd_buildid_list(int argc, const char **argv);
> +int cmd_check(int argc, const char **argv);
>  int cmd_config(int argc, const char **argv);
>  int cmd_c2c(int argc, const char **argv);
>  int cmd_diff(int argc, const char **argv);
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index bd3f80b5bb46..4def800f4089 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -52,6 +52,7 @@ static struct cmd_struct commands[] = {
>  	{ "archive",	NULL,	0 },
>  	{ "buildid-cache", cmd_buildid_cache, 0 },
>  	{ "buildid-list", cmd_buildid_list, 0 },
> +	{ "check",	cmd_check,	0 },
>  	{ "config",	cmd_config,	0 },
>  	{ "c2c",	cmd_c2c,	0 },
>  	{ "diff",	cmd_diff,	0 },
> -- 
> 2.45.2
> 

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

[Aditya Gupta]

Hi Namhyung,

> > +static const char * const check_subcommands[] = { "feature", NULL };
> > +static struct option check_options[] = {
> > +	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
> > +	OPT_END()
> > +};
> > +static struct option check_feature_options[] = { OPT_END() };
> 
> It should be OPT_PARENT(check_options) instead of OPT_END().

I did not know that. Thank you.

> 
> > +
> > +static const char *check_usage[] = {
> > +	"perf check [<subcommand>] [<options>]",
> 
> You can leave this NULL and parse_options_subcommand() will fill the
> first element automatically using check_subcommands[].
> 
> Please see other commands like 'perf sched' how to handle this.

It doesn't seem to be working, hence added that check_usage in v12
itself.

    $ ./perf check
	Usage: (null)

    -q, --quiet           do not show any warnings or messages

    $ ./perf sched
	Usage: (null)

    -D, --dump-raw-trace  dump raw trace in ASCII
    -f, --force           don't complain, do it
    -i, --input <file>    input file name
    -v, --verbose         be more verbose (show symbol address, etc)

Debugging it further, this behaviour was changed in

    commit 230a7a71f9221: libsubcmd: Fix parse-options memory leak

Where the generated usage string is deallocated, and usage[0] string is
reassigned as NULL.

If expected behaviour was allocation of the usage string, it should be
okay for the buffer to not get deallocated for the entirety of the perf
process's lifetime right ?

Thanks,
Aditya Gupta

> 
> Thanks,
> Namhyung
> 
> 
> > +	NULL
> > +};
> > +static const char *check_feature_usage[] = {
> > +	"perf check feature <feature_list>",
> > +	NULL
> > +};
> > +
> > +struct feature_status supported_features[] = {
> > +	FEATURE_STATUS("aio", HAVE_AIO_SUPPORT),
> > +	FEATURE_STATUS("bpf", HAVE_LIBBPF_SUPPORT),
> > +	FEATURE_STATUS("bpf_skeletons", HAVE_BPF_SKEL),
> > +	FEATURE_STATUS("debuginfod", HAVE_DEBUGINFOD_SUPPORT),
> > +	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("libaudit", HAVE_LIBAUDIT_SUPPORT),
> > +	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
> > +	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),
> > +	FEATURE_STATUS("libcrypto", HAVE_LIBCRYPTO_SUPPORT),
> > +	FEATURE_STATUS("libdw-dwarf-unwind", HAVE_DWARF_SUPPORT),
> > +	FEATURE_STATUS("libelf", HAVE_LIBELF_SUPPORT),
> > +	FEATURE_STATUS("libnuma", HAVE_LIBNUMA_SUPPORT),
> > +	FEATURE_STATUS("libopencsd", HAVE_CSTRACE_SUPPORT),
> > +	FEATURE_STATUS("libperl", HAVE_LIBPERL_SUPPORT),
> > +	FEATURE_STATUS("libpfm4", HAVE_LIBPFM),
> > +	FEATURE_STATUS("libpython", HAVE_LIBPYTHON_SUPPORT),
> > +	FEATURE_STATUS("libslang", HAVE_SLANG_SUPPORT),
> > +	FEATURE_STATUS("libtraceevent", HAVE_LIBTRACEEVENT),
> > +	FEATURE_STATUS("libunwind", HAVE_LIBUNWIND_SUPPORT),
> > +	FEATURE_STATUS("lzma", HAVE_LZMA_SUPPORT),
> > +	FEATURE_STATUS("numa_num_possible_cpus", HAVE_LIBNUMA_SUPPORT),
> > +	FEATURE_STATUS("syscall_table", HAVE_SYSCALL_TABLE_SUPPORT),
> > +	FEATURE_STATUS("zlib", HAVE_ZLIB_SUPPORT),
> > +	FEATURE_STATUS("zstd", HAVE_ZSTD_SUPPORT),
> > +
> > +	/* this should remain at end, to know the array end */
> > +	FEATURE_STATUS(NULL, _)
> > +};
> > +
> > +static void on_off_print(const char *status)
> > +{
> > +	printf("[ ");
> > +
> > +	if (!strcmp(status, "OFF"))
> > +		color_fprintf(stdout, PERF_COLOR_RED, "%-3s", status);
> > +	else
> > +		color_fprintf(stdout, PERF_COLOR_GREEN, "%-3s", status);
> > +
> > +	printf(" ]");
> > +}
> > +
> > +/* Helper function to print status of a feature along with name/macro */
> > +static void status_print(const char *name, const char *macro,
> > +			 const char *status)
> > +{
> > +	printf("%22s: ", name);
> > +	on_off_print(status);
> > +	printf("  # %s\n", macro);
> > +}
> > +
> > +#define STATUS(feature)                                           \
> > +do {                                                              \
> > +	if (feature.is_builtin)                                   \
> > +		status_print(feature.name, feature.macro, "on");  \
> > +	else                                                      \
> > +		status_print(feature.name, feature.macro, "OFF"); \
> > +} while (0)
> > +
> > +/**
> > + * check whether "feature" is built-in with perf
> > + *
> > + * returns:
> > + *    0: NOT built-in or Feature not known
> > + *    1: Built-in
> > + */
> > +static int has_support(const char *feature)
> > +{
> > +	for (int i = 0; supported_features[i].name; ++i) {
> > +		if ((strcasecmp(feature, supported_features[i].name) == 0) ||
> > +		    (strcasecmp(feature, supported_features[i].macro) == 0)) {
> > +			if (!quiet)
> > +				STATUS(supported_features[i]);
> > +			return supported_features[i].is_builtin;
> > +		}
> > +	}
> > +
> > +	if (!quiet)
> > +		pr_err("Feature not known: '%s'\n", feature);
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +/**
> > + * Usage: 'perf check feature <feature_list>'
> > + *
> > + * <feature_list> can be a single feature name/macro, or a comma-separated list
> > + * of feature names/macros
> > + * eg. argument can be "libtraceevent" or "libtraceevent,bpf" etc
> > + *
> > + * In case of a comma-separated list, feature_enabled will be 1, only if
> > + * all features passed in the string are supported
> > + *
> > + * Note that argv will get modified
> > + */
> > +static int subcommand_feature(int argc, const char **argv)
> > +{
> > +	char *feature_list;
> > +	char *feature_name;
> > +	int feature_enabled;
> > +
> > +	argc = parse_options(argc, argv, check_feature_options,
> > +			check_feature_usage, 0);
> > +
> > +	if (!argc)
> > +		usage_with_options(check_feature_usage, check_feature_options);
> > +
> > +	if (argc > 1) {
> > +		pr_err("Too many arguments passed to 'perf check feature'\n");
> > +		return -1;
> > +	}
> > +
> > +	feature_enabled = 1;
> > +	/* feature_list is a non-const copy of 'argv[0]' */
> > +	feature_list = strdup(argv[0]);
> > +	if (!feature_list) {
> > +		pr_err("ERROR: failed to allocate memory for feature list\n");
> > +		return -1;
> > +	}
> > +
> > +	feature_name = strtok(feature_list, ",");
> > +
> > +	while (feature_name) {
> > +		feature_enabled &= has_support(feature_name);
> > +		feature_name = strtok(NULL, ",");
> > +	}
> > +
> > +	free(feature_list);
> > +
> > +	return !feature_enabled;
> > +}
> > +
> > +int cmd_check(int argc, const char **argv)
> > +{
> > +	argc = parse_options_subcommand(argc, argv, check_options,
> > +			check_subcommands, check_usage, 0);
> > +
> > +	if (!argc)
> > +		usage_with_options(check_usage, check_options);
> > +
> > +	if (strcmp(argv[0], "feature") == 0)
> > +		return subcommand_feature(argc, argv);
> > +
> > +	/* If no subcommand matched above, print usage help */
> > +	usage_with_options(check_usage, check_options);
> > +	return 0;
> > +}
> > diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> > index f4375deabfa3..94f4b3769bf7 100644
> > --- a/tools/perf/builtin.h
> > +++ b/tools/perf/builtin.h
> > @@ -2,6 +2,22 @@
> >  #ifndef BUILTIN_H
> >  #define BUILTIN_H
> >  
> > +#include <stddef.h>
> > +#include <linux/compiler.h>
> > +#include <tools/config.h>
> > +
> > +struct feature_status {
> > +	const char *name;
> > +	const char *macro;
> > +	int is_builtin;
> > +};
> > +
> > +#define FEATURE_STATUS(name_, macro_) {    \
> > +	.name = name_,                     \
> > +	.macro = #macro_,                  \
> > +	.is_builtin = IS_BUILTIN(macro_) }
> > +
> > +extern struct feature_status supported_features[];
> >  struct cmdnames;
> >  
> >  void list_common_cmds_help(void);
> > @@ -11,6 +27,7 @@ int cmd_annotate(int argc, const char **argv);
> >  int cmd_bench(int argc, const char **argv);
> >  int cmd_buildid_cache(int argc, const char **argv);
> >  int cmd_buildid_list(int argc, const char **argv);
> > +int cmd_check(int argc, const char **argv);
> >  int cmd_config(int argc, const char **argv);
> >  int cmd_c2c(int argc, const char **argv);
> >  int cmd_diff(int argc, const char **argv);
> > diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> > index bd3f80b5bb46..4def800f4089 100644
> > --- a/tools/perf/perf.c
> > +++ b/tools/perf/perf.c
> > @@ -52,6 +52,7 @@ static struct cmd_struct commands[] = {
> >  	{ "archive",	NULL,	0 },
> >  	{ "buildid-cache", cmd_buildid_cache, 0 },
> >  	{ "buildid-list", cmd_buildid_list, 0 },
> > +	{ "check",	cmd_check,	0 },
> >  	{ "config",	cmd_config,	0 },
> >  	{ "c2c",	cmd_c2c,	0 },
> >  	{ "diff",	cmd_diff,	0 },
> > -- 
> > 2.45.2
> > 

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

[Namhyung Kim]

On Sun, Jun 30, 2024 at 05:07:10PM +0530, Aditya Gupta wrote:
> Hi Namhyung,
> 
> > > +static const char * const check_subcommands[] = { "feature", NULL };
> > > +static struct option check_options[] = {
> > > +	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
> > > +	OPT_END()
> > > +};
> > > +static struct option check_feature_options[] = { OPT_END() };
> > 
> > It should be OPT_PARENT(check_options) instead of OPT_END().
> 
> I did not know that. Thank you.
> 
> > 
> > > +
> > > +static const char *check_usage[] = {
> > > +	"perf check [<subcommand>] [<options>]",
> > 
> > You can leave this NULL and parse_options_subcommand() will fill the
> > first element automatically using check_subcommands[].
> > 
> > Please see other commands like 'perf sched' how to handle this.
> 
> It doesn't seem to be working, hence added that check_usage in v12
> itself.
> 
>     $ ./perf check
> 	Usage: (null)
> 
>     -q, --quiet           do not show any warnings or messages
> 
>     $ ./perf sched
> 	Usage: (null)
> 
>     -D, --dump-raw-trace  dump raw trace in ASCII
>     -f, --force           don't complain, do it
>     -i, --input <file>    input file name
>     -v, --verbose         be more verbose (show symbol address, etc)
> 
> Debugging it further, this behaviour was changed in
> 
>     commit 230a7a71f9221: libsubcmd: Fix parse-options memory leak
> 
> Where the generated usage string is deallocated, and usage[0] string is
> reassigned as NULL.

Ok, thanks for the investigation.  It's a bug then.

> 
> If expected behaviour was allocation of the usage string, it should be
> okay for the buffer to not get deallocated for the entirety of the perf
> process's lifetime right ?

Right, it should not deallocate it in the parse_options_subcommand().
I think we need to change the exit path of the commands to free the
usage[0] manually.

Thanks,
Namhyung

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

[Aditya Gupta]

Hi Namhyung,

> > <...snip...>
> >
> > > > +static const char *check_usage[] = {
> > > > +	"perf check [<subcommand>] [<options>]",
> > > 
> > > You can leave this NULL and parse_options_subcommand() will fill the
> > > first element automatically using check_subcommands[].
> > > 
> > > Please see other commands like 'perf sched' how to handle this.
> > 
> > It doesn't seem to be working, hence added that check_usage in v12
> > itself.
> > 
> >     $ ./perf check
> > 	Usage: (null)
> > 
> >     -q, --quiet           do not show any warnings or messages
> > 
> >     $ ./perf sched
> > 	Usage: (null)
> > 
> >     -D, --dump-raw-trace  dump raw trace in ASCII
> >     -f, --force           don't complain, do it
> >     -i, --input <file>    input file name
> >     -v, --verbose         be more verbose (show symbol address, etc)
> > 
> > Debugging it further, this behaviour was changed in
> > 
> >     commit 230a7a71f9221: libsubcmd: Fix parse-options memory leak
> > 
> > Where the generated usage string is deallocated, and usage[0] string is
> > reassigned as NULL.
> 
> Ok, thanks for the investigation.  It's a bug then.
> 
> > 
> > If expected behaviour was allocation of the usage string, it should be
> > okay for the buffer to not get deallocated for the entirety of the perf
> > process's lifetime right ?
> 
> Right, it should not deallocate it in the parse_options_subcommand().
> I think we need to change the exit path of the commands to free the
> usage[0] manually.

Sure, I can implement that if you have things in hand, or I am fine with
you implementing it since you will already have an idea what to do.

Maybe `free(command_usage[0])` or something like
`parse_options_free_subcommand()` ?

Thanks,
Aditya Gupta

> 
> Thanks,
> Namhyung
> 

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

[Namhyung Kim]

On Wed, Jul 03, 2024 at 04:17:09PM +0530, Aditya Gupta wrote:
> Hi Namhyung,
> 
> > > <...snip...>
> > >
> > > > > +static const char *check_usage[] = {
> > > > > +	"perf check [<subcommand>] [<options>]",
> > > > 
> > > > You can leave this NULL and parse_options_subcommand() will fill the
> > > > first element automatically using check_subcommands[].
> > > > 
> > > > Please see other commands like 'perf sched' how to handle this.
> > > 
> > > It doesn't seem to be working, hence added that check_usage in v12
> > > itself.
> > > 
> > >     $ ./perf check
> > > 	Usage: (null)
> > > 
> > >     -q, --quiet           do not show any warnings or messages
> > > 
> > >     $ ./perf sched
> > > 	Usage: (null)
> > > 
> > >     -D, --dump-raw-trace  dump raw trace in ASCII
> > >     -f, --force           don't complain, do it
> > >     -i, --input <file>    input file name
> > >     -v, --verbose         be more verbose (show symbol address, etc)
> > > 
> > > Debugging it further, this behaviour was changed in
> > > 
> > >     commit 230a7a71f9221: libsubcmd: Fix parse-options memory leak
> > > 
> > > Where the generated usage string is deallocated, and usage[0] string is
> > > reassigned as NULL.
> > 
> > Ok, thanks for the investigation.  It's a bug then.
> > 
> > > 
> > > If expected behaviour was allocation of the usage string, it should be
> > > okay for the buffer to not get deallocated for the entirety of the perf
> > > process's lifetime right ?
> > 
> > Right, it should not deallocate it in the parse_options_subcommand().
> > I think we need to change the exit path of the commands to free the
> > usage[0] manually.
> 
> Sure, I can implement that if you have things in hand, or I am fine with
> you implementing it since you will already have an idea what to do.
> 
> Maybe `free(command_usage[0])` or something like
> `parse_options_free_subcommand()` ?

Yeah, but this should be a separate fix and it's ok to keep the usage
string like in your patch.

Thanks,
Namhyung

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

[Namhyung Kim]

Hello,

On Wed, Jul 03, 2024 at 04:17:09PM +0530, Aditya Gupta wrote:
> Hi Namhyung,
> 
> > > <...snip...>
> > >
> > > > > +static const char *check_usage[] = {
> > > > > +	"perf check [<subcommand>] [<options>]",
> > > > 
> > > > You can leave this NULL and parse_options_subcommand() will fill the
> > > > first element automatically using check_subcommands[].
> > > > 
> > > > Please see other commands like 'perf sched' how to handle this.
> > > 
> > > It doesn't seem to be working, hence added that check_usage in v12
> > > itself.
> > > 
> > >     $ ./perf check
> > > 	Usage: (null)
> > > 
> > >     -q, --quiet           do not show any warnings or messages
> > > 
> > >     $ ./perf sched
> > > 	Usage: (null)
> > > 
> > >     -D, --dump-raw-trace  dump raw trace in ASCII
> > >     -f, --force           don't complain, do it
> > >     -i, --input <file>    input file name
> > >     -v, --verbose         be more verbose (show symbol address, etc)
> > > 
> > > Debugging it further, this behaviour was changed in
> > > 
> > >     commit 230a7a71f9221: libsubcmd: Fix parse-options memory leak
> > > 
> > > Where the generated usage string is deallocated, and usage[0] string is
> > > reassigned as NULL.
> > 
> > Ok, thanks for the investigation.  It's a bug then.
> > 
> > > 
> > > If expected behaviour was allocation of the usage string, it should be
> > > okay for the buffer to not get deallocated for the entirety of the perf
> > > process's lifetime right ?
> > 
> > Right, it should not deallocate it in the parse_options_subcommand().
> > I think we need to change the exit path of the commands to free the
> > usage[0] manually.
> 
> Sure, I can implement that if you have things in hand, or I am fine with
> you implementing it since you will already have an idea what to do.
> 
> Maybe `free(command_usage[0])` or something like
> `parse_options_free_subcommand()` ?

It seems we don't have a place to mark whether it needs to free or not.
Let's go with free(command_usage[0]) for now.

Thanks,
Namhyung

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

[Aditya Gupta]

Hi Namhyung,

On 13/07/24 01:52, Namhyung Kim wrote:
>
> <...snip...>
>> Maybe `free(command_usage[0])` or something like
>> `parse_options_free_subcommand()` ?
> It seems we don't have a place to mark whether it needs to free or not.
> Let's go with free(command_usage[0]) for now.


Sure, I will post a separate fix patch for this.


Thanks,

Aditya Gupta

>
> Thanks,
> Namhyung
>

[PATCH v12 2/4] perf version: update --build-options to use 'supported_features' array

[Aditya Gupta]

Now that the feature list has been duplicated in a global
'supported_features' array, use that array instead of manually checking
status of built-in features.

This helps in being consistent with commands such as 'perf check feature',
so commands can use the same array, and any new feature can be added at
one place, in the 'supported_features' array

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/builtin-version.c | 43 +++++++-----------------------------
 1 file changed, 8 insertions(+), 35 deletions(-)

diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
index 398aa53e9e2e..e149d96c6dc5 100644
--- a/tools/perf/builtin-version.c
+++ b/tools/perf/builtin-version.c
@@ -46,45 +46,18 @@ static void status_print(const char *name, const char *macro,
 	printf("  # %s\n", macro);
 }
 
-#define STATUS(__d, __m)				\
-do {							\
-	if (IS_BUILTIN(__d))				\
-		status_print(#__m, #__d, "on");		\
-	else						\
-		status_print(#__m, #__d, "OFF");	\
+#define STATUS(feature)                                   \
+do {                                                      \
+	if (feature.is_builtin)                               \
+		status_print(feature.name, feature.macro, "on");  \
+	else                                                  \
+		status_print(feature.name, feature.macro, "OFF"); \
 } while (0)
 
 static void library_status(void)
 {
-	STATUS(HAVE_DWARF_SUPPORT, dwarf);
-	STATUS(HAVE_DWARF_GETLOCATIONS_SUPPORT, dwarf_getlocations);
-#ifndef HAVE_SYSCALL_TABLE_SUPPORT
-	STATUS(HAVE_LIBAUDIT_SUPPORT, libaudit);
-#endif
-	STATUS(HAVE_SYSCALL_TABLE_SUPPORT, syscall_table);
-	STATUS(HAVE_LIBBFD_SUPPORT, libbfd);
-	STATUS(HAVE_DEBUGINFOD_SUPPORT, debuginfod);
-	STATUS(HAVE_LIBELF_SUPPORT, libelf);
-	STATUS(HAVE_LIBNUMA_SUPPORT, libnuma);
-	STATUS(HAVE_LIBNUMA_SUPPORT, numa_num_possible_cpus);
-	STATUS(HAVE_LIBPERL_SUPPORT, libperl);
-	STATUS(HAVE_LIBPYTHON_SUPPORT, libpython);
-	STATUS(HAVE_SLANG_SUPPORT, libslang);
-	STATUS(HAVE_LIBCRYPTO_SUPPORT, libcrypto);
-	STATUS(HAVE_LIBUNWIND_SUPPORT, libunwind);
-	STATUS(HAVE_DWARF_SUPPORT, libdw-dwarf-unwind);
-	STATUS(HAVE_LIBCAPSTONE_SUPPORT, libcapstone);
-	STATUS(HAVE_ZLIB_SUPPORT, zlib);
-	STATUS(HAVE_LZMA_SUPPORT, lzma);
-	STATUS(HAVE_AUXTRACE_SUPPORT, get_cpuid);
-	STATUS(HAVE_LIBBPF_SUPPORT, bpf);
-	STATUS(HAVE_AIO_SUPPORT, aio);
-	STATUS(HAVE_ZSTD_SUPPORT, zstd);
-	STATUS(HAVE_LIBPFM, libpfm4);
-	STATUS(HAVE_LIBTRACEEVENT, libtraceevent);
-	STATUS(HAVE_BPF_SKEL, bpf_skeletons);
-	STATUS(HAVE_DWARF_UNWIND_SUPPORT, dwarf-unwind-support);
-	STATUS(HAVE_CSTRACE_SUPPORT, libopencsd);
+	for (int i = 0; supported_features[i].name; ++i)
+		STATUS(supported_features[i]);
 }
 
 int cmd_version(int argc, const char **argv)
-- 
2.45.2

[PATCH v12 3/4] perf tests task_analyzer: use perf check for libtraceevent support

[Aditya Gupta]

Currently we use output of 'perf version --build-options', to check
whether perf was built with libtraceevent support.

Instead, use 'perf check feature libtraceevent' to check for
libtraceevent support.

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/tests/shell/test_task_analyzer.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/shell/test_task_analyzer.sh b/tools/perf/tests/shell/test_task_analyzer.sh
index 92d15154ba79..456665ba465e 100755
--- a/tools/perf/tests/shell/test_task_analyzer.sh
+++ b/tools/perf/tests/shell/test_task_analyzer.sh
@@ -52,8 +52,8 @@ find_str_or_fail() {
 
 # check if perf is compiled with libtraceevent support
 skip_no_probe_record_support() {
-	perf version --build-options | grep -q " OFF .* HAVE_LIBTRACEEVENT" && return 2
-	return 0
+	perf check feature -q libtraceevent && return 0
+	return 2
 }
 
 prepare_perf_data() {
-- 
2.45.2

[PATCH v12 4/4] tools/perf/tests: Update probe_vfs_getname.sh script to use perf check feature

[Aditya Gupta]

From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

In probe_vfs_getname.sh, current we use "perf record --dry-run"
to check for libtraceevent and skip the test if perf is not
build with libtraceevent. Change the check to use "perf check feature"
option

Acked-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/shell/lib/probe_vfs_getname.sh           | 4 ++--
 tools/perf/tests/shell/record+probe_libc_inet_pton.sh     | 5 ++++-
 tools/perf/tests/shell/record+script_probe_vfs_getname.sh | 5 ++++-
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/shell/lib/probe_vfs_getname.sh b/tools/perf/tests/shell/lib/probe_vfs_getname.sh
index bf4c1fb71c4b..24cca0fbfe31 100644
--- a/tools/perf/tests/shell/lib/probe_vfs_getname.sh
+++ b/tools/perf/tests/shell/lib/probe_vfs_getname.sh
@@ -27,7 +27,7 @@ skip_if_no_debuginfo() {
 # check if perf is compiled with libtraceevent support
 skip_no_probe_record_support() {
 	if [ $had_vfs_getname -eq 1 ] ; then
-		perf record --dry-run -e $1 2>&1 | grep "libtraceevent is necessary for tracepoint support" && return 2
-		return 1
+		perf check feature -q libtraceevent && return 1
+		return 2
 	fi
 }
diff --git a/tools/perf/tests/shell/record+probe_libc_inet_pton.sh b/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
index 72c65570db37..f38c8ead0b03 100755
--- a/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
+++ b/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
@@ -63,7 +63,10 @@ trace_libc_inet_pton_backtrace() {
 
 	# Check presence of libtraceevent support to run perf record
 	skip_no_probe_record_support "$event_name/$eventattr/"
-	[ $? -eq 2 ] && return 2
+	if [ $? -eq 2 ]; then
+		echo "WARN: Skipping test trace_libc_inet_pton_backtrace. No libtraceevent support."
+		return 2
+	fi
 
 	perf record -e $event_name/$eventattr/ -o $perf_data ping -6 -c 1 ::1 > /dev/null 2>&1
 	# check if perf data file got created in above step.
diff --git a/tools/perf/tests/shell/record+script_probe_vfs_getname.sh b/tools/perf/tests/shell/record+script_probe_vfs_getname.sh
index 5eedbe29bba1..9a61928e3c9a 100755
--- a/tools/perf/tests/shell/record+script_probe_vfs_getname.sh
+++ b/tools/perf/tests/shell/record+script_probe_vfs_getname.sh
@@ -21,7 +21,10 @@ record_open_file() {
 	echo "Recording open file:"
 	# Check presence of libtraceevent support to run perf record
 	skip_no_probe_record_support "probe:vfs_getname*"
-	[ $? -eq 2 ] && return 2
+	if [ $? -eq 2 ]; then
+		echo "WARN: Skipping test record_open_file. No libtraceevent support"
+		return 2
+	fi
 	perf record -o ${perfdata} -e probe:vfs_getname\* touch $file
 }
 
-- 
2.45.2