All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 1/3] perf report: compile tips.txt in perf binary
@ 2021-04-30 13:33 Denys Zagorui
  2021-04-30 13:33 ` [PATCH v4 2/3] perf tests: avoid storing an absolute path " Denys Zagorui
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Denys Zagorui @ 2021-04-30 13:33 UTC (permalink / raw)
  To: jolsa, linux-kernel, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, namhyung

It seems there is some need to have an ability to invoke perf from
build directory without installation
(84cfac7f05e1: perf tools: Set and pass DOCDIR to builtin-report.c)
DOCDIR definition contains an absolute path to kernel source directory.
It is build machine related info and it makes perf binary unreproducible.

This can be avoided by compiling tips.txt in perf directly.

Signed-off-by: Denys Zagorui <dzagorui@cisco.com>
---
 tools/perf/Build               |  2 +-
 tools/perf/Documentation/Build |  9 ++++++++
 tools/perf/builtin-report.c    | 39 ++++++++++++++++++++++++++--------
 tools/perf/util/util.c         | 28 ------------------------
 tools/perf/util/util.h         |  2 --
 5 files changed, 40 insertions(+), 40 deletions(-)
 create mode 100644 tools/perf/Documentation/Build

diff --git a/tools/perf/Build b/tools/perf/Build
index db61dbe2b543..3a2e768d7576 100644
--- a/tools/perf/Build
+++ b/tools/perf/Build
@@ -45,12 +45,12 @@ CFLAGS_perf.o              += -DPERF_HTML_PATH="BUILD_STR($(htmldir_SQ))"	\
 			      -DPREFIX="BUILD_STR($(prefix_SQ))"
 CFLAGS_builtin-trace.o	   += -DSTRACE_GROUPS_DIR="BUILD_STR($(STRACE_GROUPS_DIR_SQ))"
 CFLAGS_builtin-report.o	   += -DTIPDIR="BUILD_STR($(tipdir_SQ))"
-CFLAGS_builtin-report.o	   += -DDOCDIR="BUILD_STR($(srcdir_SQ)/Documentation)"
 
 perf-y += util/
 perf-y += arch/
 perf-y += ui/
 perf-y += scripts/
 perf-$(CONFIG_TRACE) += trace/beauty/
+perf-y += Documentation/
 
 gtk-y += ui/gtk/
diff --git a/tools/perf/Documentation/Build b/tools/perf/Documentation/Build
new file mode 100644
index 000000000000..83e16764caa4
--- /dev/null
+++ b/tools/perf/Documentation/Build
@@ -0,0 +1,9 @@
+perf-y += tips.o
+
+quiet_cmd_ld_tips = LD       $@
+      cmd_ld_tips = $(LD) -r -b binary -o $@ $<
+
+$(OUTPUT)Documentation/tips.o: Documentation/tips.txt FORCE
+	$(call rule_mkdir)
+	$(call if_changed,ld_tips)
+
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 2a845d6cac09..88375ed76d53 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -47,7 +47,6 @@
 #include "util/time-utils.h"
 #include "util/auxtrace.h"
 #include "util/units.h"
-#include "util/util.h" // perf_tip()
 #include "ui/ui.h"
 #include "ui/progress.h"
 #include "util/block-info.h"
@@ -107,6 +106,9 @@ struct report {
 	int			nr_block_reports;
 };
 
+extern char _binary_Documentation_tips_txt_start[];
+extern char _binary_Documentation_tips_txt_end[];
+
 static int report__config(const char *var, const char *value, void *cb)
 {
 	struct report *rep = cb;
@@ -604,19 +606,38 @@ static int report__gtk_browse_hists(struct report *rep, const char *help)
 	return hist_browser(rep->session->evlist, help, NULL, rep->min_percent);
 }
 
+#define MAX_TIPS        60
+
+static const char *perf_tip(void)
+{
+	char *str[MAX_TIPS];
+	int i = 0;
+
+	_binary_Documentation_tips_txt_start[_binary_Documentation_tips_txt_end -
+		_binary_Documentation_tips_txt_start - 1] = 0;
+
+	str[i] = strtok(_binary_Documentation_tips_txt_start, "\n");
+	if (!str[i])
+		return "Tips cannot be found!";
+
+	i++;
+
+	while (i < MAX_TIPS) {
+		str[i] = strtok(NULL, "\n");
+		if (!str[i])
+			break;
+		i++;
+	}
+
+	return str[random() % i];
+}
+
 static int report__browse_hists(struct report *rep)
 {
 	int ret;
 	struct perf_session *session = rep->session;
 	struct evlist *evlist = session->evlist;
-	const char *help = perf_tip(system_path(TIPDIR));
-
-	if (help == NULL) {
-		/* fallback for people who don't install perf ;-) */
-		help = perf_tip(DOCDIR);
-		if (help == NULL)
-			help = "Cannot load tips.txt file, please install perf!";
-	}
+	const char *help = perf_tip();
 
 	switch (use_browser) {
 	case 1:
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 37a9492edb3e..3bba74e431ed 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -379,34 +379,6 @@ fetch_kernel_version(unsigned int *puint, char *str,
 	return 0;
 }
 
-const char *perf_tip(const char *dirpath)
-{
-	struct strlist *tips;
-	struct str_node *node;
-	char *tip = NULL;
-	struct strlist_config conf = {
-		.dirname = dirpath,
-		.file_only = true,
-	};
-
-	tips = strlist__new("tips.txt", &conf);
-	if (tips == NULL)
-		return errno == ENOENT ? NULL :
-			"Tip: check path of tips.txt or get more memory! ;-p";
-
-	if (strlist__nr_entries(tips) == 0)
-		goto out;
-
-	node = strlist__entry(tips, random() % strlist__nr_entries(tips));
-	if (asprintf(&tip, "Tip: %s", node->s) < 0)
-		tip = (char *)"Tip: get more memory! ;-)";
-
-out:
-	strlist__delete(tips);
-
-	return tip;
-}
-
 char *perf_exe(char *buf, int len)
 {
 	int n = readlink("/proc/self/exe", buf, len);
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index ad737052e597..80b194ee6c7d 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -39,8 +39,6 @@ int fetch_kernel_version(unsigned int *puint,
 #define KVER_FMT	"%d.%d.%d"
 #define KVER_PARAM(x)	KVER_VERSION(x), KVER_PATCHLEVEL(x), KVER_SUBLEVEL(x)
 
-const char *perf_tip(const char *dirpath);
-
 #ifndef HAVE_SCHED_GETCPU_SUPPORT
 int sched_getcpu(void);
 #endif
-- 
2.26.2.Cisco


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

* [PATCH v4 2/3] perf tests: avoid storing an absolute path in perf binary
  2021-04-30 13:33 [PATCH v4 1/3] perf report: compile tips.txt in perf binary Denys Zagorui
@ 2021-04-30 13:33 ` Denys Zagorui
  2021-05-06 14:40   ` Jiri Olsa
  2021-04-30 13:33 ` [PATCH v4 3/3] perf parse-events: add bison --file-prefix-map option Denys Zagorui
  2021-05-06 13:54 ` [PATCH v4 1/3] perf report: compile tips.txt in perf binary Jiri Olsa
  2 siblings, 1 reply; 9+ messages in thread
From: Denys Zagorui @ 2021-04-30 13:33 UTC (permalink / raw)
  To: jolsa, linux-kernel, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, namhyung

python binding test uses PYTHONPATH definition to find python/perf.so
library. This definition is an absolute path that makes perf binary
unreproducible. This path can be found during runtime execution.

Signed-off-by: Denys Zagorui <dzagorui@cisco.com>
---
 tools/perf/tests/Build        |  2 +-
 tools/perf/tests/python-use.c | 19 ++++++++++++++++++-
 tools/perf/util/util.c        | 21 +++++++++++++++++++++
 tools/perf/util/util.h        |  2 ++
 4 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 650aec19d490..a20098dcdbc4 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -98,5 +98,5 @@ perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
 endif
 
 CFLAGS_attr.o         += -DBINDIR="BUILD_STR($(bindir_SQ))" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
-CFLAGS_python-use.o   += -DPYTHONPATH="BUILD_STR($(OUTPUT)python)" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
+CFLAGS_python-use.o   += -DPYTHON="BUILD_STR($(PYTHON_WORD))"
 CFLAGS_dwarf-unwind.o += -fno-optimize-sibling-calls
diff --git a/tools/perf/tests/python-use.c b/tools/perf/tests/python-use.c
index 98c6d474aa6f..c7a0c9b5366f 100644
--- a/tools/perf/tests/python-use.c
+++ b/tools/perf/tests/python-use.c
@@ -8,18 +8,35 @@
 #include <linux/compiler.h>
 #include "tests.h"
 #include "util/debug.h"
+#include "util/util.h"
+#include <sys/stat.h>
 
 int test__python_use(struct test *test __maybe_unused, int subtest __maybe_unused)
 {
 	char *cmd;
 	int ret;
+	char *exec_path;
+	char *pythonpath;
+	struct stat sb;
+
+	exec_path = perf_exe_path();
+	if (exec_path == NULL)
+		return -1;
+
+	if (asprintf(&pythonpath, "%spython", exec_path) < 0)
+		return -1;
+
+	if (stat(pythonpath, &sb) || !S_ISDIR(sb.st_mode))
+		pythonpath[0] = 0;
 
 	if (asprintf(&cmd, "echo \"import sys ; sys.path.append('%s'); import perf\" | %s %s",
-		     PYTHONPATH, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
+		     pythonpath, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
 		return -1;
 
 	pr_debug("python usage test: \"%s\"\n", cmd);
 	ret = system(cmd) ? -1 : 0;
 	free(cmd);
+	free(exec_path);
+	free(pythonpath);
 	return ret;
 }
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 3bba74e431ed..54e80452887c 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -388,3 +388,24 @@ char *perf_exe(char *buf, int len)
 	}
 	return strcpy(buf, "perf");
 }
+
+char *perf_exe_path(void)
+{
+	int i;
+	char *buf;
+
+	buf = malloc(PATH_MAX);
+	buf = perf_exe(buf, PATH_MAX);
+
+	for (i = strlen(buf) - 1; i != 0 && buf[i] != '/'; i--)
+		;
+
+	if (!i) {
+		free(buf);
+		return NULL;
+	}
+
+	buf[i + 1] = 0;
+
+	return buf;
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 80b194ee6c7d..4e871e890ef8 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -49,6 +49,8 @@ void perf_set_singlethreaded(void);
 void perf_set_multithreaded(void);
 
 char *perf_exe(char *buf, int len);
+/* perf_exe_path return malloc'd string on success, caller must free it */
+char *perf_exe_path(void);
 
 #ifndef O_CLOEXEC
 #ifdef __sparc__
-- 
2.26.2.Cisco


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

* [PATCH v4 3/3] perf parse-events: add bison --file-prefix-map option
  2021-04-30 13:33 [PATCH v4 1/3] perf report: compile tips.txt in perf binary Denys Zagorui
  2021-04-30 13:33 ` [PATCH v4 2/3] perf tests: avoid storing an absolute path " Denys Zagorui
