linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/4] Introduce perf check subcommand
@ 2023-11-16 21:34 Aditya Gupta
  2023-11-16 21:34 ` [PATCH v7 1/4] perf check: introduce " Aditya Gupta
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-11-16 21:34 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

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"

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

    or

    perf check --feature libtraceevent

    or

    perf check --feature libtraceevent,libbpf_support

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

Git tree
========

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

Changelog
=========
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 (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       |  59 +++++++++
 tools/perf/builtin-check.c                    | 122 ++++++++++++++++++
 tools/perf/builtin-version.c                  |  39 ++----
 tools/perf/builtin.h                          |  18 +++
 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, 221 insertions(+), 37 deletions(-)
 create mode 100644 tools/perf/Documentation/perf-check.txt
 create mode 100644 tools/perf/builtin-check.c

-- 
2.41.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v7 1/4] perf check: introduce check subcommand
  2023-11-16 21:34 [PATCH v7 0/4] Introduce perf check subcommand Aditya Gupta
@ 2023-11-16 21:34 ` Aditya Gupta
  2023-12-04  0:24   ` Arnaldo Carvalho de Melo
  2023-11-16 21:34 ` [PATCH v7 2/4] perf version: update --build-options to use 'supported_features' array Aditya Gupta
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Aditya Gupta @ 2023-11-16 21:34 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

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 1 only
if both libtraceevent and libbpf_support are enabled, else 0 in all other cases

    perf check --feature libtraceevent,libbpf_support

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

Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/Build                        |   1 +
 tools/perf/Documentation/perf-check.txt |  65 +++++++++++
 tools/perf/builtin-check.c              | 146 ++++++++++++++++++++++++
 tools/perf/builtin.h                    |  18 +++
 tools/perf/perf.c                       |   1 +
 5 files changed, 231 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 aa7623622834..a55a797c1b5f 100644
--- a/tools/perf/Build
+++ b/tools/perf/Build
@@ -1,5 +1,6 @@
 perf-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..dfb1dddd0ac3
