[PATCH v13 0/7] Introduce perf check subcommand

[PATCH v13 0/7] 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-v13

Changelog
=========

v13:
+ patch #1: add fix for parse_options_subcommand not setting usage string
+ patch #2: if unknown feature name passed, print "please use 'perf version
  --build-options' to see which ones are available"
+ patch #6: fix inconsistency in feature names (dwarf-unwind-support & get_cpuid)
+ patch #7: add more features to feature_list

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/

Aditya Gupta (6):
  tools/lib/subcmd: Don't free the usage string
  perf check: Introduce 'check' subcommand
  perf version: Update --build-options to use 'supported_features' array
  tools/perf/tests: Update test_task_analyzer.sh to use perf check
    feature
  perf: Fix inconsistencies in feature names
  perf: Add more features to supported_features list

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

 tools/lib/subcmd/parse-options.c              |   8 +-
 tools/perf/Build                              |   1 +
 tools/perf/Documentation/perf-check.txt       | 106 +++++++++
 tools/perf/builtin-check.c                    | 205 ++++++++++++++++++
 tools/perf/builtin-kmem.c                     |   2 +
 tools/perf/builtin-kvm.c                      |   3 +
 tools/perf/builtin-kwork.c                    |   3 +
 tools/perf/builtin-lock.c                     |   3 +
 tools/perf/builtin-mem.c                      |   3 +
 tools/perf/builtin-sched.c                    |   3 +
 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 +-
 17 files changed, 370 insertions(+), 46 deletions(-)
 create mode 100644 tools/perf/Documentation/perf-check.txt
 create mode 100644 tools/perf/builtin-check.c

-- 
2.45.2

[PATCH v13 1/7] tools/lib/subcmd: Don't free the usage string

[Aditya Gupta]

Currently, commands which depend on 'parse_options_subcommand()' don't
show the usage string, and instead show '(null)'

    $ ./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)

'parse_options_subcommand()' is generally expected to initialise the usage
string, with information in the passed 'subcommands[]' array

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.

As discussed in [1], free the allocated usage string in the main function
itself, and don't reset usage string to NULL in parse_options_subcommand

With this change, the behaviour is restored.

    $ ./perf sched
        Usage: perf sched [<options>] {record|latency|map|replay|script|timehist}

           -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)

[1]: https://lore.kernel.org/linux-perf-users/htq5vhx6piet4nuq2mmhk7fs2bhfykv52dbppwxmo3s7du2odf@styd27tioc6e/

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Disha Goel <disgoel@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Fixes: 230a7a71f922 ("libsubcmd: Fix parse-options memory leak")
Acked-by: Namhyung Kim <namhyung@kernel.org>
Suggested-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>

---
Note:
This patch is independent of the series.

But I kept it along with the series, since the rest of the patches should
be applied only after this patch is applied (else the 'free' in
builtin-check.c will crash)
---
---
 tools/lib/subcmd/parse-options.c | 8 +++-----
 tools/perf/builtin-kmem.c        | 2 ++
 tools/perf/builtin-kvm.c         | 3 +++
 tools/perf/builtin-kwork.c       | 3 +++
 tools/perf/builtin-lock.c        | 3 +++
 tools/perf/builtin-mem.c         | 3 +++
 tools/perf/builtin-sched.c       | 3 +++
 7 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
index 4b60ec03b0bb..eb896d30545b 100644
--- a/tools/lib/subcmd/parse-options.c
+++ b/tools/lib/subcmd/parse-options.c
@@ -633,10 +633,11 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
 			const char *const subcommands[], const char *usagestr[], int flags)
 {
 	struct parse_opt_ctx_t ctx;
-	char *buf = NULL;
 
 	/* build usage string if it's not provided */
 	if (subcommands && !usagestr[0]) {
+		char *buf = NULL;
+
 		astrcatf(&buf, "%s %s [<options>] {", subcmd_config.exec_name, argv[0]);
 
 		for (int i = 0; subcommands[i]; i++) {
@@ -678,10 +679,7 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
 			astrcatf(&error_buf, "unknown switch `%c'", *ctx.opt);
 		usage_with_options(usagestr, options);
 	}
-	if (buf) {
-		usagestr[0] = NULL;
-		free(buf);
-	}
+
 	return parse_options_end(&ctx);
 }
 
diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index 6fd95be5032b..6b88b8b40784 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -2058,6 +2058,8 @@ int cmd_kmem(int argc, const char **argv)
 
 out_delete:
 	perf_session__delete(session);
+	/* free usage string allocated by parse_options_subcommand */
+	free((void *)kmem_usage[0]);
 
 	return ret;
 }
diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 71165036e4ca..988bef73bd09 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2187,5 +2187,8 @@ int cmd_kvm(int argc, const char **argv)
 	else
 		usage_with_options(kvm_usage, kvm_options);
 
+	/* free usage string allocated by parse_options_subcommand */
+	free((void *)kvm_usage[0]);
+
 	return 0;
 }
diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
index 56e3f3a5e03a..fd53838b5a78 100644
--- a/tools/perf/builtin-kwork.c
+++ b/tools/perf/builtin-kwork.c
@@ -2520,5 +2520,8 @@ int cmd_kwork(int argc, const char **argv)
 	} else
 		usage_with_options(kwork_usage, kwork_options);
 
+	/* free usage string allocated by parse_options_subcommand */
+	free((void *)kwork_usage[0]);
+
 	return 0;
 }
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 0253184b3b58..b25d50716e63 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -2713,6 +2713,9 @@ int cmd_lock(int argc, const char **argv)
 		usage_with_options(lock_usage, lock_options);
 	}
 
+	/* free usage string allocated by parse_options_subcommand */
+	free((void *)lock_usage[0]);
+
 	zfree(&lockhash_table);
 	return rc;
 }
diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
index 863fcd735dae..b7c1cf6d0e5a 100644
--- a/tools/perf/builtin-mem.c
+++ b/tools/perf/builtin-mem.c
@@ -517,5 +517,8 @@ int cmd_mem(int argc, const char **argv)
 	else
 		usage_with_options(mem_usage, mem_options);
 
+	/* free usage string allocated by parse_options_subcommand */
+	free((void *)mem_usage[0]);
+
 	return 0;
 }
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 8750b5f2d49b..d6acc53ae89e 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3805,5 +3805,8 @@ int cmd_sched(int argc, const char **argv)
 		usage_with_options(sched_usage, sched_options);
 	}
 
+	/* free usage string allocated by parse_options_subcommand */
+	free((void *)sched_usage[0]);
+
 	return 0;
 }
-- 
2.45.2

[PATCH v13 2/7] 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

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Disha Goel <disgoel@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: 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/Build                        |   1 +
 tools/perf/Documentation/perf-check.txt |  82 +++++++++++
 tools/perf/builtin-check.c              | 180 ++++++++++++++++++++++++
 tools/perf/builtin.h                    |  17 +++
 tools/perf/perf.c                       |   1 +
 5 files changed, 281 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..b081514d240a
--- /dev/null
+++ b/tools/perf/Documentation/perf-check.txt
@@ -0,0 +1,82 @@
+perf-check(1)
+===============
+
+NAME
+----
+perf-check - check if features are present in perf
+
+SYNOPSIS
+--------
+[verse]
+'perf check' [<options>]
+'perf check' {feature <feature_list>} [<options>]
+
+DESCRIPTION
+-----------
+With no subcommands given, 'perf check' command just prints the command
+usage 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
+                dwarf-unwind-support    /  HAVE_DWARF_UNWIND_SUPPORT
+                get_cpuid               /  HAVE_AUXTRACE_SUPPORT
+                libaudit                /  HAVE_LIBAUDIT_SUPPORT
+                libbfd                  /  HAVE_LIBBFD_SUPPORT
+                libcapstone             /  HAVE_LIBCAPSTONE_SUPPORT
+                libcrypto               /  HAVE_LIBCRYPTO_SUPPORT
+                libdw-dwarf-unwind      /  HAVE_DWARF_SUPPORT
+                libelf                  /  HAVE_LIBELF_SUPPORT
+                libnuma                 /  HAVE_LIBNUMA_SUPPORT
+                libopencsd              /  HAVE_CSTRACE_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
+                numa_num_possible_cpus  /  HAVE_LIBNUMA_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..16a04b5456d9
--- /dev/null
+++ b/tools/perf/builtin-check.c
@@ -0,0 +1,180 @@
+// 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_PARENT(check_options) };
+
+static const char *check_usage[] = { NULL, 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("Unknown feature '%s', please use 'perf version --build-options' to see which ones are available.\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 */
+	pr_err("Unknown subcommand: %s\n", argv[0]);
+	usage_with_options(check_usage, check_options);
+
+	/* free usage string allocated by parse_options_subcommand */
+	free((void *)check_usage[0]);
+
+	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

[PATCH v13 3/7] 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

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Disha Goel <disgoel@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
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 v13 4/7] tools/perf/tests: Update test_task_analyzer.sh to use perf check feature

[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.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Disha Goel <disgoel@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
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 v13 5/7] 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

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Disha Goel <disgoel@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
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

[PATCH v13 6/7] perf: Fix inconsistencies in feature names

[Aditya Gupta]

Fix two inconsistencies in feature names as discussed in [1]:
1. Rename "dwarf-unwind-support" to "dwarf-unwind"
2. 'get_cpuid' feature and 'HAVE_AUXTRACE_SUPPORT' names don't
   look related, change the feature name to 'auxtrace' to match the
   macro name, as 'get_cpuid' string is not used anywhere to check the
   feature presence

[1]: https://lore.kernel.org/linux-perf-users/ZoRw5we4HLSTZND6@x1/

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Disha Goel <disgoel@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/Documentation/perf-check.txt | 4 ++--
 tools/perf/builtin-check.c              | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/Documentation/perf-check.txt b/tools/perf/Documentation/perf-check.txt
index b081514d240a..10f69fb6850b 100644
--- a/tools/perf/Documentation/perf-check.txt
+++ b/tools/perf/Documentation/perf-check.txt
@@ -49,8 +49,8 @@ feature::
                 debuginfod              /  HAVE_DEBUGINFOD_SUPPORT
                 dwarf                   /  HAVE_DWARF_SUPPORT
                 dwarf_getlocations      /  HAVE_DWARF_GETLOCATIONS_SUPPORT
-                dwarf-unwind-support    /  HAVE_DWARF_UNWIND_SUPPORT
-                get_cpuid               /  HAVE_AUXTRACE_SUPPORT
+                dwarf-unwind            /  HAVE_DWARF_UNWIND_SUPPORT
+                auxtrace                /  HAVE_AUXTRACE_SUPPORT
                 libaudit                /  HAVE_LIBAUDIT_SUPPORT
                 libbfd                  /  HAVE_LIBBFD_SUPPORT
                 libcapstone             /  HAVE_LIBCAPSTONE_SUPPORT
diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
index 16a04b5456d9..0b76b6e42b78 100644
--- a/tools/perf/builtin-check.c
+++ b/tools/perf/builtin-check.c
@@ -29,8 +29,8 @@ struct feature_status supported_features[] = {
 	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("dwarf-unwind", HAVE_DWARF_UNWIND_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),
-- 
2.45.2

[PATCH v13 7/7] perf: Add more features to supported_features list

[Aditya Gupta]

As suggested in [1], add more feature names and corresponding names,
based on the information in Makefile.config

The entries were added after seeing the corresponding -D option which is
added to CFLAGS, based on the presence of a feature in Makefile.config,
such as:

    ifeq ($(feature-file-handle), 1)
      CFLAGS += -DHAVE_FILE_HANDLE
    endif

For above feature 'file-handle', corresponding entry has been added to
supported_features as below:

    FEATURE_STATUS("file-handle", HAVE_FILE_HANDLE)

[1]: https://lore.kernel.org/linux-perf-users/Zn7EvDbsnlbLXj4g@x1/

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Disha Goel <disgoel@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>

---
Could not add below features to the list:

* 'fortify-source': is a compiler option, doesn't add any feature MACRO
* 'stackprotector-all': is a compiler option, doesn't add any feature MACRO
* 'libtracefs': defines 'LIBTRACEFS_VERSION', not something like 'HAVE_LIBTRACEFS_SUPPORT'
* 'reallocarray': it's absence is known by 'COMPAT_NEED_REALLOCARRAY', but no flag for 'presence' of feature

Also, few names may feel inconsistent, but left as is:
* 'HAVE_GETTID': could be 'HAVE_GETTID_SUPPORT'
* 'LIBC_SUPPORT': could be 'HAVE_LIBC_SUPPORT'
* 'HAVE_LIBNUMA_SUPPORT': used by both 'numa_num_possible_cpus' and
  'libnuma', can remove one of those
* Currently some features have '_' in name, others have '-'
---
---
 tools/perf/Documentation/perf-check.txt | 78 ++++++++++++++++---------
 tools/perf/builtin-check.c              | 28 ++++++++-
 2 files changed, 77 insertions(+), 29 deletions(-)

diff --git a/tools/perf/Documentation/perf-check.txt b/tools/perf/Documentation/perf-check.txt
index 10f69fb6850b..cc1fe02ca01a 100644
--- a/tools/perf/Documentation/perf-check.txt
+++ b/tools/perf/Documentation/perf-check.txt
@@ -43,33 +43,57 @@ feature::
                 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
-                dwarf-unwind            /  HAVE_DWARF_UNWIND_SUPPORT
-                auxtrace                /  HAVE_AUXTRACE_SUPPORT
-                libaudit                /  HAVE_LIBAUDIT_SUPPORT
-                libbfd                  /  HAVE_LIBBFD_SUPPORT
-                libcapstone             /  HAVE_LIBCAPSTONE_SUPPORT
-                libcrypto               /  HAVE_LIBCRYPTO_SUPPORT
-                libdw-dwarf-unwind      /  HAVE_DWARF_SUPPORT
-                libelf                  /  HAVE_LIBELF_SUPPORT
-                libnuma                 /  HAVE_LIBNUMA_SUPPORT
-                libopencsd              /  HAVE_CSTRACE_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
-                numa_num_possible_cpus  /  HAVE_LIBNUMA_SUPPORT
-                syscall_table           /  HAVE_SYSCALL_TABLE_SUPPORT
-                zlib                    /  HAVE_ZLIB_SUPPORT
-                zstd                    /  HAVE_ZSTD_SUPPORT
+                aio                         /  HAVE_AIO_SUPPORT
+                auxtrace                    /  HAVE_AUXTRACE_SUPPORT
+                backtrace                   /  HAVE_BACKTRACE_SUPPORT
+                bpf                         /  HAVE_LIBBPF_SUPPORT
+                bpf_skeletons               /  HAVE_BPF_SKEL
+                debuginfod                  /  HAVE_DEBUGINFOD_SUPPORT
+                disassembler-four-args      /  DISASM_FOUR_ARGS_SIGNATURE
+                disassembler-init-styled    /  DISASM_INIT_STYLED
+                dwarf_getcfi                /  HAVE_DWARF_CFI_SUPPORT
+                dwarf_getlocations          /  HAVE_DWARF_GETLOCATIONS_SUPPORT
+                dwarf                       /  HAVE_DWARF_SUPPORT
+                dwarf-unwind                /  HAVE_DWARF_UNWIND_SUPPORT
+                eventfd                     /  HAVE_EVENTFD_SUPPORT
+                file-handle                 /  HAVE_FILE_HANDLE
+                get_current_dir_name        /  HAVE_GET_CURRENT_DIR_NAME
+                gettid                      /  HAVE_GETTID
+                glibc                       /  LIBC_SUPPORT
+                libaio                      /  HAVE_AIO_SUPPORT
+                libaudit                    /  HAVE_LIBAUDIT_SUPPORT
+                libbfd-buildid              /  HAVE_LIBBFD_BUILDID_SUPPORT
+                libbfd                      /  HAVE_LIBBFD_SUPPORT
+                libcap                      /  HAVE_LIBCAP_SUPPORT
+                libcapstone                 /  HAVE_LIBCAPSTONE_SUPPORT
+                libcrypto                   /  HAVE_LIBCRYPTO_SUPPORT
+                libdw-dwarf-unwind          /  HAVE_DWARF_SUPPORT
+                libelf-gelf_getnote         /  HAVE_GELF_GETNOTE_SUPPORT
+                libelf-getphdrnum           /  HAVE_ELF_GETPHDRNUM_SUPPORT
+                libelf-getshdrstrndx        /  HAVE_ELF_GETSHDRSTRNDX_SUPPORT
+                libelf                      /  HAVE_LIBELF_SUPPORT
+                libnuma                     /  HAVE_LIBNUMA_SUPPORT
+                libopencsd                  /  HAVE_CSTRACE_SUPPORT
+                libperl                     /  HAVE_LIBPERL_SUPPORT
+                libpfm4                     /  HAVE_LIBPFM
+                libpython                   /  HAVE_LIBPYTHON_SUPPORT
+                libslang                    /  HAVE_SLANG_SUPPORT
+                libslang-include-subdir     /  HAVE_SLANG_INCLUDE_SUBDIR
+                libtraceevent               /  HAVE_LIBTRACEEVENT
+                libunwind                   /  HAVE_LIBUNWIND_SUPPORT
+                libzstd                     /  HAVE_ZSTD_SUPPORT
+                lzma                        /  HAVE_LZMA_SUPPORT
+                numa_num_possible_cpus      /  HAVE_LIBNUMA_SUPPORT
+                pthread-attr-setaffinity-np /  HAVE_PTHREAD_ATTR_SETAFFINITY_NP
+                pthread-barrier             /  HAVE_PTHREAD_BARRIER
+                scandirat                   /  HAVE_SCANDIRAT_SUPPORT
+                sched_getcpu                /  HAVE_SCHED_GETCPU_SUPPORT
+                sdt                         /  HAVE_SDT_EVENT
+                setns                       /  HAVE_SETNS_SUPPORT
+                syscall_table               /  HAVE_SYSCALL_TABLE_SUPPORT
+                timerfd                     /  HAVE_TIMERFD_SUPPORT
+                zlib                        /  HAVE_ZLIB_SUPPORT
+                zstd                        /  HAVE_ZSTD_SUPPORT
 
 OPTIONS
 -------
diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
index 0b76b6e42b78..8c1deee80a68 100644
--- a/tools/perf/builtin-check.c
+++ b/tools/perf/builtin-check.c
@@ -24,18 +24,33 @@ static const char *check_feature_usage[] = {
 
 struct feature_status supported_features[] = {
 	FEATURE_STATUS("aio", HAVE_AIO_SUPPORT),
+	FEATURE_STATUS("auxtrace", HAVE_AUXTRACE_SUPPORT),
+	FEATURE_STATUS("backtrace", HAVE_BACKTRACE_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("disassembler-four-args", DISASM_FOUR_ARGS_SIGNATURE),
+	FEATURE_STATUS("disassembler-init-styled", DISASM_INIT_STYLED),
+	FEATURE_STATUS("dwarf_getcfi", HAVE_DWARF_CFI_SUPPORT),
 	FEATURE_STATUS("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
+	FEATURE_STATUS("dwarf", HAVE_DWARF_SUPPORT),
 	FEATURE_STATUS("dwarf-unwind", HAVE_DWARF_UNWIND_SUPPORT),
-	FEATURE_STATUS("auxtrace", HAVE_AUXTRACE_SUPPORT),
+	FEATURE_STATUS("eventfd", HAVE_EVENTFD_SUPPORT),
+	FEATURE_STATUS("file-handle", HAVE_FILE_HANDLE),
+	FEATURE_STATUS("get_current_dir_name", HAVE_GET_CURRENT_DIR_NAME),
+	FEATURE_STATUS("gettid", HAVE_GETTID),
+	FEATURE_STATUS("glibc", LIBC_SUPPORT),
+	FEATURE_STATUS("libaio", HAVE_AIO_SUPPORT),
 	FEATURE_STATUS("libaudit", HAVE_LIBAUDIT_SUPPORT),
+	FEATURE_STATUS("libbfd-buildid", HAVE_LIBBFD_BUILDID_SUPPORT),
 	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
+	FEATURE_STATUS("libcap", HAVE_LIBCAP_SUPPORT),
 	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),
 	FEATURE_STATUS("libcrypto", HAVE_LIBCRYPTO_SUPPORT),
 	FEATURE_STATUS("libdw-dwarf-unwind", HAVE_DWARF_SUPPORT),
+	FEATURE_STATUS("libelf-gelf_getnote", HAVE_GELF_GETNOTE_SUPPORT),
+	FEATURE_STATUS("libelf-getphdrnum", HAVE_ELF_GETPHDRNUM_SUPPORT),
+	FEATURE_STATUS("libelf-getshdrstrndx", HAVE_ELF_GETSHDRSTRNDX_SUPPORT),
 	FEATURE_STATUS("libelf", HAVE_LIBELF_SUPPORT),
 	FEATURE_STATUS("libnuma", HAVE_LIBNUMA_SUPPORT),
 	FEATURE_STATUS("libopencsd", HAVE_CSTRACE_SUPPORT),
@@ -43,11 +58,20 @@ struct feature_status supported_features[] = {
 	FEATURE_STATUS("libpfm4", HAVE_LIBPFM),
 	FEATURE_STATUS("libpython", HAVE_LIBPYTHON_SUPPORT),
 	FEATURE_STATUS("libslang", HAVE_SLANG_SUPPORT),
+	FEATURE_STATUS("libslang-include-subdir", HAVE_SLANG_INCLUDE_SUBDIR),
 	FEATURE_STATUS("libtraceevent", HAVE_LIBTRACEEVENT),
 	FEATURE_STATUS("libunwind", HAVE_LIBUNWIND_SUPPORT),
+	FEATURE_STATUS("libzstd", HAVE_ZSTD_SUPPORT),
 	FEATURE_STATUS("lzma", HAVE_LZMA_SUPPORT),
 	FEATURE_STATUS("numa_num_possible_cpus", HAVE_LIBNUMA_SUPPORT),
+	FEATURE_STATUS("pthread-attr-setaffinity-np", HAVE_PTHREAD_ATTR_SETAFFINITY_NP),
+	FEATURE_STATUS("pthread-barrier", HAVE_PTHREAD_BARRIER),
+	FEATURE_STATUS("scandirat", HAVE_SCANDIRAT_SUPPORT),
+	FEATURE_STATUS("sched_getcpu", HAVE_SCHED_GETCPU_SUPPORT),
+	FEATURE_STATUS("sdt", HAVE_SDT_EVENT),
+	FEATURE_STATUS("setns", HAVE_SETNS_SUPPORT),
 	FEATURE_STATUS("syscall_table", HAVE_SYSCALL_TABLE_SUPPORT),
+	FEATURE_STATUS("timerfd", HAVE_TIMERFD_SUPPORT),
 	FEATURE_STATUS("zlib", HAVE_ZLIB_SUPPORT),
 	FEATURE_STATUS("zstd", HAVE_ZSTD_SUPPORT),
 
-- 
2.45.2

Re: [PATCH v13 1/7] tools/lib/subcmd: Don't free the usage string

[Aditya Gupta]

Hello,

This patch is independent of the series. I had also sent this patch as a standalone single patch yesterday.

But I kept it along with the series only since, the rest of the patches should
be applied only after this patch is applied (else the 'free' in
builtin-check.c will crash)


Thanks,
Aditya Gupta

On 18/07/24 14:29, Aditya Gupta wrote:
> Currently, commands which depend on 'parse_options_subcommand()' don't
> show the usage string, and instead show '(null)'
>
>      $ ./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)
>
> 'parse_options_subcommand()' is generally expected to initialise the usage
> string, with information in the passed 'subcommands[]' array
>
> 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.
>
> As discussed in [1], free the allocated usage string in the main function
> itself, and don't reset usage string to NULL in parse_options_subcommand
>
> With this change, the behaviour is restored.
>
>      $ ./perf sched
>          Usage: perf sched [<options>] {record|latency|map|replay|script|timehist}
>
>             -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)
>
> [1]: https://lore.kernel.org/linux-perf-users/htq5vhx6piet4nuq2mmhk7fs2bhfykv52dbppwxmo3s7du2odf@styd27tioc6e/
>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Cc: Disha Goel <disgoel@linux.vnet.ibm.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Kajol Jain <kjain@linux.ibm.com>
> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Fixes: 230a7a71f922 ("libsubcmd: Fix parse-options memory leak")
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> Suggested-by: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
>
> ---
> Note:
> This patch is independent of the series.
>
> But I kept it along with the series, since the rest of the patches should
> be applied only after this patch is applied (else the 'free' in
> builtin-check.c will crash)
> ---
> ---
>   tools/lib/subcmd/parse-options.c | 8 +++-----
>   tools/perf/builtin-kmem.c        | 2 ++
>   tools/perf/builtin-kvm.c         | 3 +++
>   tools/perf/builtin-kwork.c       | 3 +++
>   tools/perf/builtin-lock.c        | 3 +++
>   tools/perf/builtin-mem.c         | 3 +++
>   tools/perf/builtin-sched.c       | 3 +++
>   7 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
> index 4b60ec03b0bb..eb896d30545b 100644
> --- a/tools/lib/subcmd/parse-options.c
> +++ b/tools/lib/subcmd/parse-options.c
> @@ -633,10 +633,11 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
>   			const char *const subcommands[], const char *usagestr[], int flags)
>   {
>   	struct parse_opt_ctx_t ctx;
> -	char *buf = NULL;
>   
>   	/* build usage string if it's not provided */
>   	if (subcommands && !usagestr[0]) {
> +		char *buf = NULL;
> +
>   		astrcatf(&buf, "%s %s [<options>] {", subcmd_config.exec_name, argv[0]);
>   
>   		for (int i = 0; subcommands[i]; i++) {
> @@ -678,10 +679,7 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
>   			astrcatf(&error_buf, "unknown switch `%c'", *ctx.opt);
>   		usage_with_options(usagestr, options);
>   	}
> -	if (buf) {
> -		usagestr[0] = NULL;
> -		free(buf);
> -	}
> +
>   	return parse_options_end(&ctx);
>   }
>   
> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> index 6fd95be5032b..6b88b8b40784 100644
> --- a/tools/perf/builtin-kmem.c
> +++ b/tools/perf/builtin-kmem.c
> @@ -2058,6 +2058,8 @@ int cmd_kmem(int argc, const char **argv)
>   
>   out_delete:
>   	perf_session__delete(session);
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)kmem_usage[0]);
>   
>   	return ret;
>   }
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 71165036e4ca..988bef73bd09 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -2187,5 +2187,8 @@ int cmd_kvm(int argc, const char **argv)
>   	else
>   		usage_with_options(kvm_usage, kvm_options);
>   
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)kvm_usage[0]);
> +
>   	return 0;
>   }
> diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
> index 56e3f3a5e03a..fd53838b5a78 100644
> --- a/tools/perf/builtin-kwork.c
> +++ b/tools/perf/builtin-kwork.c
> @@ -2520,5 +2520,8 @@ int cmd_kwork(int argc, const char **argv)
>   	} else
>   		usage_with_options(kwork_usage, kwork_options);
>   
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)kwork_usage[0]);
> +
>   	return 0;
>   }
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 0253184b3b58..b25d50716e63 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -2713,6 +2713,9 @@ int cmd_lock(int argc, const char **argv)
>   		usage_with_options(lock_usage, lock_options);
>   	}
>   
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)lock_usage[0]);
> +
>   	zfree(&lockhash_table);
>   	return rc;
>   }
> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
> index 863fcd735dae..b7c1cf6d0e5a 100644
> --- a/tools/perf/builtin-mem.c
> +++ b/tools/perf/builtin-mem.c
> @@ -517,5 +517,8 @@ int cmd_mem(int argc, const char **argv)
>   	else
>   		usage_with_options(mem_usage, mem_options);
>   
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)mem_usage[0]);
> +
>   	return 0;
>   }
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 8750b5f2d49b..d6acc53ae89e 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -3805,5 +3805,8 @@ int cmd_sched(int argc, const char **argv)
>   		usage_with_options(sched_usage, sched_options);
>   	}
>   
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)sched_usage[0]);
> +
>   	return 0;
>   }