@ 2021-04-30 13:33 ` Denys Zagorui
  2021-05-06 15:15   ` Jiri Olsa
  2021-05-06 13:54 ` [PATCH v4 1/3] perf report: compile tips.txt in perf binary Jiri Olsa
  2 siblings, 1 reply; 9+ messages in thread
From: Denys Zagorui @ 2021-04-30 13:33 UTC (permalink / raw)
  To: jolsa, linux-kernel, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, namhyung

bison stores full paths in generated files and those paths are stored in
resulting perf binary. Starting from v3.7.1. those paths can be remapped
by using --file-prefix-map option. So use this option if it possible to
make perf binary more reproducible.

Signed-off-by: Denys Zagorui <dzagorui@cisco.com>
---
 tools/perf/Makefile.config | 9 +++++++++
 tools/perf/util/Build      | 6 +++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index d8e59d31399a..2035bae6d5c5 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -195,6 +195,12 @@ ifeq ($(call get-executable,$(BISON)),)
   dummy := $(error Error: $(BISON) is missing on this system, please install it)
 endif
 
+ifneq ($(OUTPUT),)
+  ifeq ($(shell expr $(shell $(BISON) --version | grep bison | sed -e 's/.\+ \([0-9]\+\).\([0-9]\+\).\([0-9]\+\)/\1\2\3/g') \>\= 371), 1)
+    BISON_FILE_PREFIX_MAP := --file-prefix-map=$(OUTPUT)=
+  endif
+endif
+
 # Treat warnings as errors unless directed not to
 ifneq ($(WERROR),0)
   CORE_CFLAGS += -Werror
@@ -1208,3 +1214,6 @@ $(call detected_var,LIBDIR)
 $(call detected_var,GTK_CFLAGS)
 $(call detected_var,PERL_EMBED_CCOPTS)
 $(call detected_var,PYTHON_EMBED_CCOPTS)
+ifneq ($(BISON_FILE_PREFIX_MAP),)
+$(call detected_var,BISON_FILE_PREFIX_MAP)
+endif
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index e3e12f9d4733..33476b1d28d5 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -204,7 +204,7 @@ $(OUTPUT)util/parse-events-flex.c $(OUTPUT)util/parse-events-flex.h: util/parse-
 
 $(OUTPUT)util/parse-events-bison.c $(OUTPUT)util/parse-events-bison.h: util/parse-events.y
 	$(call rule_mkdir)
-	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) \
+	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) $(BISON_FILE_PREFIX_MAP) \
 		-o $(OUTPUT)util/parse-events-bison.c -p parse_events_
 
 $(OUTPUT)util/expr-flex.c $(OUTPUT)util/expr-flex.h: util/expr.l $(OUTPUT)util/expr-bison.c
@@ -214,7 +214,7 @@ $(OUTPUT)util/expr-flex.c $(OUTPUT)util/expr-flex.h: util/expr.l $(OUTPUT)util/e
 
 $(OUTPUT)util/expr-bison.c $(OUTPUT)util/expr-bison.h: util/expr.y
 	$(call rule_mkdir)
-	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) \
+	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) $(BISON_FILE_PREFIX_MAP) \
 		-o $(OUTPUT)util/expr-bison.c -p expr_
 
 $(OUTPUT)util/pmu-flex.c $(OUTPUT)util/pmu-flex.h: util/pmu.l $(OUTPUT)util/pmu-bison.c
@@ -224,7 +224,7 @@ $(OUTPUT)util/pmu-flex.c $(OUTPUT)util/pmu-flex.h: util/pmu.l $(OUTPUT)util/pmu-
 
 $(OUTPUT)util/pmu-bison.c $(OUTPUT)util/pmu-bison.h: util/pmu.y
 	$(call rule_mkdir)
-	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) \
+	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) $(BISON_FILE_PREFIX_MAP) \
 		-o $(OUTPUT)util/pmu-bison.c -p perf_pmu_
 
 FLEX_GE_26 := $(shell expr $(shell $(FLEX) --version | sed -e  's/flex \([0-9]\+\).\([0-9]\+\)/\1\2/g') \>\= 26)
-- 
2.26.2.Cisco


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

* Re: [PATCH v4 1/3] perf report: compile tips.txt in perf binary
  2021-04-30 13:33 [PATCH v4 1/3] perf report: compile tips.txt in perf binary Denys Zagorui
  2021-04-30 13:33 ` [PATCH v4 2/3] perf tests: avoid storing an absolute path " Denys Zagorui
  2021-04-30 13:33 ` [PATCH v4 3/3] perf parse-events: add bison --file-prefix-map option Denys Zagorui
@ 2021-05-06 13:54 ` Jiri Olsa
  2021-05-07  5:46   ` Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
  2 siblings, 1 reply; 9+ messages in thread
From: Jiri Olsa @ 2021-05-06 13:54 UTC (permalink / raw)
  To: Denys Zagorui
  Cc: linux-kernel, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, namhyung

On Fri, Apr 30, 2021 at 06:33:48AM -0700, Denys Zagorui wrote:
> It seems there is some need to have an ability to invoke perf from
> build directory without installation
> (84cfac7f05e1: perf tools: Set and pass DOCDIR to builtin-report.c)
> DOCDIR definition contains an absolute path to kernel source directory.
> It is build machine related info and it makes perf binary unreproducible.
> 
> This can be avoided by compiling tips.txt in perf directly.
> 
> Signed-off-by: Denys Zagorui <dzagorui@cisco.com>
> ---
>  tools/perf/Build               |  2 +-
>  tools/perf/Documentation/Build |  9 ++++++++
>  tools/perf/builtin-report.c    | 39 ++++++++++++++++++++++++++--------
>  tools/perf/util/util.c         | 28 ------------------------
>  tools/perf/util/util.h         |  2 --
>  5 files changed, 40 insertions(+), 40 deletions(-)
>  create mode 100644 tools/perf/Documentation/Build
> 
> diff --git a/tools/perf/Build b/tools/perf/Build
> index db61dbe2b543..3a2e768d7576 100644
> --- a/tools/perf/Build
> +++ b/tools/perf/Build
> @@ -45,12 +45,12 @@ CFLAGS_perf.o              += -DPERF_HTML_PATH="BUILD_STR($(htmldir_SQ))"	\
>  			      -DPREFIX="BUILD_STR($(prefix_SQ))"
>  CFLAGS_builtin-trace.o	   += -DSTRACE_GROUPS_DIR="BUILD_STR($(STRACE_GROUPS_DIR_SQ))"
>  CFLAGS_builtin-report.o	   += -DTIPDIR="BUILD_STR($(tipdir_SQ))"
> -CFLAGS_builtin-report.o	   += -DDOCDIR="BUILD_STR($(srcdir_SQ)/Documentation)"
>  
>  perf-y += util/
>  perf-y += arch/
>  perf-y += ui/
>  perf-y += scripts/
>  perf-$(CONFIG_TRACE) += trace/beauty/
> +perf-y += Documentation/
>  
>  gtk-y += ui/gtk/
> diff --git a/tools/perf/Documentation/Build b/tools/perf/Documentation/Build
> new file mode 100644
> index 000000000000..83e16764caa4
> --- /dev/null
> +++ b/tools/perf/Documentation/Build
> @@ -0,0 +1,9 @@
> +perf-y += tips.o
> +
> +quiet_cmd_ld_tips = LD       $@
> +      cmd_ld_tips = $(LD) -r -b binary -o $@ $<
> +
> +$(OUTPUT)Documentation/tips.o: Documentation/tips.txt FORCE
> +	$(call rule_mkdir)
> +	$(call if_changed,ld_tips)

nice, I had no idea ld could do it like this ;-)

> +
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 2a845d6cac09..88375ed76d53 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -47,7 +47,6 @@
>  #include "util/time-utils.h"
>  #include "util/auxtrace.h"
>  #include "util/units.h"
> -#include "util/util.h" // perf_tip()
>  #include "ui/ui.h"
>  #include "ui/progress.h"
>  #include "util/block-info.h"
> @@ -107,6 +106,9 @@ struct report {
>  	int			nr_block_reports;
>  };
>  
> +extern char _binary_Documentation_tips_txt_start[];
> +extern char _binary_Documentation_tips_txt_end[];
> +
>  static int report__config(const char *var, const char *value, void *cb)
>  {
>  	struct report *rep = cb;
> @@ -604,19 +606,38 @@ static int report__gtk_browse_hists(struct report *rep, const char *help)
>  	return hist_browser(rep->session->evlist, help, NULL, rep->min_percent);
>  }
>  
> +#define MAX_TIPS        60
> +
> +static const char *perf_tip(void)
> +{
> +	char *str[MAX_TIPS];
> +	int i = 0;
> +
> +	_binary_Documentation_tips_txt_start[_binary_Documentation_tips_txt_end -
> +		_binary_Documentation_tips_txt_start - 1] = 0;
> +
> +	str[i] = strtok(_binary_Documentation_tips_txt_start, "\n");
> +	if (!str[i])
> +		return "Tips cannot be found!";
> +
> +	i++;
> +
> +	while (i < MAX_TIPS) {
> +		str[i] = strtok(NULL, "\n");
> +		if (!str[i])
> +			break;
> +		i++;
> +	}

I dont mind to keep static count of tips, but would be great
to have some check when there are more tips in the tips.txt

or perhaps pick tip with random index within the tips data size?
you would just do strtok until you reach the string that has the
random index inside.. you wouldn't need str pointers then

jirka


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

* Re: [PATCH v4 2/3] perf tests: avoid storing an absolute path in perf binary
  2021-04-30 13:33 ` [PATCH v4 2/3] perf tests: avoid storing an absolute path " Denys Zagorui