--- /dev/null
+++ b/tools/perf/Documentation/perf-check.txt
@@ -0,0 +1,65 @@
+perf-check(1)
+===============
+
+NAME
+----
+perf-check - check features in perf
+
+SYNOPSIS
+--------
+'perf check' [<options>]
+
+DESCRIPTION
+-----------
+With no options given, the 'perf check' just prints the perf version
+on the standard output.
+
+If the option '--feature' is given, 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.
+
+OPTIONS
+-------
+-q::
+		Do not print any messages or warnings
+
+--feature::
+        Print whether feature(s) is compiled-in or not. A feature name/macro is
+        required to be passed after this flag
+
+        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
+        compiled-in.
+
+        Example Usage:
+                perf check --feature libtraceevent
+                perf check --feature HAVE_LIBTRACEEVENT
+                perf check --feature libtraceevent,libbpf_support
+
+        Supported feature names/macro:
+                dwarf      /  HAVE_DWARF_SUPPORT
+                dwarf_getlocations  /  HAVE_DWARF_GETLOCATIONS_SUPPORT
+                libaudit   /  HAVE_LIBAUDIT_SUPPORT
+                syscall_table       /  HAVE_SYSCALL_TABLE_SUPPORT
+                libbfd     /  HAVE_LIBBFD_SUPPORT
+                debuginfod /  HAVE_DEBUGINFOD_SUPPORT
+                libelf     /  HAVE_LIBELF_SUPPORT
+                libnuma    /  HAVE_LIBNUMA_SUPPORT
+                numa_num_possible_cpus  /  HAVE_LIBNUMA_SUPPORT
+                libperl    /  HAVE_LIBPERL_SUPPORT
+                libpython  /  HAVE_LIBPYTHON_SUPPORT
+                libslang   /  HAVE_SLANG_SUPPORT
+                libcrypto  /  HAVE_LIBCRYPTO_SUPPORT
+                libunwind  /  HAVE_LIBUNWIND_SUPPORT
+                libdw-dwarf-unwind  /  HAVE_DWARF_SUPPORT
+                zlib       /  HAVE_ZLIB_SUPPORT
+                lzma       /  HAVE_LZMA_SUPPORT
+                get_cpuid  /  HAVE_AUXTRACE_SUPPORT
+                bpf        /  HAVE_LIBBPF_SUPPORT
+                aio        /  HAVE_AIO_SUPPORT
+                zstd       /  HAVE_ZSTD_SUPPORT
+                libpfm4    /  HAVE_LIBPFM
+                libtraceevent      /  HAVE_LIBTRACEEVENT
+                bpf_skeletons      /  HAVE_BPF_SKEL
diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
new file mode 100644
index 000000000000..25373f055121
--- /dev/null
+++ b/tools/perf/builtin-check.c
@@ -0,0 +1,146 @@
+// 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>
+
+struct check {
+	const char *feature;
+};
+
+static struct check check;
+
+static struct option check_options[] = {
+	OPT_STRING(0, "feature", &check.feature, NULL, "check if feature(s) is built in"),
+	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
+	OPT_END(),
+};
+
+static const char * const check_usage[] = {
+	"perf check [<options>]",
+	NULL
+};
+
+struct feature_support supported_features[] = {
+	FEATURE_SUPPORT("dwarf", HAVE_DWARF_SUPPORT),
+	FEATURE_SUPPORT("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
+	FEATURE_SUPPORT("libaudit", HAVE_LIBAUDIT_SUPPORT),
+	FEATURE_SUPPORT("syscall_table", HAVE_SYSCALL_TABLE_SUPPORT),
+	FEATURE_SUPPORT("libbfd", HAVE_LIBBFD_SUPPORT),
+	FEATURE_SUPPORT("debuginfod", HAVE_DEBUGINFOD_SUPPORT),
+	FEATURE_SUPPORT("libelf", HAVE_LIBELF_SUPPORT),
+	FEATURE_SUPPORT("libnuma", HAVE_LIBNUMA_SUPPORT),
+	FEATURE_SUPPORT("numa_num_possible_cpus", HAVE_LIBNUMA_SUPPORT),
+	FEATURE_SUPPORT("libperl", HAVE_LIBPERL_SUPPORT),
+	FEATURE_SUPPORT("libpython", HAVE_LIBPYTHON_SUPPORT),
+	FEATURE_SUPPORT("libslang", HAVE_SLANG_SUPPORT),
+	FEATURE_SUPPORT("libcrypto", HAVE_LIBCRYPTO_SUPPORT),
+	FEATURE_SUPPORT("libunwind", HAVE_LIBUNWIND_SUPPORT),
+	FEATURE_SUPPORT("libdw-dwarf-unwind", HAVE_DWARF_SUPPORT),
+	FEATURE_SUPPORT("zlib", HAVE_ZLIB_SUPPORT),
+	FEATURE_SUPPORT("lzma", HAVE_LZMA_SUPPORT),
+	FEATURE_SUPPORT("get_cpuid", HAVE_AUXTRACE_SUPPORT),
+	FEATURE_SUPPORT("bpf", HAVE_LIBBPF_SUPPORT),
+	FEATURE_SUPPORT("aio", HAVE_AIO_SUPPORT),
+	FEATURE_SUPPORT("zstd", HAVE_ZSTD_SUPPORT),
+	FEATURE_SUPPORT("libpfm4", HAVE_LIBPFM),
+	FEATURE_SUPPORT("libtraceevent", HAVE_LIBTRACEEVENT),
+	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL),
+
+	/* this should remain at end, to know the array end */
+	FEATURE_SUPPORT(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(" ]");
+}
+
+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 ((strcmp(feature, supported_features[i].name) == 0) ||
+		    (strcmp(feature, supported_features[i].macro) == 0)) {
+			if (!quiet)
+				STATUS(supported_features[i]);
+			return supported_features[i].is_builtin;
+		}
+	}
+
+	if (!quiet)
+		color_fprintf(stdout, PERF_COLOR_RED, "Feature not known: '%s'\n", feature);
+
+	return 0;
+}
+
+int cmd_check(int argc, const char **argv)
+{
+	char *feature_list;
+	char *feature_name;
+	int feature_enabled;
+
+	argc = parse_options(argc, argv, check_options, check_usage,
+			     PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (check.feature) {
+		/* check.feature can be a single feature name, or a comma-separated list
+		 * of feature names.
+		 * eg. check.feature can be "libtraceevent" or
+		 * "libtraceevent,libbpf_support" 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 check.feature will get modified due to strtok and str_trim
+		 */
+		feature_enabled = 1;
+		feature_list = malloc((strlen(check.feature)+1) * sizeof(char));
+		strncpy(feature_list, check.feature, strlen(check.feature)+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;
+	}
+
+	return 0;
+}
diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
index f2ab5bae2150..9f895981bc9e 100644
--- a/tools/perf/builtin.h
+++ b/tools/perf/builtin.h
@@ -2,6 +2,23 @@
 #ifndef BUILTIN_H
 #define BUILTIN_H
 
+#include <stddef.h>
+#include <linux/compiler.h>
+#include <tools/config.h>
+
+struct feature_support {
+	const char *name;
+	const char *macro;
+	int is_builtin;
+};
+
+#define FEATURE_SUPPORT(name_, macro_) { \
+	.name = name_,                       \
+	.macro = #macro_,                    \
+	.is_builtin = IS_BUILTIN(macro_) }
+
+extern struct feature_support supported_features[];
+
 void list_common_cmds_help(void);
 const char *help_unknown_cmd(const char *cmd);
 
@@ -9,6 +26,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 d3fc8090413c..6514f4121c49 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -50,6 +50,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.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v7 2/4] perf version: update --build-options to use 'supported_features' array
  2023-11-16 21:34 [PATCH v7 0/4] Introduce perf check subcommand Aditya Gupta
  2023-11-16 21:34 ` [PATCH v7 1/4] perf check: introduce " Aditya Gupta
@ 2023-11-16 21:34 ` Aditya Gupta
  2023-11-16 21:34 ` [PATCH v7 3/4] perf tests task_analyzer: use perf check for libtraceevent support Aditya Gupta
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-11-16 21:34 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

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>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/builtin-version.c | 40 ++++++++----------------------------
 1 file changed, 8 insertions(+), 32 deletions(-)

diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
index ac20c2b9bbc2..e149d96c6dc5 100644
--- a/tools/perf/builtin-version.c
+++ b/tools/perf/builtin-version.c
@@ -46,42 +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_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);
+	for (int i = 0; supported_features[i].name; ++i)
+		STATUS(supported_features[i]);
 }
 
 int cmd_version(int argc, const char **argv)
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v7 3/4] perf tests task_analyzer: use perf check for libtraceevent support
  2023-11-16 21:34 [PATCH v7 0/4] Introduce perf check subcommand Aditya Gupta
  2023-11-16 21:34 ` [PATCH v7 1/4] perf check: introduce " Aditya Gupta
  2023-11-16 21:34 ` [PATCH v7 2/4] perf version: update --build-options to use 'supported_features' array Aditya Gupta
@ 2023-11-16 21:34 ` Aditya Gupta
  2023-11-16 21:34 ` [PATCH v7 4/4] tools/perf/tests: Update probe_vfs_getname.sh script to use perf check --feature Aditya Gupta
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-11-16 21:34 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

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>
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..f4db68edb2e3 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 -q --feature libtraceevent && return 0
+	return 2
 }
 
 prepare_perf_data() {
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v7 4/4] tools/perf/tests: Update probe_vfs_getname.sh script to use perf check --feature
  2023-11-16 21:34 [PATCH v7 0/4] Introduce perf check subcommand Aditya Gupta
                   ` (2 preceding siblings ...)
  2023-11-16 21:34 ` [PATCH v7 3/4] perf tests task_analyzer: use perf check for libtraceevent support Aditya Gupta
@ 2023-11-16 21:34 ` Aditya Gupta
  2023-11-29  5:33 ` [PATCH v7 0/4] Introduce perf check subcommand Athira Rajeev
  2023-11-29  7:09 ` Aditya Gupta
  5 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-11-16 21:34 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

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..368a55c02134 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 -q --feature 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 eebeea6bdc76..758da2a23e26 100755
--- a/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
+++ b/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
@@ -60,7 +60,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.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 0/4] Introduce perf check subcommand
  2023-11-16 21:34 [PATCH v7 0/4] Introduce perf check subcommand Aditya Gupta
                   ` (3 preceding siblings ...)
  2023-11-16 21:34 ` [PATCH v7 4/4] tools/perf/tests: Update probe_vfs_getname.sh script to use perf check --feature Aditya Gupta