Re: [PATCH v13 0/7] Introduce perf check subcommand

[Aditya Gupta]

Linux-ci build test results for all patches (all green, except some 
g5_defconfig in kernel+qemu, which is red irrespective of this series):  
https://github.com/adi-g15-ibm/linux-ci/actions


Thanks,

Aditya Gupta


On 18/07/24 14:29, Aditya Gupta wrote:
> 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-v13
>
> Changelog
> =========
>
> v13:
> + patch #1: add fix for parse_options_subcommand not setting usage string
> + patch #2: if unknown feature name passed, print "please use 'perf version
>    --build-options' to see which ones are available"
> + patch #6: fix inconsistency in feature names (dwarf-unwind-support & get_cpuid)
> + patch #7: add more features to feature_list
>
> 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/
>
> Aditya Gupta (6):
>    tools/lib/subcmd: Don't free the usage string
>    perf check: Introduce 'check' subcommand
>    perf version: Update --build-options to use 'supported_features' array
>    tools/perf/tests: Update test_task_analyzer.sh to use perf check
>      feature
>    perf: Fix inconsistencies in feature names
>    perf: Add more features to supported_features list
>
> Athira Rajeev (1):
>    tools/perf/tests: Update probe_vfs_getname.sh script to use perf check
>      feature
>
>   tools/lib/subcmd/parse-options.c              |   8 +-
>   tools/perf/Build                              |   1 +
>   tools/perf/Documentation/perf-check.txt       | 106 +++++++++
>   tools/perf/builtin-check.c                    | 205 ++++++++++++++++++
>   tools/perf/builtin-kmem.c                     |   2 +
>   tools/perf/builtin-kvm.c                      |   3 +
>   tools/perf/builtin-kwork.c                    |   3 +
>   tools/perf/builtin-lock.c                     |   3 +
>   tools/perf/builtin-mem.c                      |   3 +
>   tools/perf/builtin-sched.c                    |   3 +
>   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 +-
>   17 files changed, 370 insertions(+), 46 deletions(-)
>   create mode 100644 tools/perf/Documentation/perf-check.txt
>   create mode 100644 tools/perf/builtin-check.c
>