@ 2021-05-06 14:40   ` Jiri Olsa
  2021-05-07 11:25     ` Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
  0 siblings, 1 reply; 9+ messages in thread
From: Jiri Olsa @ 2021-05-06 14:40 UTC (permalink / raw)
  To: Denys Zagorui
  Cc: linux-kernel, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, namhyung

On Fri, Apr 30, 2021 at 06:33:49AM -0700, Denys Zagorui wrote:
> python binding test uses PYTHONPATH definition to find python/perf.so
> library. This definition is an absolute path that makes perf binary
> unreproducible. This path can be found during runtime execution.
> 
> Signed-off-by: Denys Zagorui <dzagorui@cisco.com>
> ---
>  tools/perf/tests/Build        |  2 +-
>  tools/perf/tests/python-use.c | 19 ++++++++++++++++++-
>  tools/perf/util/util.c        | 21 +++++++++++++++++++++
>  tools/perf/util/util.h        |  2 ++
>  4 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 650aec19d490..a20098dcdbc4 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -98,5 +98,5 @@ perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
>  endif
>  
>  CFLAGS_attr.o         += -DBINDIR="BUILD_STR($(bindir_SQ))" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
> -CFLAGS_python-use.o   += -DPYTHONPATH="BUILD_STR($(OUTPUT)python)" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
> +CFLAGS_python-use.o   += -DPYTHON="BUILD_STR($(PYTHON_WORD))"
>  CFLAGS_dwarf-unwind.o += -fno-optimize-sibling-calls
> diff --git a/tools/perf/tests/python-use.c b/tools/perf/tests/python-use.c
> index 98c6d474aa6f..c7a0c9b5366f 100644
> --- a/tools/perf/tests/python-use.c
> +++ b/tools/perf/tests/python-use.c
> @@ -8,18 +8,35 @@
>  #include <linux/compiler.h>
>  #include "tests.h"
>  #include "util/debug.h"
> +#include "util/util.h"
> +#include <sys/stat.h>
>  
>  int test__python_use(struct test *test __maybe_unused, int subtest __maybe_unused)
>  {
>  	char *cmd;
>  	int ret;
> +	char *exec_path;
> +	char *pythonpath;
> +	struct stat sb;
> +
> +	exec_path = perf_exe_path();
> +	if (exec_path == NULL)
> +		return -1;

should we return TEST_SKIP in here?

> +
> +	if (asprintf(&pythonpath, "%spython", exec_path) < 0)
> +		return -1;

leaking exec_path

> +
> +	if (stat(pythonpath, &sb) || !S_ISDIR(sb.st_mode))
> +		pythonpath[0] = 0;
>  
>  	if (asprintf(&cmd, "echo \"import sys ; sys.path.append('%s'); import perf\" | %s %s",
> -		     PYTHONPATH, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
> +		     pythonpath, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
>  		return -1;

leaking exec_path and pythonpath

>  
>  	pr_debug("python usage test: \"%s\"\n", cmd);
>  	ret = system(cmd) ? -1 : 0;
>  	free(cmd);
> +	free(exec_path);
> +	free(pythonpath);
>  	return ret;
>  }
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 3bba74e431ed..54e80452887c 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -388,3 +388,24 @@ char *perf_exe(char *buf, int len)
>  	}
>  	return strcpy(buf, "perf");
>  }
> +
> +char *perf_exe_path(void)
> +{
> +	int i;
> +	char *buf;
> +
> +	buf = malloc(PATH_MAX);

need to check buf != NULL

> +	buf = perf_exe(buf, PATH_MAX);
> +
> +	for (i = strlen(buf) - 1; i != 0 && buf[i] != '/'; i--)
> +		;

could we call dirname for this?

thanks,
jirka

> +
> +	if (!i) {
> +		free(buf);
> +		return NULL;
> +	}
> +
> +	buf[i + 1] = 0;
> +
> +	return buf;
> +}
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 80b194ee6c7d..4e871e890ef8 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -49,6 +49,8 @@ void perf_set_singlethreaded(void);
>  void perf_set_multithreaded(void);
>  
>  char *perf_exe(char *buf, int len);
> +/* perf_exe_path return malloc'd string on success, caller must free it */
> +char *perf_exe_path(void);
>  
>  #ifndef O_CLOEXEC
>  #ifdef __sparc__
> -- 
> 2.26.2.Cisco
> 


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