@ 2023-11-29  5:33 ` Athira Rajeev
  2023-11-29  7:07   ` Aditya Gupta
  2023-11-29  7:09 ` Aditya Gupta
  5 siblings, 1 reply; 13+ messages in thread
From: Athira Rajeev @ 2023-11-29  5:33 UTC (permalink / raw)
  To: Aditya Gupta, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers
  Cc: Jiri Olsa, linux-perf-users, Madhavan Srinivasan, Kajol Jain,
	Disha Goel



> On 17-Nov-2023, at 3:04 AM, Aditya Gupta <adityag@linux.ibm.com> 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"
> 
> 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
> 
>    or
> 
>    perf check --feature libtraceevent
> 
>    or
> 
>    perf check --feature libtraceevent,libbpf_support
> 
> 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

For the series, 

Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

Thanks
Athira
> Architectures Tested
> ====================
> * x86_64
> * ppc64le
> 
> Git tree
> ========
> 
> Git tree with this patch series applied for testing:
> https://github.com/adi-g15-ibm/linux/tree/perf-check-feature-v7
> 
> Changelog
> =========
> 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 (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       |  59 +++++++++
> tools/perf/builtin-check.c                    | 122 ++++++++++++++++++
> tools/perf/builtin-version.c                  |  39 ++----
> tools/perf/builtin.h                          |  18 +++
> 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, 221 insertions(+), 37 deletions(-)
> create mode 100644 tools/perf/Documentation/perf-check.txt
> create mode 100644 tools/perf/builtin-check.c
> 
> -- 
> 2.41.0
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 0/4] Introduce perf check subcommand
  2023-11-29  5:33 ` [PATCH v7 0/4] Introduce perf check subcommand Athira Rajeev
@ 2023-11-29  7:07   ` Aditya Gupta
  0 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-11-29  7:07 UTC (permalink / raw)
  To: Athira Rajeev
  Cc: Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers, Jiri Olsa,
	linux-perf-users, Madhavan Srinivasan, Kajol Jain, Disha Goel

On Wed, Nov 29, 2023 at 11:03:15AM +0530, Athira Rajeev wrote:
> 
> 
> > On 17-Nov-2023, at 3:04 AM, Aditya Gupta <adityag@linux.ibm.com> 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"
> > 
> > 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
> > 
> >    or
> > 
> >    perf check --feature libtraceevent
> > 
> >    or
> > 
> >    perf check --feature libtraceevent,libbpf_support
> > 
> > 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
> 
> For the series, 
> 
> Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

Thanks for the tag, Athira.

Thanks,

> 
> Thanks
> Athira
> > Architectures Tested
> > ====================
> > * x86_64
> > * ppc64le
> > 
> > Git tree
> > ========
> > 
> > Git tree with this patch series applied for testing:
> > https://github.com/adi-g15-ibm/linux/tree/perf-check-feature-v7
> > 
> > Changelog
> > =========
> > 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 (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       |  59 +++++++++
> > tools/perf/builtin-check.c                    | 122 ++++++++++++++++++
> > tools/perf/builtin-version.c                  |  39 ++----
> > tools/perf/builtin.h                          |  18 +++
> > 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, 221 insertions(+), 37 deletions(-)
> > create mode 100644 tools/perf/Documentation/perf-check.txt
> > create mode 100644 tools/perf/builtin-check.c
> > 
> > -- 
> > 2.41.0
> > 
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 0/4] Introduce perf check subcommand
  2023-11-16 21:34 [PATCH v7 0/4] Introduce perf check subcommand Aditya Gupta
                   ` (4 preceding siblings ...)
  2023-11-29  5:33 ` [PATCH v7 0/4] Introduce perf check subcommand Athira Rajeev
@ 2023-11-29  7:09 ` Aditya Gupta
  2023-11-30 13:33   ` Arnaldo Carvalho de Melo
  5 siblings, 1 reply; 13+ messages in thread
From: Aditya Gupta @ 2023-11-29  7:09 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

Hi Arnaldo,
Can we get this moving if the series looks good ?

Thanks,
Aditya Gupta

On Fri, Nov 17, 2023 at 03:04:31AM +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"
> 
> 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
> 
>     or
> 
>     perf check --feature libtraceevent
> 
>     or
> 
>     perf check --feature libtraceevent,libbpf_support
> 
> 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
> 
> Git tree
> ========
> 
> Git tree with this patch series applied for testing:
> https://github.com/adi-g15-ibm/linux/tree/perf-check-feature-v7
> 
> Changelog
> =========
> 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 (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       |  59 +++++++++
>  tools/perf/builtin-check.c                    | 122 ++++++++++++++++++
>  tools/perf/builtin-version.c                  |  39 ++----
>  tools/perf/builtin.h                          |  18 +++
>  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, 221 insertions(+), 37 deletions(-)
>  create mode 100644 tools/perf/Documentation/perf-check.txt
>  create mode 100644 tools/perf/builtin-check.c
> 
> -- 
> 2.41.0
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 0/4] Introduce perf check subcommand
  2023-11-29  7:09 ` Aditya Gupta
@ 2023-11-30 13:33   ` Arnaldo Carvalho de Melo
  2023-12-04  0:20     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-11-30 13:33 UTC (permalink / raw)
  To: Aditya Gupta
  Cc: jolsa, irogers, namhyung, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

Em Wed, Nov 29, 2023 at 12:39:43PM +0530, Aditya Gupta escreveu:
> Hi Arnaldo,
> Can we get this moving if the series looks good ?

I'm testing and applying it now.

Thanks,

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 0/4] Introduce perf check subcommand
  2023-11-30 13:33   ` Arnaldo Carvalho de Melo
@ 2023-12-04  0:20     ` Arnaldo Carvalho de Melo
  2023-12-04  6:15       ` Aditya Gupta
  0 siblings, 1 reply; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-12-04  0:20 UTC (permalink / raw)
  To: Aditya Gupta
  Cc: jolsa, irogers, namhyung, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

Em Thu, Nov 30, 2023 at 10:33:50AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Nov 29, 2023 at 12:39:43PM +0530, Aditya Gupta escreveu:
> > Hi Arnaldo,
> > Can we get this moving if the series looks good ?
> 
> I'm testing and applying it now.

