[PATCH 00/10] perf tools: cleanups, fixes, updates

[PATCH 00/10] perf tools: cleanups, fixes, updates

[Robert Richter]

The patch set contains some cleanups, fixes and updates I found worth
to implement during code review. Some patches can be applied
independently.

Patches #1 and #2 are a repost.

Robert Richter (10):
  perf script: Fix mem leaks and NULL pointer checks around strdup()s
  perf script: Implement option for system-wide profiling
  perf tools: Continue processing header on unknown features
  perf tools: Fix out-of-bound access to struct perf_session
  perf tool: Moving code in some files
  perf report: Setup browser if stdout is a pipe
  perf report: Accept fifos as input file
  perf tool: Unify handling of features when writing feature section
  perf tools: Improve macros for struct feature_ops
  perf tools: Use for_each_set_bit() to iterate over feature flags

 tools/perf/Documentation/perf-annotate.txt     |    2 +-
 tools/perf/Documentation/perf-buildid-list.txt |    2 +-
 tools/perf/Documentation/perf-evlist.txt       |    2 +-
 tools/perf/Documentation/perf-kmem.txt         |    2 +-
 tools/perf/Documentation/perf-lock.txt         |    2 +-
 tools/perf/Documentation/perf-report.txt       |    2 +-
 tools/perf/Documentation/perf-sched.txt        |    2 +-
 tools/perf/Documentation/perf-script.txt       |    2 +-
 tools/perf/Documentation/perf-timechart.txt    |    2 +-
 tools/perf/builtin-annotate.c                  |    4 +-
 tools/perf/builtin-buildid-list.c              |   53 +-
 tools/perf/builtin-evlist.c                    |    2 +-
 tools/perf/builtin-kmem.c                      |    2 +-
 tools/perf/builtin-lock.c                      |    2 +-
 tools/perf/builtin-report.c                    |   13 +-
 tools/perf/builtin-sched.c                     |    2 +-
 tools/perf/builtin-script.c                    |   67 ++-
 tools/perf/builtin-timechart.c                 |    4 +-
 tools/perf/util/header.c                       |  663 +++++++++++-------------
 tools/perf/util/header.h                       |    6 +-
 tools/perf/util/include/linux/bitops.h         |  118 +++++
 tools/perf/util/session.c                      |   15 +-
 tools/perf/util/session.h                      |    2 +-
 23 files changed, 527 insertions(+), 444 deletions(-)

-- 
1.7.7

[PATCH 01/10] perf script: Fix mem leaks and NULL pointer checks around strdup()s

[Robert Richter]

Fix mem leaks and missing null pointer checks after strdup().
get_script_path() did not free __script_root in case of continue.

Introducing a helper function get_script_root().

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 tools/perf/builtin-script.c |   48 ++++++++++++++++++++++++++-----------------
 1 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 2f62a29..bcfc00e 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -681,7 +681,8 @@ static int parse_output_fields(const struct option *opt __used,
 			type = PERF_TYPE_RAW;
 		else {
 			fprintf(stderr, "Invalid event type in field string.\n");
-			return -EINVAL;
+			rc = -EINVAL;
+			goto out;
 		}
 
 		if (output[type].user_set)
@@ -923,6 +924,24 @@ static int read_script_info(struct script_desc *desc, const char *filename)
 	return 0;
 }
 