* Re: [PATCH v4 3/3] perf parse-events: add bison --file-prefix-map option
  2021-04-30 13:33 ` [PATCH v4 3/3] perf parse-events: add bison --file-prefix-map option Denys Zagorui
@ 2021-05-06 15:15   ` Jiri Olsa
  2021-05-07  5:27     ` Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
  0 siblings, 1 reply; 9+ messages in thread
From: Jiri Olsa @ 2021-05-06 15:15 UTC (permalink / raw)
  To: Denys Zagorui
  Cc: linux-kernel, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, namhyung

On Fri, Apr 30, 2021 at 06:33:50AM -0700, Denys Zagorui wrote:
> bison stores full paths in generated files and those paths are stored in
> resulting perf binary. Starting from v3.7.1. those paths can be remapped
> by using --file-prefix-map option. So use this option if it possible to
> make perf binary more reproducible.
> 
> Signed-off-by: Denys Zagorui <dzagorui@cisco.com>
> ---
>  tools/perf/Makefile.config | 9 +++++++++
>  tools/perf/util/Build      | 6 +++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index d8e59d31399a..2035bae6d5c5 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -195,6 +195,12 @@ ifeq ($(call get-executable,$(BISON)),)
>    dummy := $(error Error: $(BISON) is missing on this system, please install it)
>  endif
>  
> +ifneq ($(OUTPUT),)
> +  ifeq ($(shell expr $(shell $(BISON) --version | grep bison | sed -e 's/.\+ \([0-9]\+\).\([0-9]\+\).\([0-9]\+\)/\1\2\3/g') \>\= 371), 1)
> +    BISON_FILE_PREFIX_MAP := --file-prefix-map=$(OUTPUT)=
> +  endif
> +endif

I'm on system with:

[root@kvm-02-guest15 perf]# bison --version
bison (GNU Bison) 3.7.6

but I don't see BISON_FILE_PREFIX_MAP set

[root@kvm-02-guest15 perf]# cat .config-detected | grep BISON_FILE_PREFIX_MAP
[root@kvm-02-guest15 perf]# 

jirka

> +
>  # Treat warnings as errors unless directed not to
>  ifneq ($(WERROR),0)
>    CORE_CFLAGS += -Werror
> @@ -1208,3 +1214,6 @@ $(call detected_var,LIBDIR)
>  $(call detected_var,GTK_CFLAGS)
>  $(call detected_var,PERL_EMBED_CCOPTS)
>  $(call detected_var,PYTHON_EMBED_CCOPTS)
> +ifneq ($(BISON_FILE_PREFIX_MAP),)
> +$(call detected_var,BISON_FILE_PREFIX_MAP)
> +endif
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index e3e12f9d4733..33476b1d28d5 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -204,7 +204,7 @@ $(OUTPUT)util/parse-events-flex.c $(OUTPUT)util/parse-events-flex.h: util/parse-
>  
>  $(OUTPUT)util/parse-events-bison.c $(OUTPUT)util/parse-events-bison.h: util/parse-events.y
>  	$(call rule_mkdir)
> -	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) \
> +	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) $(BISON_FILE_PREFIX_MAP) \
>  		-o $(OUTPUT)util/parse-events-bison.c -p parse_events_
>  
>  $(OUTPUT)util/expr-flex.c $(OUTPUT)util/expr-flex.h: util/expr.l $(OUTPUT)util/expr-bison.c
> @@ -214,7 +214,7 @@ $(OUTPUT)util/expr-flex.c $(OUTPUT)util/expr-flex.h: util/expr.l $(OUTPUT)util/e
>  
>  $(OUTPUT)util/expr-bison.c $(OUTPUT)util/expr-bison.h: util/expr.y
>  	$(call rule_mkdir)
> -	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) \
> +	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) $(BISON_FILE_PREFIX_MAP) \
>  		-o $(OUTPUT)util/expr-bison.c -p expr_
>  
>  $(OUTPUT)util/pmu-flex.c $(OUTPUT)util/pmu-flex.h: util/pmu.l $(OUTPUT)util/pmu-bison.c
> @@ -224,7 +224,7 @@ $(OUTPUT)util/pmu-flex.c $(OUTPUT)util/pmu-flex.h: util/pmu.l $(OUTPUT)util/pmu-
>  
>  $(OUTPUT)util/pmu-bison.c $(OUTPUT)util/pmu-bison.h: util/pmu.y
>  	$(call rule_mkdir)
> -	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) \
> +	$(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) $(BISON_FILE_PREFIX_MAP) \
>  		-o $(OUTPUT)util/pmu-bison.c -p perf_pmu_
>  
>  FLEX_GE_26 := $(shell expr $(shell $(FLEX) --version | sed -e  's/flex \([0-9]\+\).\([0-9]\+\)/\1\2/g') \>\= 26)
> -- 
> 2.26.2.Cisco
> 


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

* Re: [PATCH v4 3/3] perf parse-events: add bison --file-prefix-map option
  2021-05-06 15:15   ` Jiri Olsa