tomorrow I'll check this:

   3    28.84 alpine:3.15                   : FAIL gcc version 10.3.1 20211027 (Alpine 10.3.1_git20211027)
    builtin-check.c: In function 'cmd_check':
    builtin-check.c:132:3: error: 'strncpy' specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
      132 |   strncpy(feature_list, check.feature, strlen(check.feature)+1);
          |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    builtin-check.c:132:40: note: length computed here
      132 |   strncpy(feature_list, check.feature, strlen(check.feature)+1);
          |                                        ^~~~~~~~~~~~~~~~~~~~~
    cc1: all warnings being treated as errors
   4   133.95 alpine:3.16                   : Ok   gcc (Alpine 11.2.1_git20220219) 11.2.1 20220219 , Alpine clang version 13.0.1 flex 2.6.4
   5   114.96 alpine:3.17                   : Ok   gcc (Alpine 12.2.1_git20220924-r4) 12.2.1 20220924 , Alpine clang version 15.0.7 flex 2.6.4
   6   107.25 alpine:3.18                   : Ok   gcc (Alpine 12.2.1_git20220924-r10) 12.2.1 20220924 , Alpine clang version 16.0.6 flex 2.6.4
   7   120.58 alpine:edge                   : Ok   gcc (Alpine 13.1.1_git20230722) 13.1.1 20230722 , Alpine clang version 16.0.6 flex 2.6.4
   8    82.73 amazonlinux:2                 : Ok   gcc (GCC) 7.3.1 20180712 (Red Hat 7.3.1-17) , clang version 11.1.0 (Amazon Linux 2 11.1.0-1.amzn2.0.2) flex 2.5.37
   9    96.52 amazonlinux:2023              : Ok   gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2) , clang version 15.0.7 (Amazon Linux 15.0.7-3.amzn2023.0.1) flex 2.6.4
  10    95.74 amazonlinux:devel             : Ok   gcc (GCC) 11.3.1 20221121 (Red Hat 11.3.1-4) , clang version 15.0.6 (Amazon Linux 15.0.6-3.amzn2023.0.2) flex 2.6.4
  11   114.54 archlinux:base                : Ok   gcc (GCC) 13.2.1 20230801 , clang version 16.0.6 flex 2.6.4
  12   110.57 centos:stream                 : Ok   gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-21) , clang version 16.0.6 (Red Hat 16.0.6-2.module_el8+588+6f71ce7b) flex 2.6.1
  13   111.24 clearlinux:latest             : Ok   gcc (Clear Linux OS for Intel Architecture) 13.2.1 20231115 releases/gcc-13.2.0-463-g5d6f62c9b2 , clang version 16.0.6 flex 2.6.4
  14    74.64 debian:10                     : Ok   gcc (Debian 8.3.0-6) 8.3.0 , Debian clang version 11.0.1-2~deb10u1 flex 2.6.4
  15    16.72 debian:11                     : FAIL gcc version 10.2.1 20210110 (Debian 10.2.1-6)
        inlined from 'cmd_check' at builtin-check.c:132:3:
    /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: '__builtin_strncpy' specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
      106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
          |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    builtin-check.c: In function 'cmd_check':
    builtin-check.c:132:40: note: length computed here
      132 |   strncpy(feature_list, check.feature, strlen(check.feature)+1);
          |                                        ^~~~~~~~~~~~~~~~~~~~~
    cc1: all warnings being treated as errors
      CC      /tmp/build/perf/builtin-sched.o
      CC      /tmp/build/perf/builtin-timechart.o
      CC      /tmp/build/perf/builtin-trace.o
      CC      /tmp/build/perf/trace/beauty/clone.o
    In file included from /usr/include/string.h:495,
                     from /git/perf-6.6.0-rc1/tools/include/linux/bitmap.h:5,
                     from util/header.h:10,
                     from builtin-check.c:5:
    In function 'strncpy',
        inlined from 'cmd_check' at builtin-check.c:132:3:
    /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: '__builtin_strncpy' specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
      106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
          |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    builtin-check.c: In function 'cmd_check':
    builtin-check.c:132:40: note: length computed here
  16    86.95 debian:12                     : Ok   gcc (Debian 12.2.0-14) 12.2.0 , Debian clang version 14.0.6 flex 2.6.4
  17    87.12 debian:experimental           : Ok   gcc (Debian 13.2.0-6) 13.2.0 , Debian clang version 16.0.6 (17) flex 2.6.4
  18    29.86 debian:experimental-x-arm64   : Ok   aarch64-linux-gnu-gcc (Debian 13.2.0-6) 13.2.0  flex 2.6.4
  19    22.37 debian:experimental-x-mips    : Ok   mips-linux-gnu-gcc (Debian 12.3.0-6) 12.3.0  flex 2.6.4
  20    22.58 debian:experimental-x-mips64  : Ok   mips64-linux-gnuabi64-gcc (Debian 12.3.0-6) 12.3.0  flex 2.6.4
  21    22.42 debian:experimental-x-mipsel  : Ok   mipsel-linux-gnu-gcc (Debian 12.3.0-6) 12.3.0  flex 2.6.4
  22   111.30 fedora:35                     : Ok   gcc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-3) , clang version 13.0.1 (Fedora 13.0.1-1.fc35) flex 2.6.4
  23   107.56 fedora:37                     : Ok   gcc (GCC) 12.3.1 20230508 (Red Hat 12.3.1-1) , clang version 15.0.7 (Fedora 15.0.7-2.fc37) flex 2.6.4
  24   100.41 fedora:38                     : Ok   gcc (GCC) 13.2.1 20231011 (Red Hat 13.2.1-4) , clang version 16.0.6 (Fedora 16.0.6-3.fc38) flex 2.6.4
  25    95.37 fedora:39                     : Ok   gcc (GCC) 13.2.1 20231011 (Red Hat 13.2.1-4) , clang version 17.0.3 (Fedora 17.0.3-1.fc39) flex 2.6.4
  26    95.00 fedora:40                     : Ok   gcc (GCC) 13.2.1 20231110 (Red Hat 13.2.1-5) , clang version 17.0.4 (Fedora 17.0.4-1.fc40) flex 2.6.4
  27    95.18 fedora:rawhide                : Ok   gcc (GCC) 13.2.1 20231110 (Red Hat 13.2.1-5) , clang version 17.0.4 (Fedora 17.0.4-1.fc40) flex 2.6.4
  28   109.86 gentoo:stage3                 : Ok   gcc (Gentoo 13.2.1_p20230826 p7) 13.2.1 20230826 , clang version 16.0.6 flex 2.6.4
  29   106.38 manjaro:base                  : Ok   gcc (GCC) 13.2.1 20230801 , clang version 16.0.6 flex 2.6.4
  30   132.00 opensuse:15.4                 : Ok   gcc (SUSE Linux) 7.5.0 , clang version 15.0.7 flex 2.6.4
  31   133.47 opensuse:15.5                 : Ok   gcc (SUSE Linux) 7.5.0 , clang version 15.0.7 flex 2.6.4
  32   132.04 opensuse:tumbleweed           : Ok   gcc (SUSE Linux) 13.2.1 20230912 [revision b96e66fd4ef3e36983969fb8cdd1956f551a074b] , clang version 17.0.1 flex 2.6.4
  33   108.39 oraclelinux:8                 : Ok   gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-18.0.6) , clang version 15.0.7 (Red Hat 15.0.7-1.0.1.module+el8.8.0+20962+7ae483cf) flex 2.6.1
  34   108.26 oraclelinux:9                 : Ok   gcc (GCC) 11.3.1 20221121 (Red Hat 11.3.1-4.3.0.4) , clang version 15.0.7 (Red Hat 15.0.7-2.0.1.el9) flex 2.6.4
  35   109.41 rockylinux:8                  : Ok   gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-18) , clang version 15.0.7 (Red Hat 15.0.7-1.module+el8.8.0+1144+0a4e73bd) flex 2.6.1
  36   109.50 rockylinux:9                  : Ok   gcc (GCC) 11.3.1 20221121 (Red Hat 11.3.1-4) , clang version 15.0.7 (Red Hat 15.0.7-2.el9) flex 2.6.4
  37    24.52 ubuntu:18.04-x-arm            : Ok   arm-linux-gnueabihf-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0  flex 2.6.4
  38    13.41 ubuntu:18.04-x-arm64          : FAIL gcc version 7.5.0 (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04)
  39    23.79 ubuntu:18.04-x-powerpc        : Ok   powerpc-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0  flex 2.6.4
  40    25.25 ubuntu:18.04-x-powerpc64      : Ok   powerpc64-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0  flex 2.6.4
  41    25.39 ubuntu:18.04-x-powerpc64el    : Ok   powerpc64le-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0  flex 2.6.4
  42    39.03 ubuntu:18.04-x-riscv64        : Ok   riscv64-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0  flex 2.6.4
  43    22.10 ubuntu:18.04-x-s390           : Ok   s390x-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0  flex 2.6.4
  44    24.20 ubuntu:18.04-x-sh4            : Ok   sh4-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0  flex 2.6.4
  45    22.30 ubuntu:18.04-x-sparc64        : Ok   sparc64-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0  flex 2.6.4
  46    16.57 ubuntu:20.04                  : FAIL gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04.2)
        inlined from 'cmd_check' at builtin-check.c:132:3:
    /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: '__builtin_strncpy' specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
      106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
          |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    builtin-check.c: In function 'cmd_check':
    builtin-check.c:132:40: note: length computed here
      132 |   strncpy(feature_list, check.feature, strlen(check.feature)+1);
          |                                        ^~~~~~~~~~~~~~~~~~~~~
    cc1: all warnings being treated as errors
      MKDIR   /tmp/build/perf/ui/
      MKDIR   /tmp/build/perf/tests/
      CC      /tmp/build/perf/ui/setup.o
      CC      /tmp/build/perf/tests/builtin-test.o
    In file included from /usr/include/string.h:495,
                     from /git/perf-6.6.0-rc1/tools/include/linux/bitmap.h:5,
                     from util/header.h:10,
                     from builtin-check.c:5:
    In function 'strncpy',
        inlined from 'cmd_check' at builtin-check.c:132:3:
    /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: '__builtin_strncpy' specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
      106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
          |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    builtin-check.c: In function 'cmd_check':
    builtin-check.c:132:40: note: length computed here
  47    99.20 ubuntu:22.04                  : Ok   gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0 , Ubuntu clang version 14.0.0-1ubuntu1.1 flex 2.6.4
  48    94.57 ubuntu:23.04                  : Ok   gcc (Ubuntu 12.3.0-1ubuntu1~23.04) 12.3.0 , Ubuntu clang version 15.0.7 flex 2.6.4
  49    94.24 ubuntu:23.10                  : Ok   gcc (Ubuntu 13.2.0-4ubuntu3) 13.2.0 , Ubuntu clang version 16.0.6 (15) flex 2.6.4

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 1/4] perf check: introduce check subcommand
  2023-11-16 21:34 ` [PATCH v7 1/4] perf check: introduce " Aditya Gupta
@ 2023-12-04  0:24   ` Arnaldo Carvalho de Melo
  2023-12-04  6:15     ` Aditya Gupta
  0 siblings, 1 reply; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-12-04  0:24 UTC (permalink / raw)
  To: Aditya Gupta
  Cc: jolsa, irogers, namhyung, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

Em Fri, Nov 17, 2023 at 03:04:32AM +0530, Aditya Gupta escreveu:
> 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 1 only
> if both libtraceevent and libbpf_support are enabled, else 0 in all other cases
> 
>     perf check --feature libtraceevent,libbpf_support
> 
> 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
> 
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
>  tools/perf/Build                        |   1 +
>  tools/perf/Documentation/perf-check.txt |  65 +++++++++++
>  tools/perf/builtin-check.c              | 146 ++++++++++++++++++++++++
>  tools/perf/builtin.h                    |  18 +++
>  tools/perf/perf.c                       |   1 +
>  5 files changed, 231 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 aa7623622834..a55a797c1b5f 100644
> --- a/tools/perf/Build
> +++ b/tools/perf/Build
> @@ -1,5 +1,6 @@
>  perf-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..dfb1dddd0ac3
> --- /dev/null
> +++ b/tools/perf/Documentation/perf-check.txt
> @@ -0,0 +1,65 @@
> +perf-check(1)
> +===============
> +
> +NAME
> +----
> +perf-check - check features in perf
> +
> +SYNOPSIS
> +--------
> +'perf check' [<options>]
> +
> +DESCRIPTION
> +-----------
> +With no options given, the 'perf check' just prints the perf version
> +on the standard output.
> +
> +If the option '--feature' is given, 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.
> +
> +OPTIONS
> +-------
> +-q::
> +		Do not print any messages or warnings
> +
> +--feature::
> +        Print whether feature(s) is compiled-in or not. A feature name/macro is
> +        required to be passed after this flag
> +
> +        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
> +        compiled-in.
> +
> +        Example Usage:
> +                perf check --feature libtraceevent
> +                perf check --feature HAVE_LIBTRACEEVENT
> +                perf check --feature libtraceevent,libbpf_support
> +
> +        Supported feature names/macro:
> +                dwarf      /  HAVE_DWARF_SUPPORT
> +                dwarf_getlocations  /  HAVE_DWARF_GETLOCATIONS_SUPPORT
> +                libaudit   /  HAVE_LIBAUDIT_SUPPORT
> +                syscall_table       /  HAVE_SYSCALL_TABLE_SUPPORT
> +                libbfd     /  HAVE_LIBBFD_SUPPORT
> +                debuginfod /  HAVE_DEBUGINFOD_SUPPORT
> +                libelf     /  HAVE_LIBELF_SUPPORT
> +                libnuma    /  HAVE_LIBNUMA_SUPPORT
> +                numa_num_possible_cpus  /  HAVE_LIBNUMA_SUPPORT
> +                libperl    /  HAVE_LIBPERL_SUPPORT
> +                libpython  /  HAVE_LIBPYTHON_SUPPORT
> +                libslang   /  HAVE_SLANG_SUPPORT
> +                libcrypto  /  HAVE_LIBCRYPTO_SUPPORT
> +                libunwind  /  HAVE_LIBUNWIND_SUPPORT
> +                libdw-dwarf-unwind  /  HAVE_DWARF_SUPPORT
> +                zlib       /  HAVE_ZLIB_SUPPORT
> +                lzma       /  HAVE_LZMA_SUPPORT
> +                get_cpuid  /  HAVE_AUXTRACE_SUPPORT
> +                bpf        /  HAVE_LIBBPF_SUPPORT
> +                aio        /  HAVE_AIO_SUPPORT
> +                zstd       /  HAVE_ZSTD_SUPPORT
> +                libpfm4    /  HAVE_LIBPFM
> +                libtraceevent      /  HAVE_LIBTRACEEVENT
> +                bpf_skeletons      /  HAVE_BPF_SKEL
> diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> new file mode 100644
> index 000000000000..25373f055121
> --- /dev/null
> +++ b/tools/perf/builtin-check.c
> @@ -0,0 +1,146 @@
> +// 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>
> +
> +struct check {
> +	const char *feature;
> +};
> +
> +static struct check check;
> +
> +static struct option check_options[] = {
> +	OPT_STRING(0, "feature", &check.feature, NULL, "check if feature(s) is built in"),
> +	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
> +	OPT_END(),
> +};
> +
> +static const char * const check_usage[] = {
> +	"perf check [<options>]",
> +	NULL
> +};
> +
> +struct feature_support supported_features[] = {
> +	FEATURE_SUPPORT("dwarf", HAVE_DWARF_SUPPORT),
> +	FEATURE_SUPPORT("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
> +	FEATURE_SUPPORT("libaudit", HAVE_LIBAUDIT_SUPPORT),
> +	FEATURE_SUPPORT("syscall_table", HAVE_SYSCALL_TABLE_SUPPORT),
> +	FEATURE_SUPPORT("libbfd", HAVE_LIBBFD_SUPPORT),
> +	FEATURE_SUPPORT("debuginfod", HAVE_DEBUGINFOD_SUPPORT),
> +	FEATURE_SUPPORT("libelf", HAVE_LIBELF_SUPPORT),
> +	FEATURE_SUPPORT("libnuma", HAVE_LIBNUMA_SUPPORT),
> +	FEATURE_SUPPORT("numa_num_possible_cpus", HAVE_LIBNUMA_SUPPORT),
> +	FEATURE_SUPPORT("libperl", HAVE_LIBPERL_SUPPORT),
> +	FEATURE_SUPPORT("libpython", HAVE_LIBPYTHON_SUPPORT),
> +	FEATURE_SUPPORT("libslang", HAVE_SLANG_SUPPORT),
> +	FEATURE_SUPPORT("libcrypto", HAVE_LIBCRYPTO_SUPPORT),
> +	FEATURE_SUPPORT("libunwind", HAVE_LIBUNWIND_SUPPORT),
> +	FEATURE_SUPPORT("libdw-dwarf-unwind", HAVE_DWARF_SUPPORT),
> +	FEATURE_SUPPORT("zlib", HAVE_ZLIB_SUPPORT),
> +	FEATURE_SUPPORT("lzma", HAVE_LZMA_SUPPORT),
> +	FEATURE_SUPPORT("get_cpuid", HAVE_AUXTRACE_SUPPORT),
> +	FEATURE_SUPPORT("bpf", HAVE_LIBBPF_SUPPORT),
> +	FEATURE_SUPPORT("aio", HAVE_AIO_SUPPORT),
> +	FEATURE_SUPPORT("zstd", HAVE_ZSTD_SUPPORT),
> +	FEATURE_SUPPORT("libpfm4", HAVE_LIBPFM),
> +	FEATURE_SUPPORT("libtraceevent", HAVE_LIBTRACEEVENT),
> +	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL),
> +
> +	/* this should remain at end, to know the array end */
> +	FEATURE_SUPPORT(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(" ]");
> +}
> +
> +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 ((strcmp(feature, supported_features[i].name) == 0) ||
> +		    (strcmp(feature, supported_features[i].macro) == 0)) {
> +			if (!quiet)
> +				STATUS(supported_features[i]);
> +			return supported_features[i].is_builtin;
> +		}
> +	}
> +
> +	if (!quiet)
> +		color_fprintf(stdout, PERF_COLOR_RED, "Feature not known: '%s'\n", feature);
> +
> +	return 0;
> +}
> +
> +int cmd_check(int argc, const char **argv)
> +{
> +	char *feature_list;
> +	char *feature_name;
> +	int feature_enabled;
> +
> +	argc = parse_options(argc, argv, check_options, check_usage,
> +			     PARSE_OPT_STOP_AT_NON_OPTION);
> +
> +	if (check.feature) {
> +		/* check.feature can be a single feature name, or a comma-separated list
> +		 * of feature names.
> +		 * eg. check.feature can be "libtraceevent" or
> +		 * "libtraceevent,libbpf_support" 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 check.feature will get modified due to strtok and str_trim
> +		 */
> +		feature_enabled = 1;
> +		feature_list = malloc((strlen(check.feature)+1) * sizeof(char));

malloc can fail, do error checking here.

> +		strncpy(feature_list, check.feature, strlen(check.feature)+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;
> +	}
> +
> +	return 0;
> +}
> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> index f2ab5bae2150..9f895981bc9e 100644
> --- a/tools/perf/builtin.h
> +++ b/tools/perf/builtin.h
> @@ -2,6 +2,23 @@
>  #ifndef BUILTIN_H
>  #define BUILTIN_H
>  
> +#include <stddef.h>
> +#include <linux/compiler.h>
> +#include <tools/config.h>
> +
> +struct feature_support {
> +	const char *name;
> +	const char *macro;
> +	int is_builtin;
> +};
> +
> +#define FEATURE_SUPPORT(name_, macro_) { \
> +	.name = name_,                       \
> +	.macro = #macro_,                    \
> +	.is_builtin = IS_BUILTIN(macro_) }
> +
> +extern struct feature_support supported_features[];
> +
>  void list_common_cmds_help(void);
>  const char *help_unknown_cmd(const char *cmd);
>  
> @@ -9,6 +26,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 d3fc8090413c..6514f4121c49 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -50,6 +50,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.41.0
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 0/4] Introduce perf check subcommand
  2023-12-04  0:20     ` Arnaldo Carvalho de Melo