Re: [PATCH v13 1/7] tools/lib/subcmd: Don't free the usage string

[Arnaldo Carvalho de Melo]

On Thu, Jul 18, 2024 at 02:29:51PM +0530, Aditya Gupta wrote:
> Currently, commands which depend on 'parse_options_subcommand()' don't
> show the usage string, and instead show '(null)'
> 
>     $ ./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)
> 
> 'parse_options_subcommand()' is generally expected to initialise the usage
> string, with information in the passed 'subcommands[]' array
> 
> 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.
> 
> As discussed in [1], free the allocated usage string in the main function
> itself, and don't reset usage string to NULL in parse_options_subcommand
> 
> With this change, the behaviour is restored.
> 
>     $ ./perf sched
>         Usage: perf sched [<options>] {record|latency|map|replay|script|timehist}
> 
>            -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)
> 
> [1]: https://lore.kernel.org/linux-perf-users/htq5vhx6piet4nuq2mmhk7fs2bhfykv52dbppwxmo3s7du2odf@styd27tioc6e/
> 
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Cc: Disha Goel <disgoel@linux.vnet.ibm.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Kajol Jain <kjain@linux.ibm.com>
> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Fixes: 230a7a71f922 ("libsubcmd: Fix parse-options memory leak")
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> Suggested-by: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> 
> ---
> Note:
> This patch is independent of the series.
> 
> But I kept it along with the series, since the rest of the patches should
> be applied only after this patch is applied (else the 'free' in
> builtin-check.c will crash)
> ---
> ---
>  tools/lib/subcmd/parse-options.c | 8 +++-----
>  tools/perf/builtin-kmem.c        | 2 ++
>  tools/perf/builtin-kvm.c         | 3 +++
>  tools/perf/builtin-kwork.c       | 3 +++
>  tools/perf/builtin-lock.c        | 3 +++
>  tools/perf/builtin-mem.c         | 3 +++
>  tools/perf/builtin-sched.c       | 3 +++
>  7 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
> index 4b60ec03b0bb..eb896d30545b 100644
> --- a/tools/lib/subcmd/parse-options.c
> +++ b/tools/lib/subcmd/parse-options.c
> @@ -633,10 +633,11 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
>  			const char *const subcommands[], const char *usagestr[], int flags)
>  {
>  	struct parse_opt_ctx_t ctx;
> -	char *buf = NULL;
>  
>  	/* build usage string if it's not provided */
>  	if (subcommands && !usagestr[0]) {
> +		char *buf = NULL;
> +
>  		astrcatf(&buf, "%s %s [<options>] {", subcmd_config.exec_name, argv[0]);
>  
>  		for (int i = 0; subcommands[i]; i++) {
> @@ -678,10 +679,7 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
>  			astrcatf(&error_buf, "unknown switch `%c'", *ctx.opt);
>  		usage_with_options(usagestr, options);
>  	}
> -	if (buf) {
> -		usagestr[0] = NULL;
> -		free(buf);
> -	}
> +
>  	return parse_options_end(&ctx);
>  }
>  
> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> index 6fd95be5032b..6b88b8b40784 100644
> --- a/tools/perf/builtin-kmem.c
> +++ b/tools/perf/builtin-kmem.c
> @@ -2058,6 +2058,8 @@ int cmd_kmem(int argc, const char **argv)
>  
>  out_delete:
>  	perf_session__delete(session);
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)kmem_usage[0]);