+static char *get_script_root(struct dirent *script_dirent, const char *suffix)
+{
+	char *script_root, *str;
+
+	script_root = strdup(script_dirent->d_name);
+	if (!script_root)
+		return NULL;
+
+	str = (char *)ends_with(script_root, suffix);
+	if (!str) {
+		free(script_root);
+		return NULL;
+	}
+
+	*str = '\0';
+	return script_root;
+}
+
 static int list_available_scripts(const struct option *opt __used,
 				  const char *s __used, int unset __used)
 {
@@ -934,7 +953,6 @@ static int list_available_scripts(const struct option *opt __used,
 	struct script_desc *desc;
 	char first_half[BUFSIZ];
 	char *script_root;
-	char *str;
 
 	snprintf(scripts_path, MAXPATHLEN, "%s/scripts", perf_exec_path());
 
@@ -950,16 +968,14 @@ static int list_available_scripts(const struct option *opt __used,
 			continue;
 
 		for_each_script(lang_path, lang_dir, script_dirent, script_next) {
-			script_root = strdup(script_dirent.d_name);
-			str = (char *)ends_with(script_root, REPORT_SUFFIX);
-			if (str) {
-				*str = '\0';
+			script_root = get_script_root(&script_dirent, REPORT_SUFFIX);
+			if (script_root) {
 				desc = script_desc__findnew(script_root);
 				snprintf(script_path, MAXPATHLEN, "%s/%s",
 					 lang_path, script_dirent.d_name);
 				read_script_info(desc, script_path);
+				free(script_root);
 			}
-			free(script_root);
 		}
 	}
 
@@ -981,8 +997,7 @@ static char *get_script_path(const char *script_root, const char *suffix)
 	char script_path[MAXPATHLEN];
 	DIR *scripts_dir, *lang_dir;
 	char lang_path[MAXPATHLEN];
-	char *str, *__script_root;
-	char *path = NULL;
+	char *__script_root;
 
 	snprintf(scripts_path, MAXPATHLEN, "%s/scripts", perf_exec_path());
 
@@ -998,23 +1013,18 @@ static char *get_script_path(const char *script_root, const char *suffix)
 			continue;
 
 		for_each_script(lang_path, lang_dir, script_dirent, script_next) {
-			__script_root = strdup(script_dirent.d_name);
-			str = (char *)ends_with(__script_root, suffix);
-			if (str) {
-				*str = '\0';
-				if (strcmp(__script_root, script_root))
-					continue;
+			__script_root = get_script_root(&script_dirent, suffix);
+			if (__script_root && !strcmp(script_root, __script_root)) {
+				free(__script_root);
 				snprintf(script_path, MAXPATHLEN, "%s/%s",
 					 lang_path, script_dirent.d_name);
-				path = strdup(script_path);
-				free(__script_root);
-				break;
+				return strdup(script_path);
 			}
 			free(__script_root);
 		}
 	}
 
-	return path;
+	return NULL;
 }
 
 static bool is_top_script(const char *script_path)
-- 
1.7.7

[PATCH 02/10] perf script: Implement option for system-wide profiling

[Robert Richter]

The option is documented in man perf-script but was not yet
implemented:

       -a
           Force system-wide collection. Scripts run without a
           <command> normally use -a by default, while scripts run
           with a <command> normally don't - this option allows the
           latter to be run in system-wide mode.

As with perf record you now can profile in system-wide mode for the
runtime of a given command, e.g.:

 # perf script -a syscall-counts sleep 2

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 tools/perf/builtin-script.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index bcfc00e..82cca10 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -23,6 +23,7 @@ static u64			nr_unordered;
 extern const struct option	record_options[];
 static bool			no_callchain;
 static bool			show_full_info;
+static bool			system_wide;
 static const char		*cpu_list;
 static DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
 
@@ -1093,6 +1094,8 @@ static const struct option options[] = {
 	OPT_CALLBACK('f', "fields", NULL, "str",
 		     "comma separated output fields prepend with 'type:'. Valid types: hw,sw,trace,raw. Fields: comm,tid,pid,time,cpu,event,trace,ip,sym,dso,addr",
 		     parse_output_fields),
+	OPT_BOOLEAN('a', "all-cpus", &system_wide,
+			    "system-wide collection from all CPUs"),
 	OPT_STRING('c', "cpu", &cpu_list, "cpu", "list of cpus to profile"),
 	OPT_BOOLEAN('I', "show-info", &show_full_info,
 		    "display extended information from perf.data file"),
@@ -1120,7 +1123,6 @@ int cmd_script(int argc, const char **argv, const char *prefix __used)
 	struct perf_session *session;
 	char *script_path = NULL;
 	const char **__argv;
-	bool system_wide;
 	int i, j, err;
 
 	setup_scripting();
@@ -1188,15 +1190,17 @@ int cmd_script(int argc, const char **argv, const char *prefix __used)
 		}
 
 		if (!pid) {
-			system_wide = true;
 			j = 0;
 
 			dup2(live_pipe[1], 1);
 			close(live_pipe[0]);
 
-			if (!is_top_script(argv[0]))
+			if (is_top_script(argv[0])) {
+				system_wide = true;
+			} else if (!system_wide) {
 				system_wide = !have_cmd(argc - rep_args,
 							&argv[rep_args]);
+			}
 
 			__argv = malloc((argc + 6) * sizeof(const char *));
 			if (!__argv)
@@ -1244,10 +1248,11 @@ int cmd_script(int argc, const char **argv, const char *prefix __used)
 		script_path = rep_script_path;
 
 	if (script_path) {
-		system_wide = false;
 		j = 0;
 
-		if (rec_script_path)
+		if (!rec_script_path)
+			system_wide = false;
+		else if (!system_wide)
 			system_wide = !have_cmd(argc - 1, &argv[1]);
 
 		__argv = malloc((argc + 2) * sizeof(const char *));
-- 
1.7.7

[PATCH 03/10] perf tools: Continue processing header on unknown features

[Robert Richter]

A feature may be unknown if perf.data is created and parsed on
different perf tool versions. This should not stop the header to be
processed, instead continue processing it.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 tools/perf/util/header.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index bcd05d0..95c3ab1 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1105,7 +1105,7 @@ static int perf_file_section__fprintf_info(struct perf_file_section *section,
 	}
 	if (feat < HEADER_TRACE_INFO || feat >= HEADER_LAST_FEATURE) {
 		pr_warning("unknown feature %d\n", feat);
-		return -1;
+		return 0;
 	}
 	if (!feat_ops[feat].print)
 		return 0;
-- 
1.7.7

Re: [PATCH 03/10] perf tools: Continue processing header on unknown features

[Arnaldo Carvalho de Melo]

Em Tue, Dec 06, 2011 at 11:32:33AM +0100, Robert Richter escreveu:
> A feature may be unknown if perf.data is created and parsed on
> different perf tool versions. This should not stop the header to be
> processed, instead continue processing it.

Ok with me.
 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  tools/perf/util/header.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index bcd05d0..95c3ab1 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1105,7 +1105,7 @@ static int perf_file_section__fprintf_info(struct perf_file_section *section,
>  	}
>  	if (feat < HEADER_TRACE_INFO || feat >= HEADER_LAST_FEATURE) {
>  		pr_warning("unknown feature %d\n", feat);
> -		return -1;
> +		return 0;
>  	}
>  	if (!feat_ops[feat].print)
>  		return 0;
> -- 
> 1.7.7
> 

[PATCH 04/10] perf tools: Fix out-of-bound access to struct perf_session

[Robert Richter]

If filename is NULL there is an out-of-bound access to struct
perf_session if it would be used with perf_session__open(). Shouldn't
actually happen in current implementation as filename is always
!NULL. Fixing this by always null-terminating filename.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 tools/perf/util/session.c |    2 +-
 tools/perf/util/session.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 85c1e6b7..2ad9c10 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -132,7 +132,7 @@ struct perf_session *perf_session__new(const char *filename, int mode,
 				       bool force, bool repipe,
 				       struct perf_event_ops *ops)
 {
-	size_t len = filename ? strlen(filename) + 1 : 0;
+	size_t len = filename ? strlen(filename) : 0;
 	struct perf_session *self = zalloc(sizeof(*self) + len);
 
 	if (self == NULL)
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 6e393c9..f320cd5 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -54,7 +54,7 @@ struct perf_session {
 	char			*cwd;
 	struct ordered_samples	ordered_samples;
 	struct callchain_cursor	callchain_cursor;
-	char			filename[0];
+	char			filename[1];
 };
 
 struct perf_evsel;
-- 
1.7.7

Re: [PATCH 04/10] perf tools: Fix out-of-bound access to struct perf_session

[Arnaldo Carvalho de Melo]

Em Tue, Dec 06, 2011 at 11:32:34AM +0100, Robert Richter escreveu:
> If filename is NULL there is an out-of-bound access to struct
> perf_session if it would be used with perf_session__open(). Shouldn't
> actually happen in current implementation as filename is always
> !NULL. Fixing this by always null-terminating filename.

Interesting, defensive programing, ok.
 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  tools/perf/util/session.c |    2 +-
>  tools/perf/util/session.h |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 85c1e6b7..2ad9c10 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -132,7 +132,7 @@ struct perf_session *perf_session__new(const char *filename, int mode,
>  				       bool force, bool repipe,
>  				       struct perf_event_ops *ops)
>  {
> -	size_t len = filename ? strlen(filename) + 1 : 0;
> +	size_t len = filename ? strlen(filename) : 0;
>  	struct perf_session *self = zalloc(sizeof(*self) + len);
>  
>  	if (self == NULL)
> diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
> index 6e393c9..f320cd5 100644
> --- a/tools/perf/util/session.h
> +++ b/tools/perf/util/session.h
> @@ -54,7 +54,7 @@ struct perf_session {
>  	char			*cwd;
>  	struct ordered_samples	ordered_samples;
>  	struct callchain_cursor	callchain_cursor;
> -	char			filename[0];
> +	char			filename[1];
>  };
>  
>  struct perf_evsel;
> -- 
> 1.7.7
> 

[PATCH 05/10] perf tool: Moving code in some files

[Robert Richter]

Needed for later changes. No modified functionality.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 tools/perf/builtin-buildid-list.c |   36 ++--
 tools/perf/util/header.c          |  495 ++++++++++++++++++-------------------
 2 files changed, 264 insertions(+), 267 deletions(-)

diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
index cb690a6..4895668 100644
--- a/tools/perf/builtin-buildid-list.c
+++ b/tools/perf/builtin-buildid-list.c
@@ -39,24 +39,6 @@ static const struct option options[] = {
 	OPT_END()
 };
 
-static int perf_session__list_build_ids(void)
-{
-	struct perf_session *session;
-
-	session = perf_session__new(input_name, O_RDONLY, force, false,
-				    &build_id__mark_dso_hit_ops);
-	if (session == NULL)
-		return -1;
-
-	if (with_hits)
-		perf_session__process_events(session, &build_id__mark_dso_hit_ops);
-
-	perf_session__fprintf_dsos_buildid(session, stdout, with_hits);
-
-	perf_session__delete(session);
-	return 0;
-}
-
 static int sysfs__fprintf_build_id(FILE *fp)
 {
 	u8 kallsyms_build_id[BUILD_ID_SIZE];
@@ -85,6 +67,24 @@ static int filename__fprintf_build_id(const char *name, FILE *fp)
 	return fprintf(fp, "%s\n", sbuild_id);
 }
 
+static int perf_session__list_build_ids(void)
+{
+	struct perf_session *session;
+
+	session = perf_session__new(input_name, O_RDONLY, force, false,
+				    &build_id__mark_dso_hit_ops);
+	if (session == NULL)
+		return -1;
+
+	if (with_hits)
+		perf_session__process_events(session, &build_id__mark_dso_hit_ops);
+
+	perf_session__fprintf_dsos_buildid(session, stdout, with_hits);
+
+	perf_session__delete(session);
+	return 0;
+}
+
 static int __cmd_buildid_list(void)
 {
 	if (show_kernel)
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 95c3ab1..dda3c5f 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -28,9 +28,6 @@ static struct perf_trace_event_type *events;
 static u32 header_argc;
 static const char **header_argv;
 
-static int dsos__write_buildid_table(struct perf_header *header, int fd);
-static int perf_session__cache_build_ids(struct perf_session *session);
-
 int perf_header__push_event(u64 id, const char *name)
 {
 	if (strlen(name) > MAX_EVENT_NAME)
@@ -187,6 +184,252 @@ perf_header__set_cmdline(int argc, const char **argv)
 	return 0;
 }
 
+#define dsos__for_each_with_build_id(pos, head)	\
+	list_for_each_entry(pos, head, node)	\
+		if (!pos->has_build_id)		\
+			continue;		\
+		else
+
+static int __dsos__write_buildid_table(struct list_head *head, pid_t pid,
+				u16 misc, int fd)
+{
+	struct dso *pos;
+
+	dsos__for_each_with_build_id(pos, head) {
+		int err;
+		struct build_id_event b;
+		size_t len;
+
+		if (!pos->hit)
+			continue;
+		len = pos->long_name_len + 1;
+		len = ALIGN(len, NAME_ALIGN);
+		memset(&b, 0, sizeof(b));
+		memcpy(&b.build_id, pos->build_id, sizeof(pos->build_id));
+		b.pid = pid;
+		b.header.misc = misc;
+		b.header.size = sizeof(b) + len;
+		err = do_write(fd, &b, sizeof(b));
+		if (err < 0)
+			return err;
+		err = write_padded(fd, pos->long_name,
+				   pos->long_name_len + 1, len);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
+static int machine__write_buildid_table(struct machine *machine, int fd)
+{
+	int err;
+	u16 kmisc = PERF_RECORD_MISC_KERNEL,
+	    umisc = PERF_RECORD_MISC_USER;
+
+	if (!machine__is_host(machine)) {
+		kmisc = PERF_RECORD_MISC_GUEST_KERNEL;
+		umisc = PERF_RECORD_MISC_GUEST_USER;
+	}
+
+	err = __dsos__write_buildid_table(&machine->kernel_dsos, machine->pid,
+					  kmisc, fd);
+	if (err == 0)
+		err = __dsos__write_buildid_table(&machine->user_dsos,
+						  machine->pid, umisc, fd);
+	return err;
+}
+
+static int dsos__write_buildid_table(struct perf_header *header, int fd)
+{
+	struct perf_session *session = container_of(header,
+			struct perf_session, header);
+	struct rb_node *nd;
+	int err = machine__write_buildid_table(&session->host_machine, fd);
+
+	if (err)
+		return err;
+
+	for (nd = rb_first(&session->machines); nd; nd = rb_next(nd)) {
+		struct machine *pos = rb_entry(nd, struct machine, rb_node);
+		err = machine__write_buildid_table(pos, fd);
+		if (err)
+			break;
+	}
+	return err;
+}
+
+int build_id_cache__add_s(const char *sbuild_id, const char *debugdir,
+			  const char *name, bool is_kallsyms)
+{
+	const size_t size = PATH_MAX;
+	char *realname, *filename = zalloc(size),
+	     *linkname = zalloc(size), *targetname;
+	int len, err = -1;
+
+	if (is_kallsyms) {
+		if (symbol_conf.kptr_restrict) {
+			pr_debug("Not caching a kptr_restrict'ed /proc/kallsyms\n");
+			return 0;
+		}
+		realname = (char *)name;
+	} else
+		realname = realpath(name, NULL);
+
+	if (realname == NULL || filename == NULL || linkname == NULL)
+		goto out_free;
+
+	len = snprintf(filename, size, "%s%s%s",
+		       debugdir, is_kallsyms ? "/" : "", realname);
+	if (mkdir_p(filename, 0755))
+		goto out_free;
+
+	snprintf(filename + len, sizeof(filename) - len, "/%s", sbuild_id);
+
+	if (access(filename, F_OK)) {
+		if (is_kallsyms) {
+			 if (copyfile("/proc/kallsyms", filename))
+				goto out_free;
+		} else if (link(realname, filename) && copyfile(name, filename))
+			goto out_free;
+	}
+
+	len = snprintf(linkname, size, "%s/.build-id/%.2s",
+		       debugdir, sbuild_id);
+
+	if (access(linkname, X_OK) && mkdir_p(linkname, 0755))
+		goto out_free;
+
+	snprintf(linkname + len, size - len, "/%s", sbuild_id + 2);
+	targetname = filename + strlen(debugdir) - 5;
+	memcpy(targetname, "../..", 5);
+
+	if (symlink(targetname, linkname) == 0)
+		err = 0;
+out_free:
+	if (!is_kallsyms)
+		free(realname);
+	free(filename);
+	free(linkname);
+	return err;
+}
+
+static int build_id_cache__add_b(const u8 *build_id, size_t build_id_size,
+				 const char *name, const char *debugdir,
+				 bool is_kallsyms)
+{
+	char sbuild_id[BUILD_ID_SIZE * 2 + 1];
+
+	build_id__sprintf(build_id, build_id_size, sbuild_id);
+
+	return build_id_cache__add_s(sbuild_id, debugdir, name, is_kallsyms);
+}
+
+int build_id_cache__remove_s(const char *sbuild_id, const char *debugdir)
+{
+	const size_t size = PATH_MAX;
+	char *filename = zalloc(size),
+	     *linkname = zalloc(size);
+	int err = -1;
+
+	if (filename == NULL || linkname == NULL)
+		goto out_free;
+
+	snprintf(linkname, size, "%s/.build-id/%.2s/%s",
+		 debugdir, sbuild_id, sbuild_id + 2);
+
+	if (access(linkname, F_OK))
+		goto out_free;
+
+	if (readlink(linkname, filename, size - 1) < 0)
+		goto out_free;
+
+	if (unlink(linkname))
+		goto out_free;
+
+	/*
+	 * Since the link is relative, we must make it absolute:
+	 */
+	snprintf(linkname, size, "%s/.build-id/%.2s/%s",
+		 debugdir, sbuild_id, filename);
+
+	if (unlink(linkname))
+		goto out_free;
+
+	err = 0;
+out_free:
+	free(filename);
+	free(linkname);
+	return err;
+}
+
+static int dso__cache_build_id(struct dso *dso, const char *debugdir)
+{
+	bool is_kallsyms = dso->kernel && dso->long_name[0] != '/';
+
+	return build_id_cache__add_b(dso->build_id, sizeof(dso->build_id),
+				     dso->long_name, debugdir, is_kallsyms);
+}
+
+static int __dsos__cache_build_ids(struct list_head *head, const char *debugdir)
+{
+	struct dso *pos;
+	int err = 0;
+
+	dsos__for_each_with_build_id(pos, head)
+		if (dso__cache_build_id(pos, debugdir))
+			err = -1;
+
+	return err;
+}
+
+static int machine__cache_build_ids(struct machine *machine, const char *debugdir)
+{
+	int ret = __dsos__cache_build_ids(&machine->kernel_dsos, debugdir);
+	ret |= __dsos__cache_build_ids(&machine->user_dsos, debugdir);
+	return ret;
+}
+
+static int perf_session__cache_build_ids(struct perf_session *session)
+{
+	struct rb_node *nd;
+	int ret;
+	char debugdir[PATH_MAX];
+
+	snprintf(debugdir, sizeof(debugdir), "%s", buildid_dir);
+
+	if (mkdir(debugdir, 0755) != 0 && errno != EEXIST)
+		return -1;
+
+	ret = machine__cache_build_ids(&session->host_machine, debugdir);
+
+	for (nd = rb_first(&session->machines); nd; nd = rb_next(nd)) {
+		struct machine *pos = rb_entry(nd, struct machine, rb_node);
+		ret |= machine__cache_build_ids(pos, debugdir);
+	}
+	return ret ? -1 : 0;
+}
+
+static bool machine__read_build_ids(struct machine *machine, bool with_hits)
+{
+	bool ret = __dsos__read_build_ids(&machine->kernel_dsos, with_hits);
+	ret |= __dsos__read_build_ids(&machine->user_dsos, with_hits);
+	return ret;
+}
+
+static bool perf_session__read_build_ids(struct perf_session *session, bool with_hits)
+{
+	struct rb_node *nd;
+	bool ret = machine__read_build_ids(&session->host_machine, with_hits);
+
+	for (nd = rb_first(&session->machines); nd; nd = rb_next(nd)) {
+		struct machine *pos = rb_entry(nd, struct machine, rb_node);
+		ret |= machine__read_build_ids(pos, with_hits);
+	}
+
+	return ret;
+}
+
 static int write_trace_info(int fd, struct perf_header *h __used,
 			    struct perf_evlist *evlist)
 {
@@ -1132,252 +1375,6 @@ int perf_header__fprintf_info(struct perf_session *session, FILE *fp, bool full)
 	return 0;
 }
 
-#define dsos__for_each_with_build_id(pos, head)	\
-	list_for_each_entry(pos, head, node)	\
-		if (!pos->has_build_id)		\
-			continue;		\
-		else
-
-static int __dsos__write_buildid_table(struct list_head *head, pid_t pid,
-				u16 misc, int fd)
-{
-	struct dso *pos;
-
-	dsos__for_each_with_build_id(pos, head) {
-		int err;
-		struct build_id_event b;
-		size_t len;
-
-		if (!pos->hit)
-			continue;
-		len = pos->long_name_len + 1;
-		len = ALIGN(len, NAME_ALIGN);
-		memset(&b, 0, sizeof(b));
-		memcpy(&b.build_id, pos->build_id, sizeof(pos->build_id));
-		b.pid = pid;
-		b.header.misc = misc;
-		b.header.size = sizeof(b) + len;
-		err = do_write(fd, &b, sizeof(b));
-		if (err < 0)
-			return err;
-		err = write_padded(fd, pos->long_name,
-				   pos->long_name_len + 1, len);
-		if (err < 0)
-			return err;
-	}
-
-	return 0;
-}
-
-static int machine__write_buildid_table(struct machine *machine, int fd)
-{
-	int err;
-	u16 kmisc = PERF_RECORD_MISC_KERNEL,
-	    umisc = PERF_RECORD_MISC_USER;
-
-	if (!machine__is_host(machine)) {
-		kmisc = PERF_RECORD_MISC_GUEST_KERNEL;
-		umisc = PERF_RECORD_MISC_GUEST_USER;
-	}
-
-	err = __dsos__write_buildid_table(&machine->kernel_dsos, machine->pid,
-					  kmisc, fd);
-	if (err == 0)
-		err = __dsos__write_buildid_table(&machine->user_dsos,
-						  machine->pid, umisc, fd);
-	return err;
-}
-
-static int dsos__write_buildid_table(struct perf_header *header, int fd)
-{
-	struct perf_session *session = container_of(header,
-			struct perf_session, header);
-	struct rb_node *nd;
-	int err = machine__write_buildid_table(&session->host_machine, fd);
-
-	if (err)
-		return err;
-
-	for (nd = rb_first(&session->machines); nd; nd = rb_next(nd)) {
-		struct machine *pos = rb_entry(nd, struct machine, rb_node);
-		err = machine__write_buildid_table(pos, fd);
-		if (err)
-			break;
-	}
-	return err;
-}
-
-int build_id_cache__add_s(const char *sbuild_id, const char *debugdir,
-			  const char *name, bool is_kallsyms)
-{
-	const size_t size = PATH_MAX;
-	char *realname, *filename = zalloc(size),
-	     *linkname = zalloc(size), *targetname;
-	int len, err = -1;
-
-	if (is_kallsyms) {
-		if (symbol_conf.kptr_restrict) {
-			pr_debug("Not caching a kptr_restrict'ed /proc/kallsyms\n");
-			return 0;
-		}
-		realname = (char *)name;
-	} else
-		realname = realpath(name, NULL);
-
-	if (realname == NULL || filename == NULL || linkname == NULL)
-		goto out_free;
-
-	len = snprintf(filename, size, "%s%s%s",
-		       debugdir, is_kallsyms ? "/" : "", realname);
-	if (mkdir_p(filename, 0755))
-		goto out_free;
-
-	snprintf(filename + len, sizeof(filename) - len, "/%s", sbuild_id);
-
-	if (access(filename, F_OK)) {
-		if (is_kallsyms) {
-			 if (copyfile("/proc/kallsyms", filename))
-				goto out_free;
-		} else if (link(realname, filename) && copyfile(name, filename))
-			goto out_free;
-	}
-
-	len = snprintf(linkname, size, "%s/.build-id/%.2s",
-		       debugdir, sbuild_id);
-
-	if (access(linkname, X_OK) && mkdir_p(linkname, 0755))
-		goto out_free;
-
-	snprintf(linkname + len, size - len, "/%s", sbuild_id + 2);
-	targetname = filename + strlen(debugdir) - 5;
-	memcpy(targetname, "../..", 5);
-
-	if (symlink(targetname, linkname) == 0)
-		err = 0;
-out_free:
-	if (!is_kallsyms)
-		free(realname);
-	free(filename);
-	free(linkname);
-	return err;
-}
-
-static int build_id_cache__add_b(const u8 *build_id, size_t build_id_size,
-				 const char *name, const char *debugdir,
-				 bool is_kallsyms)
-{
-	char sbuild_id[BUILD_ID_SIZE * 2 + 1];
-
-	build_id__sprintf(build_id, build_id_size, sbuild_id);
-
-	return build_id_cache__add_s(sbuild_id, debugdir, name, is_kallsyms);
-}
-
-int build_id_cache__remove_s(const char *sbuild_id, const char *debugdir)
-{
-	const size_t size = PATH_MAX;
-	char *filename = zalloc(size),
-	     *linkname = zalloc(size);
-	int err = -1;
-
-	if (filename == NULL || linkname == NULL)
-		goto out_free;
-
-	snprintf(linkname, size, "%s/.build-id/%.2s/%s",
-		 debugdir, sbuild_id, sbuild_id + 2);
-
-	if (access(linkname, F_OK))
-		goto out_free;
-
-	if (readlink(linkname, filename, size - 1) < 0)
-		goto out_free;
-
-	if (unlink(linkname))
-		goto out_free;
-
-	/*
-	 * Since the link is relative, we must make it absolute:
-	 */
-	snprintf(linkname, size, "%s/.build-id/%.2s/%s",
-		 debugdir, sbuild_id, filename);
-
-	if (unlink(linkname))
-		goto out_free;
-
-	err = 0;
-out_free:
-	free(filename);
-	free(linkname);
-	return err;
-}
-
-static int dso__cache_build_id(struct dso *dso, const char *debugdir)
-{
-	bool is_kallsyms = dso->kernel && dso->long_name[0] != '/';
-
-	return build_id_cache__add_b(dso->build_id, sizeof(dso->build_id),
-				     dso->long_name, debugdir, is_kallsyms);
-}
-
-static int __dsos__cache_build_ids(struct list_head *head, const char *debugdir)
-{
-	struct dso *pos;
-	int err = 0;
-
-	dsos__for_each_with_build_id(pos, head)
-		if (dso__cache_build_id(pos, debugdir))
-			err = -1;
-
-	return err;
-}
-
-static int machine__cache_build_ids(struct machine *machine, const char *debugdir)
-{
-	int ret = __dsos__cache_build_ids(&machine->kernel_dsos, debugdir);
-	ret |= __dsos__cache_build_ids(&machine->user_dsos, debugdir);
-	return ret;
-}
-
-static int perf_session__cache_build_ids(struct perf_session *session)
-{
-	struct rb_node *nd;
-	int ret;
-	char debugdir[PATH_MAX];
-
-	snprintf(debugdir, sizeof(debugdir), "%s", buildid_dir);
-
-	if (mkdir(debugdir, 0755) != 0 && errno != EEXIST)
-		return -1;
-
-	ret = machine__cache_build_ids(&session->host_machine, debugdir);
-
-	for (nd = rb_first(&session->machines); nd; nd = rb_next(nd)) {
-		struct machine *pos = rb_entry(nd, struct machine, rb_node);
-		ret |= machine__cache_build_ids(pos, debugdir);
-	}
-	return ret ? -1 : 0;
-}
-
-static bool machine__read_build_ids(struct machine *machine, bool with_hits)
-{
-	bool ret = __dsos__read_build_ids(&machine->kernel_dsos, with_hits);
-	ret |= __dsos__read_build_ids(&machine->user_dsos, with_hits);
-	return ret;
-}
-
-static bool perf_session__read_build_ids(struct perf_session *session, bool with_hits)
-{
-	struct rb_node *nd;
-	bool ret = machine__read_build_ids(&session->host_machine, with_hits);
-
-	for (nd = rb_first(&session->machines); nd; nd = rb_next(nd)) {
-		struct machine *pos = rb_entry(nd, struct machine, rb_node);
-		ret |= machine__read_build_ids(pos, with_hits);
-	}
-
-	return ret;
-}
-
 static int do_write_feat(int fd, struct perf_header *h, int type,
 			 struct perf_file_section **p,
 			 struct perf_evlist *evlist)
-- 
1.7.7

[PATCH 06/10] perf report: Setup browser if stdout is a pipe

[Robert Richter]

The decision to setup the browser should be made if stdout is pipe,
not stdin.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 tools/perf/builtin-report.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 4d7c834..88ca2d4 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -504,6 +504,8 @@ static const struct option options[] = {
 
 int cmd_report(int argc, const char **argv, const char *prefix __used)
 {
+	struct stat st;
+
 	argc = parse_options(argc, argv, options, report_usage, 0);
 
 	if (use_stdio)
@@ -514,10 +516,11 @@ int cmd_report(int argc, const char **argv, const char *prefix __used)
 	if (inverted_callchain)
 		callchain_param.order = ORDER_CALLER;
 
-	if (strcmp(input_name, "-") != 0)
-		setup_browser(true);
-	else
+	if (!fstat(STDOUT_FILENO, &st) && S_ISFIFO(st.st_mode))
 		use_browser = 0;
+	else
+		setup_browser(true);
+
 	/*
 	 * Only in the newt browser we are doing integrated annotation,
 	 * so don't allocate extra space that won't be used in the stdio
-- 
1.7.7

Re: [PATCH 06/10] perf report: Setup browser if stdout is a pipe

[Arnaldo Carvalho de Melo]

Em Tue, Dec 06, 2011 at 11:32:36AM +0100, Robert Richter escreveu:
> The decision to setup the browser should be made if stdout is pipe,
> not stdin.

I can't remember what made that logic be like that...  Here it is:

commit 46656ac7fb3252f8a3db29b18638e0e8067849ba
Author: Tom Zanussi <tzanussi@gmail.com>
Date:   Thu Apr 1 23:59:17 2010 -0500

    perf report: Introduce special handling for pipe input
    
    Adds special treatment for stdin - if the user specifies '-i -'
    to perf report, the intent is that the event stream be written
    to stdin rather than from a disk file.
    
    The actual handling of the '-' filename is done by the session;
    this just adds a signal handler to stop reporting, and turns off
    interference by the pager.
    
    Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
    Acked-by: Thomas Gleixner <tglx@linutronix.de>
    Cc: fweisbec@gmail.com
    Cc: rostedt@goodmis.org
    Cc: k-keiichi@bx.jp.nec.com
    Cc: acme@ghostprotocols.net
    LKML-Reference: <1270184365-8281-4-git-send-email-tzanussi@gmail.com>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>


I can't understand the comment either, as I think it should've read "the
intent is that the event stream be _read from stdin_ rather than from a
disk file."

And I don't know what would be the pager interference there.

Tom, could you elaborate on this?

Thanks,

- Arnaldo
    
 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  tools/perf/builtin-report.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 4d7c834..88ca2d4 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -504,6 +504,8 @@ static const struct option options[] = {
>  
>  int cmd_report(int argc, const char **argv, const char *prefix __used)
>  {
> +	struct stat st;
> +
>  	argc = parse_options(argc, argv, options, report_usage, 0);
>  
>  	if (use_stdio)
> @@ -514,10 +516,11 @@ int cmd_report(int argc, const char **argv, const char *prefix __used)
>  	if (inverted_callchain)
>  		callchain_param.order = ORDER_CALLER;
>  
> -	if (strcmp(input_name, "-") != 0)
> -		setup_browser(true);
> -	else
> +	if (!fstat(STDOUT_FILENO, &st) && S_ISFIFO(st.st_mode))
>  		use_browser = 0;
> +	else
> +		setup_browser(true);
> +
>  	/*
>  	 * Only in the newt browser we are doing integrated annotation,
>  	 * so don't allocate extra space that won't be used in the stdio
> -- 
> 1.7.7
> 

Re: [PATCH 06/10] perf report: Setup browser if stdout is a pipe

[Tom Zanussi]

On Tue, 2011-12-06 at 11:29 -0200, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 06, 2011 at 11:32:36AM +0100, Robert Richter escreveu:
> > The decision to setup the browser should be made if stdout is pipe,
> > not stdin.
> 
> I can't remember what made that logic be like that...  Here it is:
> 
> commit 46656ac7fb3252f8a3db29b18638e0e8067849ba
> Author: Tom Zanussi <tzanussi@gmail.com>
> Date:   Thu Apr 1 23:59:17 2010 -0500
> 
>     perf report: Introduce special handling for pipe input
>     
>     Adds special treatment for stdin - if the user specifies '-i -'
>     to perf report, the intent is that the event stream be written
>     to stdin rather than from a disk file.
>     
>     The actual handling of the '-' filename is done by the session;
>     this just adds a signal handler to stop reporting, and turns off
>     interference by the pager.
>     
>     Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
>     Acked-by: Thomas Gleixner <tglx@linutronix.de>
>     Cc: fweisbec@gmail.com
>     Cc: rostedt@goodmis.org
>     Cc: k-keiichi@bx.jp.nec.com
>     Cc: acme@ghostprotocols.net
>     LKML-Reference: <1270184365-8281-4-git-send-email-tzanussi@gmail.com>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> 
> I can't understand the comment either, as I think it should've read "the
> intent is that the event stream be _read from stdin_ rather than from a
> disk file."
> 
> And I don't know what would be the pager interference there.
> 
> Tom, could you elaborate on this?
> 

Hi Arnaldo,

Yeah, I think your comment is correct - the commit has a typo and should
read 'the intent is that the event stream be read from stdin'.

Basically, it's from the standpoint of a pipeline such as 'perf record |
perf report+scripting engine' where the output of perf record is
continuously fed into a scripting engine.  The script in then end takes
complete control of the output and doesn't want to be interfered with by
a pager.  IIRC that caused a problem with the 'top'-type scripts.

Anyway, from the standpoint of 'perf report' in a 'live' pipeline, I
think the current code is correct - if the input is coming from a pipe,
the assumption is that it's being passed to a script which will take
care of all the output details, so turn the pager off...

Tom 

> Thanks,
> 
> - Arnaldo
>     
>  
> > Signed-off-by: Robert Richter <robert.richter@amd.com>
> > ---
> >  tools/perf/builtin-report.c |    9 ++++++---
> >  1 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index 4d7c834..88ca2d4 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -504,6 +504,8 @@ static const struct option options[] = {
> >  
> >  int cmd_report(int argc, const char **argv, const char *prefix __used)
> >  {
> > +	struct stat st;
> > +
> >  	argc = parse_options(argc, argv, options, report_usage, 0);
> >  
> >  	if (use_stdio)
> > @@ -514,10 +516,11 @@ int cmd_report(int argc, const char **argv, const char *prefix __used)
> >  	if (inverted_callchain)
> >  		callchain_param.order = ORDER_CALLER;
> >  
> > -	if (strcmp(input_name, "-") != 0)
> > -		setup_browser(true);
> > -	else
> > +	if (!fstat(STDOUT_FILENO, &st) && S_ISFIFO(st.st_mode))
> >  		use_browser = 0;
> > +	else
> > +		setup_browser(true);
> > +
> >  	/*
> >  	 * Only in the newt browser we are doing integrated annotation,
> >  	 * so don't allocate extra space that won't be used in the stdio
> > -- 
> > 1.7.7
> > 

Re: [PATCH 06/10] perf report: Setup browser if stdout is a pipe

[Robert Richter]

On 06.12.11 11:29:10, Arnaldo Carvalho de Melo wrote:
> commit 46656ac7fb3252f8a3db29b18638e0e8067849ba
> Author: Tom Zanussi <tzanussi@gmail.com>
> Date:   Thu Apr 1 23:59:17 2010 -0500
> 
>     perf report: Introduce special handling for pipe input
>     
>     Adds special treatment for stdin - if the user specifies '-i -'
>     to perf report, the intent is that the event stream be written
>     to stdin rather than from a disk file.
>     
>     The actual handling of the '-' filename is done by the session;
>     this just adds a signal handler to stop reporting, and turns off
>     interference by the pager.
>     
>     Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
>     Acked-by: Thomas Gleixner <tglx@linutronix.de>
>     Cc: fweisbec@gmail.com
>     Cc: rostedt@goodmis.org
>     Cc: k-keiichi@bx.jp.nec.com
>     Cc: acme@ghostprotocols.net
>     LKML-Reference: <1270184365-8281-4-git-send-email-tzanussi@gmail.com>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> 
> I can't understand the comment either, as I think it should've read "the
> intent is that the event stream be _read from stdin_ rather than from a
> disk file."
> 
> And I don't know what would be the pager interference there.
> 
> Tom, could you elaborate on this?

This patch is part of a patch set introducing "live mode". I guess the
intention here is to disable the pager for continuously writing to the
terminal and stopping perf with ^C, e.g. (notice the ^C):

 # perf record -e <...> -a | perf report
# perf record -a -e cpu-cycles | perf report -i -   
# ========
# captured on: Tue Dec  6 18:09:07 2011
# ========
#
^C# Events: 9  cycles
#
# Overhead  Command      Shared Object                          Symbol
[ perf record: Woken up 1 times to write data ]
# ........  .......  .................  ..............................
#
[ perf record: Captured and wrote 0.106 MB (null) (~4627 samples) ]
[...]

A pager would catch the ^C, also an appended pipe. So, my version also
does not work for this case, suggest to skip this patch first in favor
of a better solution.

But will have to rework the next patch because a potential null
pointer access to input_name.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 07/10] perf report: Accept fifos as input file

[Robert Richter]

The default input file for perf report is not handled the same way as
perf record does it for its output file. This leads to unexpected
behavior of perf report, etc. E.g.:

 # perf record -a -e cpu-cycles sleep 2 | perf report | cat
 failed to open perf.data: No such file or directory  (try 'perf record' first)

While perf record writes to a fifo, perf report expects perf.data to
be read. This patch changes this to accept fifos as input file.

Applies to the following commands:

 perf annotate
 perf buildid-list
 perf evlist
 perf kmem
 perf lock
 perf report
 perf sched
 perf script
 perf timechart

Also fixes char const* -> const char* type declaration for filename
strings.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 tools/perf/Documentation/perf-annotate.txt     |    2 +-
 tools/perf/Documentation/perf-buildid-list.txt |    2 +-
 tools/perf/Documentation/perf-evlist.txt       |    2 +-
 tools/perf/Documentation/perf-kmem.txt         |    2 +-
 tools/perf/Documentation/perf-lock.txt         |    2 +-
 tools/perf/Documentation/perf-report.txt       |    2 +-
 tools/perf/Documentation/perf-sched.txt        |    2 +-
 tools/perf/Documentation/perf-script.txt       |    2 +-
 tools/perf/Documentation/perf-timechart.txt    |    2 +-
 tools/perf/builtin-annotate.c                  |    4 ++--
 tools/perf/builtin-buildid-list.c              |   19 ++++++++++---------
 tools/perf/builtin-evlist.c                    |    2 +-
 tools/perf/builtin-kmem.c                      |    2 +-
 tools/perf/builtin-lock.c                      |    2 +-
 tools/perf/builtin-report.c                    |    4 ++--
 tools/perf/builtin-sched.c                     |    2 +-
 tools/perf/builtin-script.c                    |    4 ++--
 tools/perf/builtin-timechart.c                 |    4 ++--
 tools/perf/util/session.c                      |   15 +++++++++++++--
 19 files changed, 44 insertions(+), 32 deletions(-)

diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
index fe6762e..0d0f217 100644
--- a/tools/perf/Documentation/perf-annotate.txt
+++ b/tools/perf/Documentation/perf-annotate.txt
@@ -22,7 +22,7 @@ OPTIONS
 -------
 -i::
 --input=::
-        Input file name. (default: perf.data)
+        Input file name. (default: perf.data unless stdin is a fifo)
 
 -d::
 --dsos=<dso[,dso...]>::
diff --git a/tools/perf/Documentation/perf-buildid-list.txt b/tools/perf/Documentation/perf-buildid-list.txt
index cc22325..25c52ef 100644
--- a/tools/perf/Documentation/perf-buildid-list.txt
+++ b/tools/perf/Documentation/perf-buildid-list.txt
@@ -26,7 +26,7 @@ OPTIONS
         Show only DSOs with hits.
 -i::
 --input=::
-        Input file name. (default: perf.data)
+        Input file name. (default: perf.data unless stdin is a fifo)
 -f::
 --force::
 	Don't do ownership validation.
diff --git a/tools/perf/Documentation/perf-evlist.txt b/tools/perf/Documentation/perf-evlist.txt
index 0cada9e..0507ec7 100644
--- a/tools/perf/Documentation/perf-evlist.txt
+++ b/tools/perf/Documentation/perf-evlist.txt
@@ -18,7 +18,7 @@ OPTIONS
 -------
 -i::
 --input=::
-        Input file name. (default: perf.data)
+        Input file name. (default: perf.data unless stdin is a fifo)
 
 SEE ALSO
 --------
diff --git a/tools/perf/Documentation/perf-kmem.txt b/tools/perf/Documentation/perf-kmem.txt
index a52fcde..7c8fbbf 100644
--- a/tools/perf/Documentation/perf-kmem.txt
+++ b/tools/perf/Documentation/perf-kmem.txt
@@ -23,7 +23,7 @@ OPTIONS
 -------
 -i <file>::
 --input=<file>::
-	Select the input file (default: perf.data)
+	Select the input file (default: perf.data unless stdin is a fifo)
 
 --caller::
 	Show per-callsite statistics
diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
index 4a26a2f..d6b2a4f 100644
--- a/tools/perf/Documentation/perf-lock.txt
+++ b/tools/perf/Documentation/perf-lock.txt
@@ -29,7 +29,7 @@ COMMON OPTIONS
 
 -i::
 --input=<file>::
-        Input file name.
+        Input file name. (default: perf.data unless stdin is a fifo)
 
 -v::
 --verbose::
diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index 212f24d..a67be86 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -19,7 +19,7 @@ OPTIONS
 -------
 -i::
 --input=::
-        Input file name. (default: perf.data)
+        Input file name. (default: perf.data unless stdin is a fifo)
 
 -v::
 --verbose::
diff --git a/tools/perf/Documentation/perf-sched.txt b/tools/perf/Documentation/perf-sched.txt
index 5b212b5..8ff4df9 100644
--- a/tools/perf/Documentation/perf-sched.txt
+++ b/tools/perf/Documentation/perf-sched.txt
@@ -40,7 +40,7 @@ OPTIONS
 -------
 -i::
 --input=<file>::
-        Input file name. (default: perf.data)
+        Input file name. (default: perf.data unless stdin is a fifo)
 
 -v::
 --verbose::
diff --git a/tools/perf/Documentation/perf-script.txt b/tools/perf/Documentation/perf-script.txt
index dec87ec..d465c11 100644
--- a/tools/perf/Documentation/perf-script.txt
+++ b/tools/perf/Documentation/perf-script.txt
@@ -106,7 +106,7 @@ OPTIONS
 
 -i::
 --input=::
-        Input file name.
+        Input file name. (default: perf.data unless stdin is a fifo)
 
 -d::
 --debug-mode::
diff --git a/tools/perf/Documentation/perf-timechart.txt b/tools/perf/Documentation/perf-timechart.txt
index d7b79e2..1632b0e 100644
--- a/tools/perf/Documentation/perf-timechart.txt
+++ b/tools/perf/Documentation/perf-timechart.txt
@@ -27,7 +27,7 @@ OPTIONS
         Select the output file (default: output.svg)
 -i::
 --input=::
-        Select the input file (default: perf.data)
+        Select the input file (default: perf.data unless stdin is a fifo)
 -w::
 --width=::
         Select the width of the SVG file (default: 1000)
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 46b4c24..a6374cd 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -30,7 +30,7 @@
 
 #include <linux/bitmap.h>
 
-static char		const *input_name = "perf.data";
+static const char	*input_name;
 
 static bool		force, use_tui, use_stdio;
 
@@ -223,7 +223,7 @@ static int __cmd_annotate(void)
 	}
 
 	if (total_nr_samples == 0) {
-		ui__warning("The %s file has no samples!\n", input_name);
+		ui__warning("The %s file has no samples!\n", session->filename);
 		goto out_delete;
 	}
 out_delete:
diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
index 4895668..5248046 100644
--- a/tools/perf/builtin-buildid-list.c
+++ b/tools/perf/builtin-buildid-list.c
@@ -18,7 +18,7 @@
 
 #include <libelf.h>
 
-static char const *input_name = "perf.data";
+static const char *input_name;
 static bool force;
 static bool show_kernel;
 static bool with_hits;
@@ -71,16 +71,24 @@ static int perf_session__list_build_ids(void)
 {
 	struct perf_session *session;
 
+	elf_version(EV_CURRENT);
+
 	session = perf_session__new(input_name, O_RDONLY, force, false,
 				    &build_id__mark_dso_hit_ops);
 	if (session == NULL)
 		return -1;
 
+	/*
+	 * See if this is an ELF file first:
+	 */
+	if (filename__fprintf_build_id(session->filename, stdout))
+		goto out;
+
 	if (with_hits)
 		perf_session__process_events(session, &build_id__mark_dso_hit_ops);
 
 	perf_session__fprintf_dsos_buildid(session, stdout, with_hits);
-
+out:
 	perf_session__delete(session);
 	return 0;
 }
@@ -90,13 +98,6 @@ static int __cmd_buildid_list(void)
 	if (show_kernel)
 		return sysfs__fprintf_build_id(stdout);
 
-	elf_version(EV_CURRENT);
-	/*
- 	 * See if this is an ELF file first:
- 	 */
-	if (filename__fprintf_build_id(input_name, stdout))
-		return 0;
-
 	return perf_session__list_build_ids();
 }
 
diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
index 4c5e9e0..2676032 100644
--- a/tools/perf/builtin-evlist.c
+++ b/tools/perf/builtin-evlist.c
@@ -15,7 +15,7 @@
 #include "util/parse-options.h"
 #include "util/session.h"
 
-static char const *input_name = "perf.data";
+static const char *input_name;
 
 static int __cmd_evlist(void)
 {
diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index 225e963..f2a1c33 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -18,7 +18,7 @@
 struct alloc_stat;
 typedef int (*sort_fn_t)(struct alloc_stat *, struct alloc_stat *);
 
-static char const		*input_name = "perf.data";
+static const char		*input_name;
 
 static int			alloc_flag;
 static int			caller_flag;
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 899080a..c743b2a 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -325,7 +325,7 @@ alloc_failed:
 	die("memory allocation failed\n");
 }
 
-static char			const *input_name = "perf.data";
+static const char *input_name;
 
 struct raw_event_sample {
 	u32			size;
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 88ca2d4..19c2217 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -35,7 +35,7 @@
 
 #include <linux/bitmap.h>
 
-static char		const *input_name = "perf.data";
+static const char	*input_name;
 
 static bool		force, use_tui, use_stdio;
 static bool		hide_unresolved;
@@ -327,7 +327,7 @@ static int __cmd_report(void)
 	}
 
 	if (nr_samples == 0) {
-		ui__warning("The %s file has no samples!\n", input_name);
+		ui__warning("The %s file has no samples!\n", session->filename);
 		goto out_delete;
 	}
 
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 5177964..9816fa9 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -19,7 +19,7 @@
 #include <pthread.h>
 #include <math.h>
 
-static char			const *input_name = "perf.data";
+static const char		*input_name;
 
 static char			default_sort_order[] = "avg, max, switch, runtime";
 static const char		*sort_order = default_sort_order;
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 82cca10..6d74933 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -433,7 +433,7 @@ static int cleanup_scripting(void)
 	return scripting_ops->stop_script();
 }
 
-static char const		*input_name = "perf.data";
+static const char *input_name;
 
 static int process_sample_event(union perf_event *event,
 				struct perf_sample *sample,
@@ -1302,7 +1302,7 @@ int cmd_script(int argc, const char **argv, const char *prefix __used)
 			return -1;
 		}
 
-		input = open(input_name, O_RDONLY);
+		input = open(session->filename, O_RDONLY);	/* input_name */
 		if (input < 0) {
 			perror("failed to open file");
 			exit(-1);
diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
index aa26f4d..08e4fd0 100644
--- a/tools/perf/builtin-timechart.c
+++ b/tools/perf/builtin-timechart.c
@@ -36,8 +36,8 @@
 #define PWR_EVENT_EXIT -1
 
 
-static char		const *input_name = "perf.data";
-static char		const *output_name = "output.svg";
+static const char	*input_name;
+static const char	*output_name = "output.svg";
 
 static unsigned int	numcpus;
 static u64		min_freq;	/* Lowest CPU frequency seen */
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 2ad9c10..fdd135d 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -132,8 +132,19 @@ struct perf_session *perf_session__new(const char *filename, int mode,
 				       bool force, bool repipe,
 				       struct perf_event_ops *ops)
 {
-	size_t len = filename ? strlen(filename) : 0;
-	struct perf_session *self = zalloc(sizeof(*self) + len);
+	struct perf_session *self;
+	struct stat st;
+	size_t len;
+
+	if (!filename || !strlen(filename)) {
+		if (!fstat(STDIN_FILENO, &st) && S_ISFIFO(st.st_mode))
+			filename = "-";
+		else
+			filename = "perf.data";
+	}
+
+	len = strlen(filename);
+	self = zalloc(sizeof(*self) + len);
 
 	if (self == NULL)
 		goto out;
-- 
1.7.7

Re: [PATCH 07/10] perf report: Accept fifos as input file

[Arnaldo Carvalho de Melo]

Em Tue, Dec 06, 2011 at 11:32:37AM +0100, Robert Richter escreveu:
> The default input file for perf report is not handled the same way as
> perf record does it for its output file. This leads to unexpected
> behavior of perf report, etc. E.g.:
> 
>  # perf record -a -e cpu-cycles sleep 2 | perf report | cat
>  failed to open perf.data: No such file or directory  (try 'perf record' first)
>
> While perf record writes to a fifo, perf report expects perf.data to
> be read. This patch changes this to accept fifos as input file.

Nice feature!

The idea of writing to a pipe initially considered just 'perf script'
iirc, but having it working for all commands like this is indeed a good
thing.
 
> Applies to the following commands:
> 
>  perf annotate
>  perf buildid-list
>  perf evlist
>  perf kmem
>  perf lock
>  perf report
>  perf sched
>  perf script
>  perf timechart
> 
> Also fixes char const* -> const char* type declaration for filename
> strings.
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  tools/perf/Documentation/perf-annotate.txt     |    2 +-
>  tools/perf/Documentation/perf-buildid-list.txt |    2 +-
>  tools/perf/Documentation/perf-evlist.txt       |    2 +-
>  tools/perf/Documentation/perf-kmem.txt         |    2 +-
>  tools/perf/Documentation/perf-lock.txt         |    2 +-
>  tools/perf/Documentation/perf-report.txt       |    2 +-
>  tools/perf/Documentation/perf-sched.txt        |    2 +-
>  tools/perf/Documentation/perf-script.txt       |    2 +-
>  tools/perf/Documentation/perf-timechart.txt    |    2 +-
>  tools/perf/builtin-annotate.c                  |    4 ++--
>  tools/perf/builtin-buildid-list.c              |   19 ++++++++++---------
>  tools/perf/builtin-evlist.c                    |    2 +-
>  tools/perf/builtin-kmem.c                      |    2 +-
>  tools/perf/builtin-lock.c                      |    2 +-
>  tools/perf/builtin-report.c                    |    4 ++--
>  tools/perf/builtin-sched.c                     |    2 +-
>  tools/perf/builtin-script.c                    |    4 ++--
>  tools/perf/builtin-timechart.c                 |    4 ++--
>  tools/perf/util/session.c                      |   15 +++++++++++++--
>  19 files changed, 44 insertions(+), 32 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
> index fe6762e..0d0f217 100644
> --- a/tools/perf/Documentation/perf-annotate.txt
> +++ b/tools/perf/Documentation/perf-annotate.txt
> @@ -22,7 +22,7 @@ OPTIONS
>  -------
>  -i::
>  --input=::
> -        Input file name. (default: perf.data)
> +        Input file name. (default: perf.data unless stdin is a fifo)
>  
>  -d::
>  --dsos=<dso[,dso...]>::
> diff --git a/tools/perf/Documentation/perf-buildid-list.txt b/tools/perf/Documentation/perf-buildid-list.txt
> index cc22325..25c52ef 100644
> --- a/tools/perf/Documentation/perf-buildid-list.txt
> +++ b/tools/perf/Documentation/perf-buildid-list.txt
> @@ -26,7 +26,7 @@ OPTIONS
>          Show only DSOs with hits.
>  -i::
>  --input=::
> -        Input file name. (default: perf.data)
> +        Input file name. (default: perf.data unless stdin is a fifo)
>  -f::
>  --force::
>  	Don't do ownership validation.
> diff --git a/tools/perf/Documentation/perf-evlist.txt b/tools/perf/Documentation/perf-evlist.txt
> index 0cada9e..0507ec7 100644
> --- a/tools/perf/Documentation/perf-evlist.txt
> +++ b/tools/perf/Documentation/perf-evlist.txt
> @@ -18,7 +18,7 @@ OPTIONS
>  -------
>  -i::
>  --input=::
> -        Input file name. (default: perf.data)
> +        Input file name. (default: perf.data unless stdin is a fifo)
>  
>  SEE ALSO
>  --------
> diff --git a/tools/perf/Documentation/perf-kmem.txt b/tools/perf/Documentation/perf-kmem.txt
> index a52fcde..7c8fbbf 100644
> --- a/tools/perf/Documentation/perf-kmem.txt
> +++ b/tools/perf/Documentation/perf-kmem.txt
> @@ -23,7 +23,7 @@ OPTIONS
>  -------
>  -i <file>::
>  --input=<file>::
> -	Select the input file (default: perf.data)
> +	Select the input file (default: perf.data unless stdin is a fifo)
>  
>  --caller::
>  	Show per-callsite statistics
> diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
> index 4a26a2f..d6b2a4f 100644
> --- a/tools/perf/Documentation/perf-lock.txt
> +++ b/tools/perf/Documentation/perf-lock.txt
> @@ -29,7 +29,7 @@ COMMON OPTIONS
>  
>  -i::
>  --input=<file>::
> -        Input file name.
> +        Input file name. (default: perf.data unless stdin is a fifo)
>  
>  -v::
>  --verbose::
> diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
> index 212f24d..a67be86 100644
> --- a/tools/perf/Documentation/perf-report.txt
> +++ b/tools/perf/Documentation/perf-report.txt
> @@ -19,7 +19,7 @@ OPTIONS
>  -------
>  -i::
>  --input=::
> -        Input file name. (default: perf.data)
> +        Input file name. (default: perf.data unless stdin is a fifo)
>  
>  -v::
>  --verbose::
> diff --git a/tools/perf/Documentation/perf-sched.txt b/tools/perf/Documentation/perf-sched.txt
> index 5b212b5..8ff4df9 100644
> --- a/tools/perf/Documentation/perf-sched.txt
> +++ b/tools/perf/Documentation/perf-sched.txt
> @@ -40,7 +40,7 @@ OPTIONS
>  -------
>  -i::
>  --input=<file>::
> -        Input file name. (default: perf.data)
> +        Input file name. (default: perf.data unless stdin is a fifo)
>  
>  -v::
>  --verbose::
> diff --git a/tools/perf/Documentation/perf-script.txt b/tools/perf/Documentation/perf-script.txt
> index dec87ec..d465c11 100644
> --- a/tools/perf/Documentation/perf-script.txt
> +++ b/tools/perf/Documentation/perf-script.txt
> @@ -106,7 +106,7 @@ OPTIONS
>  
>  -i::
>  --input=::
> -        Input file name.
> +        Input file name. (default: perf.data unless stdin is a fifo)
>  
>  -d::
>  --debug-mode::
> diff --git a/tools/perf/Documentation/perf-timechart.txt b/tools/perf/Documentation/perf-timechart.txt
> index d7b79e2..1632b0e 100644
> --- a/tools/perf/Documentation/perf-timechart.txt
> +++ b/tools/perf/Documentation/perf-timechart.txt
> @@ -27,7 +27,7 @@ OPTIONS
>          Select the output file (default: output.svg)
>  -i::
>  --input=::
> -        Select the input file (default: perf.data)
> +        Select the input file (default: perf.data unless stdin is a fifo)
>  -w::
>  --width=::
>          Select the width of the SVG file (default: 1000)
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 46b4c24..a6374cd 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -30,7 +30,7 @@
>  
>  #include <linux/bitmap.h>
>  
> -static char		const *input_name = "perf.data";
> +static const char	*input_name;
>  
>  static bool		force, use_tui, use_stdio;
>  
> @@ -223,7 +223,7 @@ static int __cmd_annotate(void)
>  	}
>  
>  	if (total_nr_samples == 0) {
> -		ui__warning("The %s file has no samples!\n", input_name);
> +		ui__warning("The %s file has no samples!\n", session->filename);
>  		goto out_delete;
>  	}
>  out_delete:
> diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
> index 4895668..5248046 100644
> --- a/tools/perf/builtin-buildid-list.c
> +++ b/tools/perf/builtin-buildid-list.c
> @@ -18,7 +18,7 @@
>  
>  #include <libelf.h>
>  
> -static char const *input_name = "perf.data";
> +static const char *input_name;
>  static bool force;
>  static bool show_kernel;
>  static bool with_hits;
> @@ -71,16 +71,24 @@ static int perf_session__list_build_ids(void)
>  {
>  	struct perf_session *session;
>  
> +	elf_version(EV_CURRENT);
> +
>  	session = perf_session__new(input_name, O_RDONLY, force, false,
>  				    &build_id__mark_dso_hit_ops);
>  	if (session == NULL)
>  		return -1;
>  
> +	/*
> +	 * See if this is an ELF file first:
> +	 */
> +	if (filename__fprintf_build_id(session->filename, stdout))
> +		goto out;
> +
>  	if (with_hits)
>  		perf_session__process_events(session, &build_id__mark_dso_hit_ops);
>  
>  	perf_session__fprintf_dsos_buildid(session, stdout, with_hits);
> -
> +out:
>  	perf_session__delete(session);
>  	return 0;
>  }
> @@ -90,13 +98,6 @@ static int __cmd_buildid_list(void)
>  	if (show_kernel)
>  		return sysfs__fprintf_build_id(stdout);
>  
> -	elf_version(EV_CURRENT);
> -	/*
> - 	 * See if this is an ELF file first:
> - 	 */
> -	if (filename__fprintf_build_id(input_name, stdout))
> -		return 0;
> -
>  	return perf_session__list_build_ids();
>  }
>  
> diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
> index 4c5e9e0..2676032 100644
> --- a/tools/perf/builtin-evlist.c
> +++ b/tools/perf/builtin-evlist.c
> @@ -15,7 +15,7 @@
>  #include "util/parse-options.h"
>  #include "util/session.h"
>  
> -static char const *input_name = "perf.data";
> +static const char *input_name;
>  
>  static int __cmd_evlist(void)
>  {
> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> index 225e963..f2a1c33 100644
> --- a/tools/perf/builtin-kmem.c
> +++ b/tools/perf/builtin-kmem.c
> @@ -18,7 +18,7 @@
>  struct alloc_stat;
>  typedef int (*sort_fn_t)(struct alloc_stat *, struct alloc_stat *);
>  
> -static char const		*input_name = "perf.data";
> +static const char		*input_name;
>  
>  static int			alloc_flag;
>  static int			caller_flag;
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 899080a..c743b2a 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -325,7 +325,7 @@ alloc_failed:
>  	die("memory allocation failed\n");
>  }
>  
> -static char			const *input_name = "perf.data";
> +static const char *input_name;
>  
>  struct raw_event_sample {
>  	u32			size;
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 88ca2d4..19c2217 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -35,7 +35,7 @@
>  
>  #include <linux/bitmap.h>
>  
> -static char		const *input_name = "perf.data";
> +static const char	*input_name;
>  
>  static bool		force, use_tui, use_stdio;
>  static bool		hide_unresolved;
> @@ -327,7 +327,7 @@ static int __cmd_report(void)
>  	}
>  
>  	if (nr_samples == 0) {
> -		ui__warning("The %s file has no samples!\n", input_name);
> +		ui__warning("The %s file has no samples!\n", session->filename);
>  		goto out_delete;
>  	}
>  
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 5177964..9816fa9 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -19,7 +19,7 @@
>  #include <pthread.h>
>  #include <math.h>
>  
> -static char			const *input_name = "perf.data";
> +static const char		*input_name;
>  
>  static char			default_sort_order[] = "avg, max, switch, runtime";
>  static const char		*sort_order = default_sort_order;
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 82cca10..6d74933 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -433,7 +433,7 @@ static int cleanup_scripting(void)
>  	return scripting_ops->stop_script();
>  }
>  
> -static char const		*input_name = "perf.data";
> +static const char *input_name;
>  
>  static int process_sample_event(union perf_event *event,
>  				struct perf_sample *sample,
> @@ -1302,7 +1302,7 @@ int cmd_script(int argc, const char **argv, const char *prefix __used)
>  			return -1;
>  		}
>  
> -		input = open(input_name, O_RDONLY);
> +		input = open(session->filename, O_RDONLY);	/* input_name */
>  		if (input < 0) {
>  			perror("failed to open file");
>  			exit(-1);
> diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
> index aa26f4d..08e4fd0 100644
> --- a/tools/perf/builtin-timechart.c
> +++ b/tools/perf/builtin-timechart.c
> @@ -36,8 +36,8 @@
>  #define PWR_EVENT_EXIT -1
>  
>  
> -static char		const *input_name = "perf.data";
> -static char		const *output_name = "output.svg";
> +static const char	*input_name;
> +static const char	*output_name = "output.svg";
>  
>  static unsigned int	numcpus;
>  static u64		min_freq;	/* Lowest CPU frequency seen */
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 2ad9c10..fdd135d 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -132,8 +132,19 @@ struct perf_session *perf_session__new(const char *filename, int mode,
>  				       bool force, bool repipe,
>  				       struct perf_event_ops *ops)
>  {
> -	size_t len = filename ? strlen(filename) : 0;
> -	struct perf_session *self = zalloc(sizeof(*self) + len);
> +	struct perf_session *self;
> +	struct stat st;
> +	size_t len;
> +
> +	if (!filename || !strlen(filename)) {
> +		if (!fstat(STDIN_FILENO, &st) && S_ISFIFO(st.st_mode))
> +			filename = "-";
> +		else
> +			filename = "perf.data";
> +	}
> +
> +	len = strlen(filename);
> +	self = zalloc(sizeof(*self) + len);
>  
>  	if (self == NULL)
>  		goto out;
> -- 
> 1.7.7
> 

[PATCH 08/10] perf tool: Unify handling of features when writing feature section

[Robert Richter]

The features HEADER_TRACE_INFO and HEADER_BUILD_ID are handled
different when writing the feature section. All other features are
simply disabled on failure and writing the section goes on without
returning an error. There is no reason for these special cases. This
patch unifies handling of the features.

This should be ok since all features can be parsed independently.
Offset and size of a feature's block is stored in struct perf_file_
section right after the data block of perf.data (see perf_session__
write_header()). Thus, if a feature does not exist then other features
can be processed anyway.

Also moving special code for HEADER_BUILD_ID out to write_build_id().

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 tools/perf/util/header.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index dda3c5f..5cfe06a 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -445,6 +445,9 @@ static int write_build_id(int fd, struct perf_header *h,
 
 	session = container_of(h, struct perf_session, header);
 
+	if (!perf_session__read_build_ids(session, true))
+		return -1;
+
 	err = dsos__write_buildid_table(h, fd);
 	if (err < 0) {
 		pr_debug("failed to write buildid table\n");
@@ -1413,10 +1416,6 @@ static int perf_header__adds_write(struct perf_header *header,
 
 	session = container_of(header, struct perf_session, header);
 
-	if (perf_header__has_feat(header, HEADER_BUILD_ID &&
-	    !perf_session__read_build_ids(session, true)))
-		perf_header__clear_feat(header, HEADER_BUILD_ID);
-
 	nr_sections = bitmap_weight(header->adds_features, HEADER_FEAT_BITS);
 	if (!nr_sections)
 		return 0;
@@ -1432,13 +1431,11 @@ static int perf_header__adds_write(struct perf_header *header,
 
 	err = do_write_feat(fd, header, HEADER_TRACE_INFO, &p, evlist);
 	if (err)
-		goto out_free;
+		perf_header__clear_feat(header, HEADER_TRACE_INFO);
 
 	err = do_write_feat(fd, header, HEADER_BUILD_ID, &p, evlist);
-	if (err) {
+	if (err)
 		perf_header__clear_feat(header, HEADER_BUILD_ID);
-		goto out_free;
-	}
 
 	err = do_write_feat(fd, header, HEADER_HOSTNAME, &p, evlist);
 	if (err)
@@ -1496,7 +1493,6 @@ static int perf_header__adds_write(struct perf_header *header,
 	err = do_write(fd, feat_sec, sec_size);
 	if (err < 0)
 		pr_debug("failed to write feature section\n");
-out_free:
 	free(feat_sec);
 	return err;
 }
-- 
1.7.7

Re: [PATCH 08/10] perf tool: Unify handling of features when writing feature section

[Arnaldo Carvalho de Melo]

Em Tue, Dec 06, 2011 at 11:32:38AM +0100, Robert Richter escreveu:
> The features HEADER_TRACE_INFO and HEADER_BUILD_ID are handled
> different when writing the feature section. All other features are
> simply disabled on failure and writing the section goes on without
> returning an error. There is no reason for these special cases. This
> patch unifies handling of the features.
> 
> This should be ok since all features can be parsed independently.
> Offset and size of a feature's block is stored in struct perf_file_
> section right after the data block of perf.data (see perf_session__
> write_header()). Thus, if a feature does not exist then other features
> can be processed anyway.
> 
> Also moving special code for HEADER_BUILD_ID out to write_build_id().

For this one I just would add a big fat warning that if there are
build-ids on the system but the table can't be written, then extreme
care has to be taken when doing a perf report.

I.e. one has to be completely sure that the binaries hasn't changed if
not validating the build-ids.

If you fix that please add this warning as well when no build-ids are
found, which hopefully is the odd case these days as all distros I'm
aware of have build-ids in all DSOs.

- Arnaldo

> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Stephane Eranian <eranian@google.com>
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  tools/perf/util/header.c |   14 +++++---------
>  1 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index dda3c5f..5cfe06a 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -445,6 +445,9 @@ static int write_build_id(int fd, struct perf_header *h,
>  
>  	session = container_of(h, struct perf_session, header);
>  
> +	if (!perf_session__read_build_ids(session, true))
> +		return -1;
> +
>  	err = dsos__write_buildid_table(h, fd);
>  	if (err < 0) {
>  		pr_debug("failed to write buildid table\n");
> @@ -1413,10 +1416,6 @@ static int perf_header__adds_write(struct perf_header *header,
>  
>  	session = container_of(header, struct perf_session, header);
>  
> -	if (perf_header__has_feat(header, HEADER_BUILD_ID &&
> -	    !perf_session__read_build_ids(session, true)))
> -		perf_header__clear_feat(header, HEADER_BUILD_ID);
> -
>  	nr_sections = bitmap_weight(header->adds_features, HEADER_FEAT_BITS);
>  	if (!nr_sections)
>  		return 0;
> @@ -1432,13 +1431,11 @@ static int perf_header__adds_write(struct perf_header *header,
>  
>  	err = do_write_feat(fd, header, HEADER_TRACE_INFO, &p, evlist);
>  	if (err)
> -		goto out_free;
> +		perf_header__clear_feat(header, HEADER_TRACE_INFO);
>  
>  	err = do_write_feat(fd, header, HEADER_BUILD_ID, &p, evlist);
> -	if (err) {
> +	if (err)
>  		perf_header__clear_feat(header, HEADER_BUILD_ID);
> -		goto out_free;
> -	}
>  
>  	err = do_write_feat(fd, header, HEADER_HOSTNAME, &p, evlist);
>  	if (err)
> @@ -1496,7 +1493,6 @@ static int perf_header__adds_write(struct perf_header *header,
>  	err = do_write(fd, feat_sec, sec_size);
>  	if (err < 0)
>  		pr_debug("failed to write feature section\n");
> -out_free:
>  	free(feat_sec);
>  	return err;
>  }
> -- 
> 1.7.7
> 

Re: [PATCH 08/10] perf tool: Unify handling of features when writing feature section

[Robert Richter]

On 06.12.11 11:36:30, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 06, 2011 at 11:32:38AM +0100, Robert Richter escreveu:
> > Also moving special code for HEADER_BUILD_ID out to write_build_id().
> 
> For this one I just would add a big fat warning that if there are
> build-ids on the system but the table can't be written, then extreme
> care has to be taken when doing a perf report.
> 
> I.e. one has to be completely sure that the binaries hasn't changed if
> not validating the build-ids.
> 
> If you fix that please add this warning as well when no build-ids are
> found, which hopefully is the odd case these days as all distros I'm
> aware of have build-ids in all DSOs.

What about the following change in addition? perf record then still
stops with an error, but --no-buildid could be used to proceed anyway:

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 766fa0a..80e08ca 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -493,6 +493,13 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
                        return err;
        }
 
+       if (!no_buildid
+           && !perf_header__has_feat(&session->header, HEADER_BUILD_ID)) {
+               pr_err("Couldn't generate buildids. "
+                      "Use --no-buildid option to profile anyway.\n");
+               return -1;
+       }
+
        rec->post_processing_offset = lseek(output, 0, SEEK_CUR);
 
        machine = perf_session__find_host_machine(session);


-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 08/10] perf tool: Unify handling of features when writing feature section

[Arnaldo Carvalho de Melo]

Em Tue, Dec 06, 2011 at 05:33:58PM +0100, Robert Richter escreveu:
> On 06.12.11 11:36:30, Arnaldo Carvalho de Melo wrote:
> > If you fix that please add this warning as well when no build-ids are
> > found, which hopefully is the odd case these days as all distros I'm
> > aware of have build-ids in all DSOs.
> 
> What about the following change in addition? perf record then still
> stops with an error, but --no-buildid could be used to proceed anyway:
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 766fa0a..80e08ca 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -493,6 +493,13 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>                         return err;
>         }
>  
> +       if (!no_buildid
> +           && !perf_header__has_feat(&session->header, HEADER_BUILD_ID)) {
> +               pr_err("Couldn't collect buildids. "
                         "Your report results may be misleading if profiled "
                         "DSOs changed after the record session.\n"
> +                      "Use --no-buildid option if you know that "
                         "there where no changes in the profiled DSOs.\n");
> +               return -1;
> +       }
> +
>         rec->post_processing_offset = lseek(output, 0, SEEK_CUR);

I can do these changes if you agree with this wording,

- Arnaldo

Re: [PATCH 08/10] perf tool: Unify handling of features when writing feature section

[Robert Richter]

On 07.12.11 12:14:09, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 06, 2011 at 05:33:58PM +0100, Robert Richter escreveu:
> > On 06.12.11 11:36:30, Arnaldo Carvalho de Melo wrote:
> > > If you fix that please add this warning as well when no build-ids are
> > > found, which hopefully is the odd case these days as all distros I'm
> > > aware of have build-ids in all DSOs.
> > 
> > What about the following change in addition? perf record then still
> > stops with an error, but --no-buildid could be used to proceed anyway:
> > 
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index 766fa0a..80e08ca 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -493,6 +493,13 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
> >                         return err;
> >         }
> >  
> > +       if (!no_buildid
> > +           && !perf_header__has_feat(&session->header, HEADER_BUILD_ID)) {
> > +               pr_err("Couldn't collect buildids. "
>                          "Your report results may be misleading if profiled "
>                          "DSOs changed after the record session.\n"
> > +                      "Use --no-buildid option if you know that "
>                          "there where no changes in the profiled DSOs.\n");
> > +               return -1;
> > +       }
> > +
> >         rec->post_processing_offset = lseek(output, 0, SEEK_CUR);
> 
> I can do these changes if you agree with this wording,

Yes, I am fine with it.

Thanks,

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 09/10] perf tools: Improve macros for struct feature_ops

[Robert Richter]

Reducing duplication and line size by extending function names for
print and write from a single name.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 tools/perf/util/header.c |   40 ++++++++++++++++++++++------------------
 1 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 5cfe06a..915a5f7 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1311,26 +1311,30 @@ struct feature_ops {
 	bool full_only;
 };
 
-#define FEAT_OPA(n, w, p) \
-	[n] = { .name = #n, .write = w, .print = p }
-#define FEAT_OPF(n, w, p) \
-	[n] = { .name = #n, .write = w, .print = p, .full_only = true }
+#define FEAT_OPA(n, func) \
+	[n] = { .name = #n, .write = write_##func, .print = print_##func }
+#define FEAT_OPF(n, func) \
+	[n] = { .name = #n, .write = write_##func, .print = print_##func, .full_only = true }
+
+/* feature_ops not implemented: */
+#define print_trace_info		NULL
+#define print_build_id			NULL
 
 static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = {
-	FEAT_OPA(HEADER_TRACE_INFO, write_trace_info, NULL),
-	FEAT_OPA(HEADER_BUILD_ID, write_build_id, NULL),
-	FEAT_OPA(HEADER_HOSTNAME, write_hostname, print_hostname),
-	FEAT_OPA(HEADER_OSRELEASE, write_osrelease, print_osrelease),
-	FEAT_OPA(HEADER_VERSION, write_version, print_version),
-	FEAT_OPA(HEADER_ARCH, write_arch, print_arch),
-	FEAT_OPA(HEADER_NRCPUS, write_nrcpus, print_nrcpus),
-	FEAT_OPA(HEADER_CPUDESC, write_cpudesc, print_cpudesc),
-	FEAT_OPA(HEADER_CPUID, write_cpuid, print_cpuid),
-	FEAT_OPA(HEADER_TOTAL_MEM, write_total_mem, print_total_mem),
-	FEAT_OPA(HEADER_EVENT_DESC, write_event_desc, print_event_desc),
-	FEAT_OPA(HEADER_CMDLINE, write_cmdline, print_cmdline),
-	FEAT_OPF(HEADER_CPU_TOPOLOGY, write_cpu_topology, print_cpu_topology),
-	FEAT_OPF(HEADER_NUMA_TOPOLOGY, write_numa_topology, print_numa_topology),
+	FEAT_OPA(HEADER_TRACE_INFO,	trace_info),
+	FEAT_OPA(HEADER_BUILD_ID,	build_id),
+	FEAT_OPA(HEADER_HOSTNAME,	hostname),
+	FEAT_OPA(HEADER_OSRELEASE,	osrelease),
+	FEAT_OPA(HEADER_VERSION,	version),
+	FEAT_OPA(HEADER_ARCH,		arch),
+	FEAT_OPA(HEADER_NRCPUS,		nrcpus),
+	FEAT_OPA(HEADER_CPUDESC,	cpudesc),
+	FEAT_OPA(HEADER_CPUID,		cpuid),
+	FEAT_OPA(HEADER_TOTAL_MEM,	total_mem),
+	FEAT_OPA(HEADER_EVENT_DESC,	event_desc),
+	FEAT_OPA(HEADER_CMDLINE,	cmdline),
+	FEAT_OPF(HEADER_CPU_TOPOLOGY,	cpu_topology),
+	FEAT_OPF(HEADER_NUMA_TOPOLOGY,	numa_topology),
 };
 
 struct header_print_data {
-- 
1.7.7

Re: [PATCH 09/10] perf tools: Improve macros for struct feature_ops

[Arnaldo Carvalho de Melo]

Em Tue, Dec 06, 2011 at 11:32:39AM +0100, Robert Richter escreveu:
> Reducing duplication and line size by extending function names for
> print and write from a single name.

Ok, more compact.
 
- Arnaldo

[PATCH 10/10] perf tools: Use for_each_set_bit() to iterate over feature flags

[Robert Richter]

This patch introduces the for_each_set_bit() macro and modifies
feature implementation to use it.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 tools/perf/util/header.c               |  118 ++++++++------------------------
 tools/perf/util/header.h               |    6 +-
 tools/perf/util/include/linux/bitops.h |  118 ++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+), 93 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 915a5f7..7bea6e4 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -8,6 +8,7 @@
 #include <stdlib.h>
 #include <linux/list.h>
 #include <linux/kernel.h>
+#include <linux/bitops.h>
 #include <sys/utsname.h>
 
 #include "evlist.h"
@@ -1353,7 +1354,7 @@ static int perf_file_section__fprintf_info(struct perf_file_section *section,
 				"%d, continuing...\n", section->offset, feat);
 		return 0;
 	}
-	if (feat < HEADER_TRACE_INFO || feat >= HEADER_LAST_FEATURE) {
+	if (feat >= HEADER_LAST_FEATURE) {
 		pr_warning("unknown feature %d\n", feat);
 		return 0;
 	}
@@ -1390,6 +1391,8 @@ static int do_write_feat(int fd, struct perf_header *h, int type,
 	int ret = 0;
 
 	if (perf_header__has_feat(h, type)) {
+		if (!feat_ops[type].write)
+			return -1;
 
 		(*p)->offset = lseek(fd, 0, SEEK_CUR);
 
@@ -1416,6 +1419,7 @@ static int perf_header__adds_write(struct perf_header *header,
 	struct perf_file_section *feat_sec, *p;
 	int sec_size;
 	u64 sec_start;
+	int feat;
 	int err;
 
 	session = container_of(header, struct perf_session, header);
@@ -1433,61 +1437,10 @@ static int perf_header__adds_write(struct perf_header *header,
 	sec_start = header->data_offset + header->data_size;
 	lseek(fd, sec_start + sec_size, SEEK_SET);
 
-	err = do_write_feat(fd, header, HEADER_TRACE_INFO, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_TRACE_INFO);
-
-	err = do_write_feat(fd, header, HEADER_BUILD_ID, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_BUILD_ID);
-
-	err = do_write_feat(fd, header, HEADER_HOSTNAME, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_HOSTNAME);
-
-	err = do_write_feat(fd, header, HEADER_OSRELEASE, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_OSRELEASE);
-
-	err = do_write_feat(fd, header, HEADER_VERSION, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_VERSION);
-
-	err = do_write_feat(fd, header, HEADER_ARCH, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_ARCH);
-
-	err = do_write_feat(fd, header, HEADER_NRCPUS, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_NRCPUS);
-
-	err = do_write_feat(fd, header, HEADER_CPUDESC, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_CPUDESC);
-
-	err = do_write_feat(fd, header, HEADER_CPUID, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_CPUID);
-
-	err = do_write_feat(fd, header, HEADER_TOTAL_MEM, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_TOTAL_MEM);
-
-	err = do_write_feat(fd, header, HEADER_CMDLINE, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_CMDLINE);
-
-	err = do_write_feat(fd, header, HEADER_EVENT_DESC, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_EVENT_DESC);
-
-	err = do_write_feat(fd, header, HEADER_CPU_TOPOLOGY, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_CPU_TOPOLOGY);
-
-	err = do_write_feat(fd, header, HEADER_NUMA_TOPOLOGY, &p, evlist);
-	if (err)
-		perf_header__clear_feat(header, HEADER_NUMA_TOPOLOGY);
+	for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
+		if (do_write_feat(fd, header, feat, &p, evlist))
+			perf_header__clear_feat(header, feat);
+	}
 
 	lseek(fd, sec_start, SEEK_SET);
 	/*
@@ -1634,20 +1587,20 @@ static int perf_header__getbuffer64(struct perf_header *header,
 int perf_header__process_sections(struct perf_header *header, int fd,
 				  void *data,
 				  int (*process)(struct perf_file_section *section,
-				  struct perf_header *ph,
-				  int feat, int fd, void *data))
+						 struct perf_header *ph,
+						 int feat, int fd, void *data))
 {
-	struct perf_file_section *feat_sec;
+	struct perf_file_section *feat_sec, *sec;
 	int nr_sections;
 	int sec_size;
-	int idx = 0;
-	int err = -1, feat = 1;
+	int feat;
+	int err;
 
 	nr_sections = bitmap_weight(header->adds_features, HEADER_FEAT_BITS);
 	if (!nr_sections)
 		return 0;
 
-	feat_sec = calloc(sizeof(*feat_sec), nr_sections);
+	feat_sec = sec = calloc(sizeof(*feat_sec), nr_sections);
 	if (!feat_sec)
 		return -1;
 
@@ -1655,20 +1608,16 @@ int perf_header__process_sections(struct perf_header *header, int fd,
 
 	lseek(fd, header->data_offset + header->data_size, SEEK_SET);
 
-	if (perf_header__getbuffer64(header, fd, feat_sec, sec_size))
+	err = perf_header__getbuffer64(header, fd, feat_sec, sec_size);
+	if (err < 0)
 		goto out_free;
 
-	err = 0;
-	while (idx < nr_sections && feat < HEADER_LAST_FEATURE) {
-		if (perf_header__has_feat(header, feat)) {
-			struct perf_file_section *sec = &feat_sec[idx++];
-
-			err = process(sec, header, feat, fd, data);
-			if (err < 0)
-				break;
-		}
-		++feat;
+	for_each_set_bit(feat, header->adds_features, HEADER_LAST_FEATURE) {
+		err = process(sec++, header, feat, fd, data);
+		if (err < 0)
+			goto out_free;
 	}
+	err = 0;
 out_free:
 	free(feat_sec);
 	return err;
@@ -1903,32 +1852,21 @@ static int perf_file_section__process(struct perf_file_section *section,
 		return 0;
 	}
 
+	if (feat >= HEADER_LAST_FEATURE) {
+		pr_debug("unknown feature %d, continuing...\n", feat);
+		return 0;
+	}
+
 	switch (feat) {
 	case HEADER_TRACE_INFO:
 		trace_report(fd, false);
 		break;
-
 	case HEADER_BUILD_ID:
 		if (perf_header__read_build_ids(ph, fd, section->offset, section->size))
 			pr_debug("Failed to read buildids, continuing...\n");
 		break;
-
-	case HEADER_HOSTNAME:
-	case HEADER_OSRELEASE:
-	case HEADER_VERSION:
-	case HEADER_ARCH:
-	case HEADER_NRCPUS:
-	case HEADER_CPUDESC:
-	case HEADER_CPUID:
-	case HEADER_TOTAL_MEM:
-	case HEADER_CMDLINE:
-	case HEADER_EVENT_DESC:
-	case HEADER_CPU_TOPOLOGY:
-	case HEADER_NUMA_TOPOLOGY:
-		break;
-
 	default:
-		pr_debug("unknown feature %d, continuing...\n", feat);
+		break;
 	}
 
 	return 0;
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index 3d5a742..5082efb 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -10,7 +10,8 @@
 #include <linux/bitmap.h>
 
 enum {
-	HEADER_TRACE_INFO = 1,
+	HEADER_RESERVED		= 0,	/* always cleared */
+	HEADER_TRACE_INFO	= 1,
 	HEADER_BUILD_ID,
 
 	HEADER_HOSTNAME,
@@ -27,10 +28,9 @@ enum {
 	HEADER_NUMA_TOPOLOGY,
 
 	HEADER_LAST_FEATURE,
+	HEADER_FEAT_BITS	= 256,
 };
 
-#define HEADER_FEAT_BITS			256
-
 struct perf_file_section {
 	u64 offset;
 	u64 size;
diff --git a/tools/perf/util/include/linux/bitops.h b/tools/perf/util/include/linux/bitops.h
index 305c848..62cdee7 100644
--- a/tools/perf/util/include/linux/bitops.h
+++ b/tools/perf/util/include/linux/bitops.h
@@ -9,6 +9,17 @@
 #define BITS_PER_BYTE           8
 #define BITS_TO_LONGS(nr)       DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
 
+#define for_each_set_bit(bit, addr, size) \
+	for ((bit) = find_first_bit((addr), (size));		\
+	     (bit) < (size);					\
+	     (bit) = find_next_bit((addr), (size), (bit) + 1))
+
+/* same as for_each_set_bit() but use bit as value to start with */
+#define for_each_set_bit_cont(bit, addr, size) \
+	for ((bit) = find_next_bit((addr), (size), (bit));	\
+	     (bit) < (size);					\
+	     (bit) = find_next_bit((addr), (size), (bit) + 1))
+
 static inline void set_bit(int nr, unsigned long *addr)
 {
 	addr[nr / BITS_PER_LONG] |= 1UL << (nr % BITS_PER_LONG);
@@ -30,4 +41,111 @@ static inline unsigned long hweight_long(unsigned long w)
 	return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
 }
 
+#define BITOP_WORD(nr)		((nr) / BITS_PER_LONG)
+
+/**
+ * __ffs - find first bit in word.
+ * @word: The word to search
+ *
+ * Undefined if no bit exists, so code should check against 0 first.
+ */
+static __always_inline unsigned long __ffs(unsigned long word)
+{
+	int num = 0;
+
+#if BITS_PER_LONG == 64
+	if ((word & 0xffffffff) == 0) {
+		num += 32;
+		word >>= 32;
+	}
+#endif
+	if ((word & 0xffff) == 0) {
+		num += 16;
+		word >>= 16;
+	}
+	if ((word & 0xff) == 0) {
+		num += 8;
+		word >>= 8;
+	}
+	if ((word & 0xf) == 0) {
+		num += 4;
+		word >>= 4;
+	}
+	if ((word & 0x3) == 0) {
+		num += 2;
+		word >>= 2;
+	}
+	if ((word & 0x1) == 0)
+		num += 1;
+	return num;
+}
+
+/*
+ * Find the first set bit in a memory region.
+ */
+static inline unsigned long
+find_first_bit(const unsigned long *addr, unsigned long size)
+{
+	const unsigned long *p = addr;
+	unsigned long result = 0;
+	unsigned long tmp;
+
+	while (size & ~(BITS_PER_LONG-1)) {
+		if ((tmp = *(p++)))
+			goto found;
+		result += BITS_PER_LONG;
+		size -= BITS_PER_LONG;
+	}
+	if (!size)
+		return result;
+
+	tmp = (*p) & (~0UL >> (BITS_PER_LONG - size));
+	if (tmp == 0UL)		/* Are any bits set? */
+		return result + size;	/* Nope. */
+found:
+	return result + __ffs(tmp);
+}
+
+/*
+ * Find the next set bit in a memory region.
+ */
+static inline unsigned long
+find_next_bit(const unsigned long *addr, unsigned long size, unsigned long offset)
+{
+	const unsigned long *p = addr + BITOP_WORD(offset);
+	unsigned long result = offset & ~(BITS_PER_LONG-1);
+	unsigned long tmp;
+
+	if (offset >= size)
+		return size;
+	size -= result;
+	offset %= BITS_PER_LONG;
+	if (offset) {
+		tmp = *(p++);
+		tmp &= (~0UL << offset);
+		if (size < BITS_PER_LONG)
+			goto found_first;
+		if (tmp)
+			goto found_middle;
+		size -= BITS_PER_LONG;
+		result += BITS_PER_LONG;
+	}
+	while (size & ~(BITS_PER_LONG-1)) {
+		if ((tmp = *(p++)))
+			goto found_middle;
+		result += BITS_PER_LONG;
+		size -= BITS_PER_LONG;
+	}
+	if (!size)
+		return result;
+	tmp = *p;
+
+found_first:
+	tmp &= (~0UL >> (BITS_PER_LONG - size));
+	if (tmp == 0UL)		/* Are any bits set? */
+		return result + size;	/* Nope. */
+found_middle:
+	return result + __ffs(tmp);
+}
+
 #endif
-- 
1.7.7

Re: [PATCH 10/10] perf tools: Use for_each_set_bit() to iterate over feature flags

[Arnaldo Carvalho de Melo]

Em Tue, Dec 06, 2011 at 11:32:40AM +0100, Robert Richter escreveu:
> This patch introduces the for_each_set_bit() macro and modifies
> feature implementation to use it.

That is cool and thanks for following the convention of using the same
include file as in the kernel proper.

What would be really great would be to just use the kernel one like in

tools/perf/util/include/linux/list.h
tools/perf/util/include/linux/rbtree.h

But that was source of contention in the past when changes in the kernel
side broke the tools, i.e. the headers were not explicitely designed for
such sharing.

I wouldn't mind fixing the tools when such things happen tho, as most of
the time what happens is not breakage, but the tools reaping the
benefits of fixes and improvements done in the much larger hacker pool
that is the landlord of the major source repository where perf lives 8-)

- Arnaldo
 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  tools/perf/util/header.c               |  118 ++++++++------------------------
>  tools/perf/util/header.h               |    6 +-
>  tools/perf/util/include/linux/bitops.h |  118 ++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+), 93 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 915a5f7..7bea6e4 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -8,6 +8,7 @@
>  #include <stdlib.h>
>  #include <linux/list.h>
>  #include <linux/kernel.h>
> +#include <linux/bitops.h>
>  #include <sys/utsname.h>
>  
>  #include "evlist.h"
> @@ -1353,7 +1354,7 @@ static int perf_file_section__fprintf_info(struct perf_file_section *section,
>  				"%d, continuing...\n", section->offset, feat);
>  		return 0;
>  	}
> -	if (feat < HEADER_TRACE_INFO || feat >= HEADER_LAST_FEATURE) {
> +	if (feat >= HEADER_LAST_FEATURE) {
>  		pr_warning("unknown feature %d\n", feat);
>  		return 0;
>  	}
> @@ -1390,6 +1391,8 @@ static int do_write_feat(int fd, struct perf_header *h, int type,
>  	int ret = 0;
>  
>  	if (perf_header__has_feat(h, type)) {
> +		if (!feat_ops[type].write)
> +			return -1;
>  
>  		(*p)->offset = lseek(fd, 0, SEEK_CUR);
>  
> @@ -1416,6 +1419,7 @@ static int perf_header__adds_write(struct perf_header *header,
>  	struct perf_file_section *feat_sec, *p;
>  	int sec_size;
>  	u64 sec_start;
> +	int feat;
>  	int err;
>  
>  	session = container_of(header, struct perf_session, header);
> @@ -1433,61 +1437,10 @@ static int perf_header__adds_write(struct perf_header *header,
>  	sec_start = header->data_offset + header->data_size;
>  	lseek(fd, sec_start + sec_size, SEEK_SET);
>  
> -	err = do_write_feat(fd, header, HEADER_TRACE_INFO, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_TRACE_INFO);
> -
> -	err = do_write_feat(fd, header, HEADER_BUILD_ID, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_BUILD_ID);
> -
> -	err = do_write_feat(fd, header, HEADER_HOSTNAME, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_HOSTNAME);
> -
> -	err = do_write_feat(fd, header, HEADER_OSRELEASE, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_OSRELEASE);
> -
> -	err = do_write_feat(fd, header, HEADER_VERSION, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_VERSION);
> -
> -	err = do_write_feat(fd, header, HEADER_ARCH, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_ARCH);
> -
> -	err = do_write_feat(fd, header, HEADER_NRCPUS, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_NRCPUS);
> -
> -	err = do_write_feat(fd, header, HEADER_CPUDESC, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_CPUDESC);
> -
> -	err = do_write_feat(fd, header, HEADER_CPUID, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_CPUID);
> -
> -	err = do_write_feat(fd, header, HEADER_TOTAL_MEM, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_TOTAL_MEM);
> -
> -	err = do_write_feat(fd, header, HEADER_CMDLINE, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_CMDLINE);
> -
> -	err = do_write_feat(fd, header, HEADER_EVENT_DESC, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_EVENT_DESC);
> -
> -	err = do_write_feat(fd, header, HEADER_CPU_TOPOLOGY, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_CPU_TOPOLOGY);
> -
> -	err = do_write_feat(fd, header, HEADER_NUMA_TOPOLOGY, &p, evlist);
> -	if (err)
> -		perf_header__clear_feat(header, HEADER_NUMA_TOPOLOGY);
> +	for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
> +		if (do_write_feat(fd, header, feat, &p, evlist))
> +			perf_header__clear_feat(header, feat);
> +	}
>  
>  	lseek(fd, sec_start, SEEK_SET);
>  	/*
> @@ -1634,20 +1587,20 @@ static int perf_header__getbuffer64(struct perf_header *header,
>  int perf_header__process_sections(struct perf_header *header, int fd,
>  				  void *data,
>  				  int (*process)(struct perf_file_section *section,
> -				  struct perf_header *ph,
> -				  int feat, int fd, void *data))
> +						 struct perf_header *ph,
> +						 int feat, int fd, void *data))
>  {
> -	struct perf_file_section *feat_sec;
> +	struct perf_file_section *feat_sec, *sec;
>  	int nr_sections;
>  	int sec_size;
> -	int idx = 0;
> -	int err = -1, feat = 1;
> +	int feat;
> +	int err;
>  
>  	nr_sections = bitmap_weight(header->adds_features, HEADER_FEAT_BITS);
>  	if (!nr_sections)
>  		return 0;
>  
> -	feat_sec = calloc(sizeof(*feat_sec), nr_sections);
> +	feat_sec = sec = calloc(sizeof(*feat_sec), nr_sections);
>  	if (!feat_sec)
>  		return -1;
>  
> @@ -1655,20 +1608,16 @@ int perf_header__process_sections(struct perf_header *header, int fd,
>  
>  	lseek(fd, header->data_offset + header->data_size, SEEK_SET);
>  
> -	if (perf_header__getbuffer64(header, fd, feat_sec, sec_size))
> +	err = perf_header__getbuffer64(header, fd, feat_sec, sec_size);
> +	if (err < 0)
>  		goto out_free;
>  
> -	err = 0;
> -	while (idx < nr_sections && feat < HEADER_LAST_FEATURE) {
> -		if (perf_header__has_feat(header, feat)) {
> -			struct perf_file_section *sec = &feat_sec[idx++];
> -
> -			err = process(sec, header, feat, fd, data);
> -			if (err < 0)
> -				break;
> -		}
> -		++feat;
> +	for_each_set_bit(feat, header->adds_features, HEADER_LAST_FEATURE) {
> +		err = process(sec++, header, feat, fd, data);
> +		if (err < 0)
> +			goto out_free;
>  	}
> +	err = 0;
>  out_free:
>  	free(feat_sec);
>  	return err;
> @@ -1903,32 +1852,21 @@ static int perf_file_section__process(struct perf_file_section *section,
>  		return 0;
>  	}
>  
> +	if (feat >= HEADER_LAST_FEATURE) {
> +		pr_debug("unknown feature %d, continuing...\n", feat);
> +		return 0;
> +	}
> +
>  	switch (feat) {
>  	case HEADER_TRACE_INFO:
>  		trace_report(fd, false);
>  		break;
> -
>  	case HEADER_BUILD_ID:
>  		if (perf_header__read_build_ids(ph, fd, section->offset, section->size))
>  			pr_debug("Failed to read buildids, continuing...\n");
>  		break;
> -
> -	case HEADER_HOSTNAME:
> -	case HEADER_OSRELEASE:
> -	case HEADER_VERSION:
> -	case HEADER_ARCH:
> -	case HEADER_NRCPUS:
> -	case HEADER_CPUDESC:
> -	case HEADER_CPUID:
> -	case HEADER_TOTAL_MEM:
> -	case HEADER_CMDLINE:
> -	case HEADER_EVENT_DESC:
> -	case HEADER_CPU_TOPOLOGY:
> -	case HEADER_NUMA_TOPOLOGY:
> -		break;
> -
>  	default:
> -		pr_debug("unknown feature %d, continuing...\n", feat);
> +		break;
>  	}
>  
>  	return 0;
> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> index 3d5a742..5082efb 100644
> --- a/tools/perf/util/header.h
> +++ b/tools/perf/util/header.h
> @@ -10,7 +10,8 @@
>  #include <linux/bitmap.h>
>  
>  enum {
> -	HEADER_TRACE_INFO = 1,
> +	HEADER_RESERVED		= 0,	/* always cleared */
> +	HEADER_TRACE_INFO	= 1,
>  	HEADER_BUILD_ID,
>  
>  	HEADER_HOSTNAME,
> @@ -27,10 +28,9 @@ enum {
>  	HEADER_NUMA_TOPOLOGY,
>  
>  	HEADER_LAST_FEATURE,
> +	HEADER_FEAT_BITS	= 256,
>  };
>  
> -#define HEADER_FEAT_BITS			256
> -
>  struct perf_file_section {
>  	u64 offset;
>  	u64 size;
> diff --git a/tools/perf/util/include/linux/bitops.h b/tools/perf/util/include/linux/bitops.h
> index 305c848..62cdee7 100644
> --- a/tools/perf/util/include/linux/bitops.h
> +++ b/tools/perf/util/include/linux/bitops.h
> @@ -9,6 +9,17 @@
>  #define BITS_PER_BYTE           8
>  #define BITS_TO_LONGS(nr)       DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>  
> +#define for_each_set_bit(bit, addr, size) \
> +	for ((bit) = find_first_bit((addr), (size));		\
> +	     (bit) < (size);					\
> +	     (bit) = find_next_bit((addr), (size), (bit) + 1))
> +
> +/* same as for_each_set_bit() but use bit as value to start with */
> +#define for_each_set_bit_cont(bit, addr, size) \
> +	for ((bit) = find_next_bit((addr), (size), (bit));	\
> +	     (bit) < (size);					\
> +	     (bit) = find_next_bit((addr), (size), (bit) + 1))
> +
>  static inline void set_bit(int nr, unsigned long *addr)
>  {
>  	addr[nr / BITS_PER_LONG] |= 1UL << (nr % BITS_PER_LONG);
> @@ -30,4 +41,111 @@ static inline unsigned long hweight_long(unsigned long w)
>  	return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
>  }
>  
> +#define BITOP_WORD(nr)		((nr) / BITS_PER_LONG)
> +
> +/**
> + * __ffs - find first bit in word.
> + * @word: The word to search
> + *
> + * Undefined if no bit exists, so code should check against 0 first.
> + */
> +static __always_inline unsigned long __ffs(unsigned long word)
> +{
> +	int num = 0;
> +
> +#if BITS_PER_LONG == 64
> +	if ((word & 0xffffffff) == 0) {
> +		num += 32;
> +		word >>= 32;
> +	}
> +#endif
> +	if ((word & 0xffff) == 0) {
> +		num += 16;
> +		word >>= 16;
> +	}
> +	if ((word & 0xff) == 0) {
> +		num += 8;
> +		word >>= 8;
> +	}
> +	if ((word & 0xf) == 0) {
> +		num += 4;
> +		word >>= 4;
> +	}
> +	if ((word & 0x3) == 0) {
> +		num += 2;
> +		word >>= 2;
> +	}
> +	if ((word & 0x1) == 0)
> +		num += 1;
> +	return num;
> +}
> +
> +/*
> + * Find the first set bit in a memory region.
> + */
> +static inline unsigned long
> +find_first_bit(const unsigned long *addr, unsigned long size)
> +{
> +	const unsigned long *p = addr;
> +	unsigned long result = 0;
> +	unsigned long tmp;
> +
> +	while (size & ~(BITS_PER_LONG-1)) {
> +		if ((tmp = *(p++)))
> +			goto found;
> +		result += BITS_PER_LONG;
> +		size -= BITS_PER_LONG;
> +	}
> +	if (!size)
> +		return result;
> +
> +	tmp = (*p) & (~0UL >> (BITS_PER_LONG - size));
> +	if (tmp == 0UL)		/* Are any bits set? */
> +		return result + size;	/* Nope. */
> +found:
> +	return result + __ffs(tmp);
> +}
> +
> +/*
> + * Find the next set bit in a memory region.
> + */
> +static inline unsigned long
> +find_next_bit(const unsigned long *addr, unsigned long size, unsigned long offset)
> +{
> +	const unsigned long *p = addr + BITOP_WORD(offset);
> +	unsigned long result = offset & ~(BITS_PER_LONG-1);
> +	unsigned long tmp;
> +
> +	if (offset >= size)
> +		return size;
> +	size -= result;
> +	offset %= BITS_PER_LONG;
> +	if (offset) {
> +		tmp = *(p++);
> +		tmp &= (~0UL << offset);
> +		if (size < BITS_PER_LONG)
> +			goto found_first;
> +		if (tmp)
> +			goto found_middle;
> +		size -= BITS_PER_LONG;
> +		result += BITS_PER_LONG;
> +	}
> +	while (size & ~(BITS_PER_LONG-1)) {
> +		if ((tmp = *(p++)))
> +			goto found_middle;
> +		result += BITS_PER_LONG;
> +		size -= BITS_PER_LONG;
> +	}
> +	if (!size)
> +		return result;
> +	tmp = *p;
> +
> +found_first:
> +	tmp &= (~0UL >> (BITS_PER_LONG - size));
> +	if (tmp == 0UL)		/* Are any bits set? */
> +		return result + size;	/* Nope. */
> +found_middle:
> +	return result + __ffs(tmp);
> +}
> +
>  #endif
> -- 
> 1.7.7
> 

Re: [PATCH 10/10] perf tools: Use for_each_set_bit() to iterate over feature flags

[Robert Richter]

On 06.12.11 11:40:54, Arnaldo Carvalho de Melo wrote:
> What would be really great would be to just use the kernel one like in
> 
> tools/perf/util/include/linux/list.h
> tools/perf/util/include/linux/rbtree.h
> 
> But that was source of contention in the past when changes in the kernel
> side broke the tools, i.e. the headers were not explicitely designed for
> such sharing.

Yeah, that didn't work out, esp. because of architectural includes and
optimizations. For most cases we don't need that level of optimization
since code is not used in the fast path.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 00/10] perf tools: cleanups, fixes, updates

[Robert Richter]

On 06.12.11 11:32:30, Robert Richter wrote:
> The patch set contains some cleanups, fixes and updates I found worth
> to implement during code review. Some patches can be applied
> independently.
> 
> Patches #1 and #2 are a repost.
> 
> Robert Richter (10):
>   perf script: Fix mem leaks and NULL pointer checks around strdup()s
>   perf script: Implement option for system-wide profiling
>   perf tools: Continue processing header on unknown features
>   perf tools: Fix out-of-bound access to struct perf_session
>   perf tool: Moving code in some files
>   perf report: Setup browser if stdout is a pipe
>   perf report: Accept fifos as input file
>   perf tool: Unify handling of features when writing feature section
>   perf tools: Improve macros for struct feature_ops
>   perf tools: Use for_each_set_bit() to iterate over feature flags

I just realized conflicts with latest tip/perf/core. This applies on
c23205c8488f11cb9ebe7a7b5851a1d8a0171011.

I can resent a rebased version but would like to wait for review
first.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 00/10] perf tools: cleanups, fixes, updates

[Arnaldo Carvalho de Melo]

Em Tue, Dec 06, 2011 at 12:04:30PM +0100, Robert Richter escreveu:
> On 06.12.11 11:32:30, Robert Richter wrote:
> > The patch set contains some cleanups, fixes and updates I found worth
> > to implement during code review. Some patches can be applied
> > independently.
> > 
> > Patches #1 and #2 are a repost.
> > 
> > Robert Richter (10):
> >   perf script: Fix mem leaks and NULL pointer checks around strdup()s
> >   perf script: Implement option for system-wide profiling

The two above were merged already in:

https://github.com/acmel/linux/commits/perf/core

> >   perf tools: Continue processing header on unknown features
> >   perf tools: Fix out-of-bound access to struct perf_session
> >   perf tool: Moving code in some files
> >   perf report: Setup browser if stdout is a pipe
> >   perf report: Accept fifos as input file
> >   perf tool: Unify handling of features when writing feature section
> >   perf tools: Improve macros for struct feature_ops
> >   perf tools: Use for_each_set_bit() to iterate over feature flags
> 
> I just realized conflicts with latest tip/perf/core. This applies on
> c23205c8488f11cb9ebe7a7b5851a1d8a0171011.
> 
> I can resent a rebased version but would like to wait for review
> first.

Ok, going thru them now.

- Arnaldo