@ 2023-12-04  6:15       ` Aditya Gupta
  0 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-12-04  6:15 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: jolsa, irogers, namhyung, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

On Sun, Dec 03, 2023 at 09:20:20PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 30, 2023 at 10:33:50AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Nov 29, 2023 at 12:39:43PM +0530, Aditya Gupta escreveu:
> > > Hi Arnaldo,
> > > Can we get this moving if the series looks good ?
> > 
> > I'm testing and applying it now.
> 
> tomorrow I'll check this:
> 
>    3    28.84 alpine:3.15                   : FAIL gcc version 10.3.1 20211027 (Alpine 10.3.1_git20211027)
>     builtin-check.c: In function 'cmd_check':
>     builtin-check.c:132:3: error: 'strncpy' specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
>       132 |   strncpy(feature_list, check.feature, strlen(check.feature)+1);
>           |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     builtin-check.c:132:40: note: length computed here
>       132 |   strncpy(feature_list, check.feature, strlen(check.feature)+1);
>           |                                        ^~~~~~~~~~~~~~~~~~~~~
>
> <...snip...>
>

Oh I see, I will fix it in next version.

The idea was that 'feature_list' is basically a non-const copy of
'check.feature', since strtok which has been used later requires it to be
non-const.

'check.feature' size we don't know since it's coming from the argument parser,
so instead of fixing a size for 'feature_list', I am instead using 'strlen' to
get buffer of same size.

I will put strlen into a variable, and use memcpy instead, how does it sound ?

Thanks,
Aditya


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 1/4] perf check: introduce check subcommand
  2023-12-04  0:24   ` Arnaldo Carvalho de Melo