Can we instead add a parse_options_exit(const char *usagestr[]) that
does the free?

So that we don't expose these internal details of libsubcmd and if
something else needs to be done at exit time, thenm that is the place to
add.

Thanks,

- Arnaldo
  
>  	return ret;
>  }
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 71165036e4ca..988bef73bd09 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -2187,5 +2187,8 @@ int cmd_kvm(int argc, const char **argv)
>  	else
>  		usage_with_options(kvm_usage, kvm_options);
>  
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)kvm_usage[0]);
> +
>  	return 0;
>  }
> diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
> index 56e3f3a5e03a..fd53838b5a78 100644
> --- a/tools/perf/builtin-kwork.c
> +++ b/tools/perf/builtin-kwork.c
> @@ -2520,5 +2520,8 @@ int cmd_kwork(int argc, const char **argv)
>  	} else
>  		usage_with_options(kwork_usage, kwork_options);
>  
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)kwork_usage[0]);
> +
>  	return 0;
>  }
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 0253184b3b58..b25d50716e63 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -2713,6 +2713,9 @@ int cmd_lock(int argc, const char **argv)
>  		usage_with_options(lock_usage, lock_options);
>  	}
>  
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)lock_usage[0]);
> +
>  	zfree(&lockhash_table);
>  	return rc;
>  }
> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
> index 863fcd735dae..b7c1cf6d0e5a 100644
> --- a/tools/perf/builtin-mem.c
> +++ b/tools/perf/builtin-mem.c
> @@ -517,5 +517,8 @@ int cmd_mem(int argc, const char **argv)
>  	else
>  		usage_with_options(mem_usage, mem_options);
>  
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)mem_usage[0]);
> +
>  	return 0;
>  }
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 8750b5f2d49b..d6acc53ae89e 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -3805,5 +3805,8 @@ int cmd_sched(int argc, const char **argv)
>  		usage_with_options(sched_usage, sched_options);
>  	}
>  
> +	/* free usage string allocated by parse_options_subcommand */
> +	free((void *)sched_usage[0]);
> +
>  	return 0;
>  }
> -- 
> 2.45.2