@ 2021-05-07  5:27     ` Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
  0 siblings, 0 replies; 9+ messages in thread
From: Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco) @ 2021-05-07  5:27 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: linux-kernel, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, namhyung

> I'm on system with:
>
> [root@kvm-02-guest15 perf]# bison --version
> bison (GNU Bison) 3.7.6
>
> but I don't see BISON_FILE_PREFIX_MAP set
>
> [root@kvm-02-guest15 perf]# cat .config-detected | grep BISON_FILE_PREFIX_MAP
> [root@kvm-02-guest15 perf]#

I forgot to mention that BISON_FILE_PREFIX_MAP is needed only if we build perf with O= .
Most probably that is why you don't see BISON_FILE_PREFIX_MAP. I'll mention this in commit
message.

Thanks,
Denys

________________________________________
From: Jiri Olsa <jolsa@redhat.com>
Sent: 06 May 2021 18:15
To: Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
Cc: linux-kernel@vger.kernel.org; peterz@infradead.org; mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com; alexander.shishkin@linux.intel.com; namhyung@kernel.org
Subject: Re: [PATCH v4 3/3] perf parse-events: add bison --file-prefix-map option

On Fri, Apr 30, 2021 at 06:33:50AM -0700, Denys Zagorui wrote:
> bison stores full paths in generated files and those paths are stored in
> resulting perf binary. Starting from v3.7.1. those paths can be remapped
> by using --file-prefix-map option. So use this option if it possible to
> make perf binary more reproducible.
>
> Signed-off-by: Denys Zagorui <dzagorui@cisco.com>
> ---
>  tools/perf/Makefile.config | 9 +++++++++
>  tools/perf/util/Build      | 6 +++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index d8e59d31399a..2035bae6d5c5 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -195,6 +195,12 @@ ifeq ($(call get-executable,$(BISON)),)
>    dummy := $(error Error: $(BISON) is missing on this system, please install it)
>  endif
>
> +ifneq ($(OUTPUT),)
> +  ifeq ($(shell expr $(shell $(BISON) --version | grep bison | sed -e 's/.\+ \([0-9]\+\).\([0-9]\+\).\([0-9]\+\)/\1\2\3/g') \>\= 371), 1)
> +    BISON_FILE_PREFIX_MAP := --file-prefix-map=$(OUTPUT)=
> +  endif
> +endif

I'm on system with:

[root@kvm-02-guest15 perf]# bison --version
bison (GNU Bison) 3.7.6

but I don't see BISON_FILE_PREFIX_MAP set

[root@kvm-02-guest15 perf]# cat .config-detected | grep BISON_FILE_PREFIX_MAP
[root@kvm-02-guest15 perf]#

jirka

> +
>  # Treat warnings as errors unless directed not to
>  ifneq ($(WERROR),0)
>    CORE_CFLAGS += -Werror
> @@ -1208,3 +1214,6 @@ $(call detected_var,LIBDIR)
>  $(call detected_var,GTK_CFLAGS)
>  $(call detected_var,PERL_EMBED_CCOPTS)
>  $(call detected_var,PYTHON_EMBED_CCOPTS)
> +ifneq ($(BISON_FILE_PREFIX_MAP),)
> +$(call detected_var,BISON_FILE_PREFIX_MAP)
> +endif
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index e3e12f9d4733..33476b1d28d5 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -204,7 +204,7 @@ $(OUTPUT)util/parse-events-flex.c $(OUTPUT)util/parse-events-flex.h: util/parse-
>
>  $(OUTPUT)util/parse-events-bison.c $(OUTPUT)util/parse-events-bison.h: util/parse-events.y
>       $(call rule_mkdir)
> -     $(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) \
> +     $(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) $(BISON_FILE_PREFIX_MAP) \
>               -o $(OUTPUT)util/parse-events-bison.c -p parse_events_
>
>  $(OUTPUT)util/expr-flex.c $(OUTPUT)util/expr-flex.h: util/expr.l $(OUTPUT)util/expr-bison.c
> @@ -214,7 +214,7 @@ $(OUTPUT)util/expr-flex.c $(OUTPUT)util/expr-flex.h: util/expr.l $(OUTPUT)util/e
>
>  $(OUTPUT)util/expr-bison.c $(OUTPUT)util/expr-bison.h: util/expr.y
>       $(call rule_mkdir)
> -     $(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) \
> +     $(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) $(BISON_FILE_PREFIX_MAP) \
>               -o $(OUTPUT)util/expr-bison.c -p expr_
>
>  $(OUTPUT)util/pmu-flex.c $(OUTPUT)util/pmu-flex.h: util/pmu.l $(OUTPUT)util/pmu-bison.c
> @@ -224,7 +224,7 @@ $(OUTPUT)util/pmu-flex.c $(OUTPUT)util/pmu-flex.h: util/pmu.l $(OUTPUT)util/pmu-
>
>  $(OUTPUT)util/pmu-bison.c $(OUTPUT)util/pmu-bison.h: util/pmu.y
>       $(call rule_mkdir)
> -     $(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) \
> +     $(Q)$(call echo-cmd,bison)$(BISON) -v $< -d $(PARSER_DEBUG_BISON) $(BISON_FILE_PREFIX_MAP) \
>               -o $(OUTPUT)util/pmu-bison.c -p perf_pmu_
>
>  FLEX_GE_26 := $(shell expr $(shell $(FLEX) --version | sed -e  's/flex \([0-9]\+\).\([0-9]\+\)/\1\2/g') \>\= 26)
> --
> 2.26.2.Cisco
>


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

* Re: [PATCH v4 1/3] perf report: compile tips.txt in perf binary
  2021-05-06 13:54 ` [PATCH v4 1/3] perf report: compile tips.txt in perf binary Jiri Olsa