@ 2023-12-04  6:15     ` Aditya Gupta
  0 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-12-04  6:15 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: jolsa, irogers, namhyung, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

On Sun, Dec 03, 2023 at 09:24:41PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 17, 2023 at 03:04:32AM +0530, Aditya Gupta escreveu:
> > 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 1 only
> > if both libtraceevent and libbpf_support are enabled, else 0 in all other cases
> > 
> >     perf check --feature libtraceevent,libbpf_support
> > 
> > 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
> > 
> > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> > ---
> >  tools/perf/Build                        |   1 +
> >  tools/perf/Documentation/perf-check.txt |  65 +++++++++++
> >  tools/perf/builtin-check.c              | 146 ++++++++++++++++++++++++
> >  tools/perf/builtin.h                    |  18 +++
> >  tools/perf/perf.c                       |   1 +
> >  5 files changed, 231 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 aa7623622834..a55a797c1b5f 100644
> > --- a/tools/perf/Build
> > +++ b/tools/perf/Build
> > @@ -1,5 +1,6 @@
> >  perf-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..dfb1dddd0ac3
> > --- /dev/null
> > +++ b/tools/perf/Documentation/perf-check.txt
> > @@ -0,0 +1,65 @@
> > +perf-check(1)
> > +===============
> > +
> > +NAME
> > +----
> > +perf-check - check features in perf
> > +
> > +SYNOPSIS
> > +--------
> > +'perf check' [<options>]
> > +
> > +DESCRIPTION
> > +-----------
> > +With no options given, the 'perf check' just prints the perf version
> > +on the standard output.
> > +
> > +If the option '--feature' is given, 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.
> > +
> > +OPTIONS
> > +-------
> > +-q::
> > +		Do not print any messages or warnings
> > +
> > +--feature::
> > +        Print whether feature(s) is compiled-in or not. A feature name/macro is
> > +        required to be passed after this flag
> > +
> > +        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
> > +        compiled-in.
> > +
> > +        Example Usage:
> > +                perf check --feature libtraceevent
> > +                perf check --feature HAVE_LIBTRACEEVENT
> > +                perf check --feature libtraceevent,libbpf_support
> > +
> > +        Supported feature names/macro:
> > +                dwarf      /  HAVE_DWARF_SUPPORT
> > +                dwarf_getlocations  /  HAVE_DWARF_GETLOCATIONS_SUPPORT
> > +                libaudit   /  HAVE_LIBAUDIT_SUPPORT
> > +                syscall_table       /  HAVE_SYSCALL_TABLE_SUPPORT
> > +                libbfd     /  HAVE_LIBBFD_SUPPORT
> > +                debuginfod /  HAVE_DEBUGINFOD_SUPPORT
> > +                libelf     /  HAVE_LIBELF_SUPPORT
> > +                libnuma    /  HAVE_LIBNUMA_SUPPORT
> > +                numa_num_possible_cpus  /  HAVE_LIBNUMA_SUPPORT
> > +                libperl    /  HAVE_LIBPERL_SUPPORT
> > +                libpython  /  HAVE_LIBPYTHON_SUPPORT
> > +                libslang   /  HAVE_SLANG_SUPPORT
> > +                libcrypto  /  HAVE_LIBCRYPTO_SUPPORT
> > +                libunwind  /  HAVE_LIBUNWIND_SUPPORT
> > +                libdw-dwarf-unwind  /  HAVE_DWARF_SUPPORT
> > +                zlib       /  HAVE_ZLIB_SUPPORT
> > +                lzma       /  HAVE_LZMA_SUPPORT
> > +                get_cpuid  /  HAVE_AUXTRACE_SUPPORT
> > +                bpf        /  HAVE_LIBBPF_SUPPORT
> > +                aio        /  HAVE_AIO_SUPPORT
> > +                zstd       /  HAVE_ZSTD_SUPPORT
> > +                libpfm4    /  HAVE_LIBPFM
> > +                libtraceevent      /  HAVE_LIBTRACEEVENT
> > +                bpf_skeletons      /  HAVE_BPF_SKEL
> > diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> > new file mode 100644
> > index 000000000000..25373f055121
> > --- /dev/null
> > +++ b/tools/perf/builtin-check.c
> > @@ -0,0 +1,146 @@
> > +// 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>
> > +
> > +struct check {
> > +	const char *feature;
> > +};
> > +
> > +static struct check check;
> > +
> > +static struct option check_options[] = {
> > +	OPT_STRING(0, "feature", &check.feature, NULL, "check if feature(s) is built in"),
> > +	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
> > +	OPT_END(),
> > +};
> > +
> > +static const char * const check_usage[] = {
> > +	"perf check [<options>]",
> > +	NULL
> > +};
> > +
> > +struct feature_support supported_features[] = {
> > +	FEATURE_SUPPORT("dwarf", HAVE_DWARF_SUPPORT),
> > +	FEATURE_SUPPORT("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
> > +	FEATURE_SUPPORT("libaudit", HAVE_LIBAUDIT_SUPPORT),
> > +	FEATURE_SUPPORT("syscall_table", HAVE_SYSCALL_TABLE_SUPPORT),
> > +	FEATURE_SUPPORT("libbfd", HAVE_LIBBFD_SUPPORT),
> > +	FEATURE_SUPPORT("debuginfod", HAVE_DEBUGINFOD_SUPPORT),
> > +	FEATURE_SUPPORT("libelf", HAVE_LIBELF_SUPPORT),
> > +	FEATURE_SUPPORT("libnuma", HAVE_LIBNUMA_SUPPORT),
> > +	FEATURE_SUPPORT("numa_num_possible_cpus", HAVE_LIBNUMA_SUPPORT),
> > +	FEATURE_SUPPORT("libperl", HAVE_LIBPERL_SUPPORT),
> > +	FEATURE_SUPPORT("libpython", HAVE_LIBPYTHON_SUPPORT),
> > +	FEATURE_SUPPORT("libslang", HAVE_SLANG_SUPPORT),
> > +	FEATURE_SUPPORT("libcrypto", HAVE_LIBCRYPTO_SUPPORT),
> > +	FEATURE_SUPPORT("libunwind", HAVE_LIBUNWIND_SUPPORT),
> > +	FEATURE_SUPPORT("libdw-dwarf-unwind", HAVE_DWARF_SUPPORT),
> > +	FEATURE_SUPPORT("zlib", HAVE_ZLIB_SUPPORT),
> > +	FEATURE_SUPPORT("lzma", HAVE_LZMA_SUPPORT),
> > +	FEATURE_SUPPORT("get_cpuid", HAVE_AUXTRACE_SUPPORT),
> > +	FEATURE_SUPPORT("bpf", HAVE_LIBBPF_SUPPORT),
> > +	FEATURE_SUPPORT("aio", HAVE_AIO_SUPPORT),
> > +	FEATURE_SUPPORT("zstd", HAVE_ZSTD_SUPPORT),
> > +	FEATURE_SUPPORT("libpfm4", HAVE_LIBPFM),
> > +	FEATURE_SUPPORT("libtraceevent", HAVE_LIBTRACEEVENT),
> > +	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL),
> > +
> > +	/* this should remain at end, to know the array end */
> > +	FEATURE_SUPPORT(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(" ]");
> > +}
> > +
> > +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 ((strcmp(feature, supported_features[i].name) == 0) ||
> > +		    (strcmp(feature, supported_features[i].macro) == 0)) {
> > +			if (!quiet)
> > +				STATUS(supported_features[i]);
> > +			return supported_features[i].is_builtin;
> > +		}
> > +	}
> > +
> > +	if (!quiet)
> > +		color_fprintf(stdout, PERF_COLOR_RED, "Feature not known: '%s'\n", feature);
> > +
> > +	return 0;
> > +}
> > +
> > +int cmd_check(int argc, const char **argv)
> > +{
> > +	char *feature_list;
> > +	char *feature_name;
> > +	int feature_enabled;
> > +
> > +	argc = parse_options(argc, argv, check_options, check_usage,
> > +			     PARSE_OPT_STOP_AT_NON_OPTION);
> > +
> > +	if (check.feature) {
> > +		/* check.feature can be a single feature name, or a comma-separated list
> > +		 * of feature names.
> > +		 * eg. check.feature can be "libtraceevent" or
> > +		 * "libtraceevent,libbpf_support" 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 check.feature will get modified due to strtok and str_trim
> > +		 */
> > +		feature_enabled = 1;
> > +		feature_list = malloc((strlen(check.feature)+1) * sizeof(char));
> 
> malloc can fail, do error checking here.

Sure, I will add a check for that.

Thanks,
Aditya Gupta

> 
> > +		strncpy(feature_list, check.feature, strlen(check.feature)+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;
> > +	}
> > +
> > +	return 0;
> > +}
> > diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> > index f2ab5bae2150..9f895981bc9e 100644
> > --- a/tools/perf/builtin.h
> > +++ b/tools/perf/builtin.h
> > @@ -2,6 +2,23 @@
> >  #ifndef BUILTIN_H
> >  #define BUILTIN_H
> >  
> > +#include <stddef.h>
> > +#include <linux/compiler.h>
> > +#include <tools/config.h>
> > +
> > +struct feature_support {
> > +	const char *name;
> > +	const char *macro;
> > +	int is_builtin;
> > +};
> > +
> > +#define FEATURE_SUPPORT(name_, macro_) { \
> > +	.name = name_,                       \
> > +	.macro = #macro_,                    \
> > +	.is_builtin = IS_BUILTIN(macro_) }
> > +
> > +extern struct feature_support supported_features[];
> > +
> >  void list_common_cmds_help(void);
> >  const char *help_unknown_cmd(const char *cmd);
> >  
> > @@ -9,6 +26,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 d3fc8090413c..6514f4121c49 100644
> > --- a/tools/perf/perf.c
> > +++ b/tools/perf/perf.c
> > @@ -50,6 +50,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.41.0
> > 
> 
> -- 
> 
> - Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-12-04  6:16 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-16 21:34 [PATCH v7 0/4] Introduce perf check subcommand Aditya Gupta
2023-11-16 21:34 ` [PATCH v7 1/4] perf check: introduce " Aditya Gupta
2023-12-04  0:24   ` Arnaldo Carvalho de Melo
2023-12-04  6:15     ` Aditya Gupta
2023-11-16 21:34 ` [PATCH v7 2/4] perf version: update --build-options to use 'supported_features' array Aditya Gupta
2023-11-16 21:34 ` [PATCH v7 3/4] perf tests task_analyzer: use perf check for libtraceevent support Aditya Gupta
2023-11-16 21:34 ` [PATCH v7 4/4] tools/perf/tests: Update probe_vfs_getname.sh script to use perf check --feature Aditya Gupta
2023-11-29  5:33 ` [PATCH v7 0/4] Introduce perf check subcommand Athira Rajeev
2023-11-29  7:07   ` Aditya Gupta
2023-11-29  7:09 ` Aditya Gupta
2023-11-30 13:33   ` Arnaldo Carvalho de Melo
2023-12-04  0:20     ` Arnaldo Carvalho de Melo
2023-12-04  6:15       ` Aditya Gupta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).