Re: [PATCH v13 1/7] tools/lib/subcmd: Don't free the usage string

[Aditya Gupta]

Hi Arnaldo,

On 26/07/24 19:53, Arnaldo Carvalho de Melo wrote:
> On Thu, Jul 18, 2024 at 02:29:51PM +0530, Aditya Gupta wrote:
>> Currently, commands which depend on 'parse_options_subcommand()' don't
>> show the usage string, and instead show '(null)'
>>
>>      $ ./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)
>>
>> 'parse_options_subcommand()' is generally expected to initialise the usage
>> string, with information in the passed 'subcommands[]' array
>>
>> 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.
>>
>> As discussed in [1], free the allocated usage string in the main function
>> itself, and don't reset usage string to NULL in parse_options_subcommand
>>
>> With this change, the behaviour is restored.
>>
>>      $ ./perf sched
>>          Usage: perf sched [<options>] {record|latency|map|replay|script|timehist}
>>
>>             -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)
>>
>> [1]: https://lore.kernel.org/linux-perf-users/htq5vhx6piet4nuq2mmhk7fs2bhfykv52dbppwxmo3s7du2odf@styd27tioc6e/
>>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> Cc: Disha Goel <disgoel@linux.vnet.ibm.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Ian Rogers <irogers@google.com>
>> Cc: Kajol Jain <kjain@linux.ibm.com>
>> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Fixes: 230a7a71f922 ("libsubcmd: Fix parse-options memory leak")
>> Acked-by: Namhyung Kim <namhyung@kernel.org>
>> Suggested-by: Namhyung Kim <namhyung@kernel.org>
>> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
>>
>> ---
>> Note:
>> This patch is independent of the series.
>>
>> But I kept it along with the series, since the rest of the patches should
>> be applied only after this patch is applied (else the 'free' in
>> builtin-check.c will crash)
>> ---
>> ---
>>   tools/lib/subcmd/parse-options.c | 8 +++-----
>>   tools/perf/builtin-kmem.c        | 2 ++
>>   tools/perf/builtin-kvm.c         | 3 +++
>>   tools/perf/builtin-kwork.c       | 3 +++
>>   tools/perf/builtin-lock.c        | 3 +++
>>   tools/perf/builtin-mem.c         | 3 +++
>>   tools/perf/builtin-sched.c       | 3 +++
>>   7 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
>> index 4b60ec03b0bb..eb896d30545b 100644
>> --- a/tools/lib/subcmd/parse-options.c
>> +++ b/tools/lib/subcmd/parse-options.c
>> @@ -633,10 +633,11 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
>>   			const char *const subcommands[], const char *usagestr[], int flags)
>>   {
>>   	struct parse_opt_ctx_t ctx;
>> -	char *buf = NULL;
>>   
>>   	/* build usage string if it's not provided */
>>   	if (subcommands && !usagestr[0]) {
>> +		char *buf = NULL;
>> +
>>   		astrcatf(&buf, "%s %s [<options>] {", subcmd_config.exec_name, argv[0]);
>>   
>>   		for (int i = 0; subcommands[i]; i++) {
>> @@ -678,10 +679,7 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
>>   			astrcatf(&error_buf, "unknown switch `%c'", *ctx.opt);
>>   		usage_with_options(usagestr, options);
>>   	}
>> -	if (buf) {
>> -		usagestr[0] = NULL;
>> -		free(buf);
>> -	}
>> +
>>   	return parse_options_end(&ctx);
>>   }
>>   
>> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
>> index 6fd95be5032b..6b88b8b40784 100644
>> --- a/tools/perf/builtin-kmem.c
>> +++ b/tools/perf/builtin-kmem.c
>> @@ -2058,6 +2058,8 @@ int cmd_kmem(int argc, const char **argv)
>>   
>>   out_delete:
>>   	perf_session__delete(session);
>> +	/* free usage string allocated by parse_options_subcommand */
>> +	free((void *)kmem_usage[0]);
> Can we instead add a parse_options_exit(const char *usagestr[]) that
> does the free?
>
> So that we don't expose these internal details of libsubcmd and if
> something else needs to be done at exit time, thenm that is the place to
> add.

Sure that would be better, but might require a flag somewhere, to know 
that `usagestr` was allocated by 'parse_options_subcommand', and not 
statically present. (so that we don't run free on a statically allocated 
string).

Currently i don't see where we can keep such thing in subcmd, will try 
to see how I can know this, or maybe some magic inside 
'parse_options_exit' to check if string is NOT NULL, AND, dynamically 
allocated, then only we free it ?


Thanks,

Aditya Gupta

> Thanks,
>
> - Arnaldo
>    
>>   	return ret;
>>   }
>> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
>> index 71165036e4ca..988bef73bd09 100644
>> --- a/tools/perf/builtin-kvm.c
>> +++ b/tools/perf/builtin-kvm.c
>> @@ -2187,5 +2187,8 @@ int cmd_kvm(int argc, const char **argv)
>>   	else
>>   		usage_with_options(kvm_usage, kvm_options);
>>   
>> +	/* free usage string allocated by parse_options_subcommand */
>> +	free((void *)kvm_usage[0]);
>> +
>>   	return 0;
>>   }
>> diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
>> index 56e3f3a5e03a..fd53838b5a78 100644
>> --- a/tools/perf/builtin-kwork.c
>> +++ b/tools/perf/builtin-kwork.c
>> @@ -2520,5 +2520,8 @@ int cmd_kwork(int argc, const char **argv)
>>   	} else
>>   		usage_with_options(kwork_usage, kwork_options);
>>   
>> +	/* free usage string allocated by parse_options_subcommand */
>> +	free((void *)kwork_usage[0]);
>> +
>>   	return 0;
>>   }
>> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
>> index 0253184b3b58..b25d50716e63 100644
>> --- a/tools/perf/builtin-lock.c
>> +++ b/tools/perf/builtin-lock.c
>> @@ -2713,6 +2713,9 @@ int cmd_lock(int argc, const char **argv)
>>   		usage_with_options(lock_usage, lock_options);
>>   	}
>>   
>> +	/* free usage string allocated by parse_options_subcommand */
>> +	free((void *)lock_usage[0]);
>> +
>>   	zfree(&lockhash_table);
>>   	return rc;
>>   }
>> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
>> index 863fcd735dae..b7c1cf6d0e5a 100644
>> --- a/tools/perf/builtin-mem.c
>> +++ b/tools/perf/builtin-mem.c
>> @@ -517,5 +517,8 @@ int cmd_mem(int argc, const char **argv)
>>   	else
>>   		usage_with_options(mem_usage, mem_options);
>>   
>> +	/* free usage string allocated by parse_options_subcommand */
>> +	free((void *)mem_usage[0]);
>> +
>>   	return 0;
>>   }
>> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
>> index 8750b5f2d49b..d6acc53ae89e 100644
>> --- a/tools/perf/builtin-sched.c
>> +++ b/tools/perf/builtin-sched.c
>> @@ -3805,5 +3805,8 @@ int cmd_sched(int argc, const char **argv)
>>   		usage_with_options(sched_usage, sched_options);
>>   	}
>>   
>> +	/* free usage string allocated by parse_options_subcommand */
>> +	free((void *)sched_usage[0]);
>> +
>>   	return 0;
>>   }
>> -- 
>> 2.45.2