@ 2021-05-07  5:46   ` Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
  0 siblings, 0 replies; 9+ messages in thread
From: Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco) @ 2021-05-07  5:46 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: linux-kernel, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, namhyung

> I dont mind to keep static count of tips, but would be great
> to have some check when there are more tips in the tips.txt
>
> or perhaps pick tip with random index within the tips data size?
> you would just do strtok until you reach the string that has the
> random index inside.. you wouldn't need str pointers then

There is no static counter of tips. MAX_TIPS is just maximum number
of tips. Number of tips in tips.txt file:

tools/perf$ wc -l Documentation/tips.txt
43 Documentation/tips.txt

By invoking strtok(..."\n") in a while loop we count how many tips do we have
and also save a pointer to corresponding tip in str[]
After we randomly pick up one of those tips (how it works previously).

________________________________________
From: Jiri Olsa <jolsa@redhat.com>
Sent: 06 May 2021 16:54
To: Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
Cc: linux-kernel@vger.kernel.org; peterz@infradead.org; mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com; alexander.shishkin@linux.intel.com; namhyung@kernel.org
Subject: Re: [PATCH v4 1/3] perf report: compile tips.txt in perf binary

On Fri, Apr 30, 2021 at 06:33:48AM -0700, Denys Zagorui wrote:
> It seems there is some need to have an ability to invoke perf from
> build directory without installation
> (84cfac7f05e1: perf tools: Set and pass DOCDIR to builtin-report.c)
> DOCDIR definition contains an absolute path to kernel source directory.
> It is build machine related info and it makes perf binary unreproducible.
>
> This can be avoided by compiling tips.txt in perf directly.
>
> Signed-off-by: Denys Zagorui <dzagorui@cisco.com>
> ---
>  tools/perf/Build               |  2 +-
>  tools/perf/Documentation/Build |  9 ++++++++
>  tools/perf/builtin-report.c    | 39 ++++++++++++++++++++++++++--------
>  tools/perf/util/util.c         | 28 ------------------------
>  tools/perf/util/util.h         |  2 --
>  5 files changed, 40 insertions(+), 40 deletions(-)
>  create mode 100644 tools/perf/Documentation/Build
>
> diff --git a/tools/perf/Build b/tools/perf/Build
> index db61dbe2b543..3a2e768d7576 100644
> --- a/tools/perf/Build
> +++ b/tools/perf/Build
> @@ -45,12 +45,12 @@ CFLAGS_perf.o              += -DPERF_HTML_PATH="BUILD_STR($(htmldir_SQ))" \
>                             -DPREFIX="BUILD_STR($(prefix_SQ))"
>  CFLAGS_builtin-trace.o          += -DSTRACE_GROUPS_DIR="BUILD_STR($(STRACE_GROUPS_DIR_SQ))"
>  CFLAGS_builtin-report.o         += -DTIPDIR="BUILD_STR($(tipdir_SQ))"
> -CFLAGS_builtin-report.o         += -DDOCDIR="BUILD_STR($(srcdir_SQ)/Documentation)"
>
>  perf-y += util/
>  perf-y += arch/
>  perf-y += ui/
>  perf-y += scripts/
>  perf-$(CONFIG_TRACE) += trace/beauty/
> +perf-y += Documentation/
>
>  gtk-y += ui/gtk/
> diff --git a/tools/perf/Documentation/Build b/tools/perf/Documentation/Build
> new file mode 100644
> index 000000000000..83e16764caa4
> --- /dev/null
> +++ b/tools/perf/Documentation/Build
> @@ -0,0 +1,9 @@
> +perf-y += tips.o
> +
> +quiet_cmd_ld_tips = LD       $@
> +      cmd_ld_tips = $(LD) -r -b binary -o $@ $<
> +
> +$(OUTPUT)Documentation/tips.o: Documentation/tips.txt FORCE
> +     $(call rule_mkdir)
> +     $(call if_changed,ld_tips)

nice, I had no idea ld could do it like this ;-)

> +
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 2a845d6cac09..88375ed76d53 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -47,7 +47,6 @@
>  #include "util/time-utils.h"
>  #include "util/auxtrace.h"
>  #include "util/units.h"
> -#include "util/util.h" // perf_tip()
>  #include "ui/ui.h"
>  #include "ui/progress.h"
>  #include "util/block-info.h"
> @@ -107,6 +106,9 @@ struct report {
>       int                     nr_block_reports;
>  };
>
> +extern char _binary_Documentation_tips_txt_start[];
> +extern char _binary_Documentation_tips_txt_end[];
> +
>  static int report__config(const char *var, const char *value, void *cb)
>  {
>       struct report *rep = cb;
> @@ -604,19 +606,38 @@ static int report__gtk_browse_hists(struct report *rep, const char *help)
>       return hist_browser(rep->session->evlist, help, NULL, rep->min_percent);
>  }
>
> +#define MAX_TIPS        60
> +
> +static const char *perf_tip(void)
> +{
> +     char *str[MAX_TIPS];
> +     int i = 0;
> +
> +     _binary_Documentation_tips_txt_start[_binary_Documentation_tips_txt_end -
> +             _binary_Documentation_tips_txt_start - 1] = 0;
> +
> +     str[i] = strtok(_binary_Documentation_tips_txt_start, "\n");
> +     if (!str[i])
> +             return "Tips cannot be found!";
> +
> +     i++;
> +
> +     while (i < MAX_TIPS) {
> +             str[i] = strtok(NULL, "\n");
> +             if (!str[i])
> +                     break;
> +             i++;
> +     }

I dont mind to keep static count of tips, but would be great
to have some check when there are more tips in the tips.txt

or perhaps pick tip with random index within the tips data size?
you would just do strtok until you reach the string that has the
random index inside.. you wouldn't need str pointers then

jirka


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

* Re: [PATCH v4 2/3] perf tests: avoid storing an absolute path in perf binary
  2021-05-06 14:40   ` Jiri Olsa