Re: [PATCH v13 0/7] Introduce perf check subcommand

[Arnaldo Carvalho de Melo]

On Thu, Jul 18, 2024 at 02:29:50PM +0530, Aditya Gupta wrote:
> 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:

I tried to resume reviewing/testing but:

Total patches: 7
---
Cover: ./v13_20240718_adityag_introduce_perf_check_subcommand.cover
 Link: https://lore.kernel.org/r/20240718085957.550858-1-adityag@linux.ibm.com
 Base: not specified
       git am ./v13_20240718_adityag_introduce_perf_check_subcommand.mbx
⬢[acme@toolbox perf-tools-next]$        git am ./v13_20240718_adityag_introduce_perf_check_subcommand.mbx
Applying: tools/lib/subcmd: Don't free the usage string
Applying: perf check: Introduce 'check' subcommand
Applying: perf version: Update --build-options to use 'supported_features' array
error: patch failed: tools/perf/builtin-version.c:46
error: tools/perf/builtin-version.c: patch does not apply
Patch failed at 0003 perf version: Update --build-options to use 'supported_features' array
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
⬢[acme@toolbox perf-tools-next]$

Re: [PATCH v13 0/7] Introduce perf check subcommand

[Aditya Gupta]

Hi Arnaldo,

On 03/09/24 21:00, Arnaldo Carvalho de Melo wrote:
> On Thu, Jul 18, 2024 at 02:29:50PM +0530, Aditya Gupta wrote:
>> 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:
> I tried to resume reviewing/testing but:
>
> Total patches: 7
> ---
> Cover: ./v13_20240718_adityag_introduce_perf_check_subcommand.cover
>   Link: https://lore.kernel.org/r/20240718085957.550858-1-adityag@linux.ibm.com
>   Base: not specified
>         git am ./v13_20240718_adityag_introduce_perf_check_subcommand.mbx
> ⬢[acme@toolbox perf-tools-next]$        git am ./v13_20240718_adityag_introduce_perf_check_subcommand.mbx
> Applying: tools/lib/subcmd: Don't free the usage string
> Applying: perf check: Introduce 'check' subcommand
> Applying: perf version: Update --build-options to use 'supported_features' array
> error: patch failed: tools/perf/builtin-version.c:46
> error: tools/perf/builtin-version.c: patch does not apply
> Patch failed at 0003 perf version: Update --build-options to use 'supported_features' array
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> hint: When you have resolved this problem, run "git am --continue".
> hint: If you prefer to skip this patch, run "git am --skip" instead.
> hint: To restore the original branch and stop patching, run "git am --abort".
> hint: Disable this message with "git config advice.mergeConflict false"
> ⬢[acme@toolbox perf-tools-next]$

Maybe I need to rebase it now. I will rebase and send it again.


Thanks,

Aditya Gupta