@ 2021-05-07 11:25     ` Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
  0 siblings, 0 replies; 9+ messages in thread
From: Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco) @ 2021-05-07 11:25 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: linux-kernel, peterz, mingo, acme, mark.rutland,
	alexander.shishkin, namhyung

>>  int test__python_use(struct test *test __maybe_unused, int subtest __maybe_unused)
>>  {
>>        char *cmd;
>>        int ret;
>> +     char *exec_path;
>> +     char *pythonpath;
>> +     struct stat sb;
>> +
>> +     exec_path = perf_exe_path();
>> +     if (exec_path == NULL)
>> +             return -1;
>
> should we return TEST_SKIP in here?

I'm not sure how it differs from a case where asprintf returns -1

>> +     buf = perf_exe(buf, PATH_MAX);
>> +
>> +     for (i = strlen(buf) - 1; i != 0 && buf[i] != '/'; i--)
>> +             ;
>
> could we call dirname for this?

Yes

Thanks,
Denys

________________________________________
From: Jiri Olsa <jolsa@redhat.com>
Sent: 06 May 2021 17:40
To: Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
Cc: linux-kernel@vger.kernel.org; peterz@infradead.org; mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com; alexander.shishkin@linux.intel.com; namhyung@kernel.org
Subject: Re: [PATCH v4 2/3] perf tests: avoid storing an absolute path in perf binary

On Fri, Apr 30, 2021 at 06:33:49AM -0700, Denys Zagorui wrote:
> python binding test uses PYTHONPATH definition to find python/perf.so
> library. This definition is an absolute path that makes perf binary
> unreproducible. This path can be found during runtime execution.
>
> Signed-off-by: Denys Zagorui <dzagorui@cisco.com>
> ---
>  tools/perf/tests/Build        |  2 +-
>  tools/perf/tests/python-use.c | 19 ++++++++++++++++++-
>  tools/perf/util/util.c        | 21 +++++++++++++++++++++
>  tools/perf/util/util.h        |  2 ++
>  4 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 650aec19d490..a20098dcdbc4 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -98,5 +98,5 @@ perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
>  endif
>
>  CFLAGS_attr.o         += -DBINDIR="BUILD_STR($(bindir_SQ))" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
> -CFLAGS_python-use.o   += -DPYTHONPATH="BUILD_STR($(OUTPUT)python)" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
> +CFLAGS_python-use.o   += -DPYTHON="BUILD_STR($(PYTHON_WORD))"
>  CFLAGS_dwarf-unwind.o += -fno-optimize-sibling-calls
> diff --git a/tools/perf/tests/python-use.c b/tools/perf/tests/python-use.c
> index 98c6d474aa6f..c7a0c9b5366f 100644
> --- a/tools/perf/tests/python-use.c
> +++ b/tools/perf/tests/python-use.c
> @@ -8,18 +8,35 @@
>  #include <linux/compiler.h>
>  #include "tests.h"
>  #include "util/debug.h"
> +#include "util/util.h"
> +#include <sys/stat.h>
>
>  int test__python_use(struct test *test __maybe_unused, int subtest __maybe_unused)
>  {
>       char *cmd;
>       int ret;
> +     char *exec_path;
> +     char *pythonpath;
> +     struct stat sb;
> +
> +     exec_path = perf_exe_path();
> +     if (exec_path == NULL)
> +             return -1;

should we return TEST_SKIP in here?

> +
> +     if (asprintf(&pythonpath, "%spython", exec_path) < 0)
> +             return -1;

leaking exec_path

> +
> +     if (stat(pythonpath, &sb) || !S_ISDIR(sb.st_mode))
> +             pythonpath[0] = 0;
>
>       if (asprintf(&cmd, "echo \"import sys ; sys.path.append('%s'); import perf\" | %s %s",
> -                  PYTHONPATH, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
> +                  pythonpath, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
>               return -1;

leaking exec_path and pythonpath

>
>       pr_debug("python usage test: \"%s\"\n", cmd);
>       ret = system(cmd) ? -1 : 0;
>       free(cmd);
> +     free(exec_path);
> +     free(pythonpath);
>       return ret;
>  }
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 3bba74e431ed..54e80452887c 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -388,3 +388,24 @@ char *perf_exe(char *buf, int len)
>       }
>       return strcpy(buf, "perf");
>  }
> +
> +char *perf_exe_path(void)
> +{
> +     int i;
> +     char *buf;
> +
> +     buf = malloc(PATH_MAX);

need to check buf != NULL

> +     buf = perf_exe(buf, PATH_MAX);
> +
> +     for (i = strlen(buf) - 1; i != 0 && buf[i] != '/'; i--)
> +             ;

could we call dirname for this?

thanks,
jirka

> +
> +     if (!i) {
> +             free(buf);
> +             return NULL;
> +     }
> +
> +     buf[i + 1] = 0;
> +
> +     return buf;
> +}
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 80b194ee6c7d..4e871e890ef8 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -49,6 +49,8 @@ void perf_set_singlethreaded(void);
>  void perf_set_multithreaded(void);
>
>  char *perf_exe(char *buf, int len);
> +/* perf_exe_path return malloc'd string on success, caller must free it */
> +char *perf_exe_path(void);
>
>  #ifndef O_CLOEXEC
>  #ifdef __sparc__
> --
> 2.26.2.Cisco
>


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

end of thread, other threads:[~2021-05-07 11:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-30 13:33 [PATCH v4 1/3] perf report: compile tips.txt in perf binary Denys Zagorui
2021-04-30 13:33 ` [PATCH v4 2/3] perf tests: avoid storing an absolute path " Denys Zagorui
2021-05-06 14:40   ` Jiri Olsa
2021-05-07 11:25     ` Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
2021-04-30 13:33 ` [PATCH v4 3/3] perf parse-events: add bison --file-prefix-map option Denys Zagorui
2021-05-06 15:15   ` Jiri Olsa
2021-05-07  5:27     ` Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
2021-05-06 13:54 ` [PATCH v4 1/3] perf report: compile tips.txt in perf binary Jiri Olsa
2021-05-07  5:46   ` Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.