[PATCHSET 0/6] perf tools: A couple of TUI improvements

[PATCHSET 0/6] perf tools: A couple of TUI improvements

[Namhyung Kim]

Hello,

I was playing with TUI code and added two new windows.  One for
showing log messages and another for showing header information.
(Maybe they can be implemented on the GTK code too someday.)

Please try to use it! :)

I put the patches on 'perf/tui-v1' branch in my tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Any feedbacks are more than welcome, thanks
Namhyung


Namhyung Kim (6):
  perf report: Use pr_*() functions if possible
  perf tools: Introduce struct perf_log
  perf tools: Get rid of a duplicate va_end()
  perf tools: Save message when pr_*() was called
  perf ui/tui: Implement log window
  perf ui/tui: Implement header window

 tools/perf/Makefile.perf        |   3 ++
 tools/perf/builtin-report.c     |  24 +++++----
 tools/perf/perf.c               |   3 ++
 tools/perf/ui/browser.h         |   3 ++
 tools/perf/ui/browsers/header.c | 116 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/ui/browsers/hists.c  |   8 +++
 tools/perf/ui/browsers/log.c    |  92 +++++++++++++++++++++++++++++++
 tools/perf/util/debug.c         |   7 ++-
 tools/perf/util/debug.h         |  15 ++++++
 tools/perf/util/log.c           | 105 ++++++++++++++++++++++++++++++++++++
 10 files changed, 364 insertions(+), 12 deletions(-)
 create mode 100644 tools/perf/ui/browsers/header.c
 create mode 100644 tools/perf/ui/browsers/log.c
 create mode 100644 tools/perf/util/log.c

-- 
1.7.11.7

[PATCH 1/6] perf report: Use pr_*() functions if possible

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

There're some places printing a message to stdout/err directly.  It
should be converted to use proper error printing functions instead.

If it's not possible, just do it when --stdio was enabled only.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 3a14dbed387c..a1f72b471a96 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -321,8 +321,8 @@ static int process_sample_event(struct perf_tool *tool,
 	int ret;
 
 	if (perf_event__preprocess_sample(event, machine, &al, sample) < 0) {
-		fprintf(stderr, "problem processing %d event, skipping it.\n",
-			event->header.type);
+		pr_debug("problem processing %d event, skipping it.\n",
+			 event->header.type);
 		return -1;
 	}
 
@@ -552,15 +552,17 @@ static int __cmd_report(struct perf_report *rep)
 		desc);
 	}
 
-	if (verbose > 3)
-		perf_session__fprintf(session, stdout);
+	if (use_browser == 0) {
+		if (verbose > 3)
+			perf_session__fprintf(session, stdout);
 
-	if (verbose > 2)
-		perf_session__fprintf_dsos(session, stdout);
+		if (verbose > 2)
+			perf_session__fprintf_dsos(session, stdout);
 
-	if (dump_trace) {
-		perf_session__fprintf_nr_events(session, stdout);
-		return 0;
+		if (dump_trace) {
+			perf_session__fprintf_nr_events(session, stdout);
+			return 0;
+		}
 	}
 
 	nr_samples = 0;
@@ -720,7 +722,7 @@ parse_callchain_opt(const struct option *opt, const char *arg, int unset)
 		return -1;
 setup:
 	if (callchain_register_param(&callchain_param) < 0) {
-		fprintf(stderr, "Can't register callchain params\n");
+		pr_err("Can't register callchain params\n");
 		return -1;
 	}
 	return 0;
@@ -942,7 +944,7 @@ repeat:
 	}
 	if (report.mem_mode) {
 		if (sort__mode == SORT_MODE__BRANCH) {
-			fprintf(stderr, "branch and mem mode incompatible\n");
+			pr_err("branch and mem mode incompatible\n");
 			goto error;
 		}
 		sort__mode = SORT_MODE__MEMORY;
-- 
1.7.11.7

[PATCH 2/6] perf tools: Introduce struct perf_log

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Add new functions to save error messages in a temp file.  It'll be
used by some UI front-ends to see the messages.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Makefile.perf |   1 +
 tools/perf/perf.c        |   3 ++
 tools/perf/util/debug.h  |  15 +++++++
 tools/perf/util/log.c    | 105 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 124 insertions(+)
 create mode 100644 tools/perf/util/log.c

diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 97a2145e4cc6..ec8184014a76 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -372,6 +372,7 @@ LIB_OBJS += $(OUTPUT)util/stat.o
 LIB_OBJS += $(OUTPUT)util/record.o
 LIB_OBJS += $(OUTPUT)util/srcline.o
 LIB_OBJS += $(OUTPUT)util/data.o
+LIB_OBJS += $(OUTPUT)util/log.o
 
 LIB_OBJS += $(OUTPUT)ui/setup.o
 LIB_OBJS += $(OUTPUT)ui/helpline.o
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 431798a4110d..bdf7bd8e4394 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -10,6 +10,7 @@
 
 #include "util/exec_cmd.h"
 #include "util/cache.h"
+#include "util/debug.h"
 #include "util/quote.h"
 #include "util/run-command.h"
 #include "util/parse-events.h"
@@ -524,6 +525,7 @@ int main(int argc, const char **argv)
 	 */
 	pthread__block_sigwinch();
 
+	perf_log_init();
 	while (1) {
 		static int done_help;
 		int was_alias = run_argv(&argc, &argv);
@@ -543,6 +545,7 @@ int main(int argc, const char **argv)
 		} else
 			break;
 	}
+	perf_log_exit();
 
 	fprintf(stderr, "Failed to run command '%s': %s\n",
 		cmd, strerror(errno));
diff --git a/tools/perf/util/debug.h b/tools/perf/util/debug.h
index 443694c36b03..ea160abc2ae0 100644
--- a/tools/perf/util/debug.h
+++ b/tools/perf/util/debug.h
@@ -19,4 +19,19 @@ int ui__warning(const char *format, ...) __attribute__((format(printf, 1, 2)));
 
 void pr_stat(const char *fmt, ...);
 
+struct perf_log {
+	FILE *fp;
+	off_t *linemap;
+	u32 lines;
+	u32 nr_alloc;
+	bool seen_newline;
+};
+
+extern struct perf_log perf_log;
+
+int perf_log_init(void);
+int perf_log_exit(void);
+void perf_log_add(const char *msg);
+void perf_log_addv(const char *fmt, va_list ap);
+
 #endif	/* __PERF_DEBUG_H */
diff --git a/tools/perf/util/log.c b/tools/perf/util/log.c
new file mode 100644
index 000000000000..4a34bed2af44
--- /dev/null
+++ b/tools/perf/util/log.c
@@ -0,0 +1,105 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include "util/debug.h"
+
+#define LINEMAP_GROW  128
+
+struct perf_log perf_log = {
+	.seen_newline = true,
+};
+
+int perf_log_init(void)
+{
+	FILE *fp;
+	char name[] = "/tmp/perf-tui-log-XXXXXX";
+	int fd = mkstemp(name);
+
+	if (fd < 0)
+		return -1;
+
+	fp = fdopen(fd, "r+");
+	if (fp == NULL) {
+		close(fd);
+		return -1;
+	}
+
+	perf_log.fp = fp;
+
+	return 0;
+}
+
+int perf_log_exit(void)
+{
+	FILE *fp = perf_log.fp;
+	if (fp)
+		fclose(fp);
+
+	free(perf_log.linemap);
+
+	perf_log.fp = NULL;
+	perf_log.linemap = NULL;
+	return 0;
+}
+
+static int grow_linemap(struct perf_log *log)
+{
+	off_t *newmap;
+	int newsize = log->nr_alloc + LINEMAP_GROW;
+
+	newmap = realloc(log->linemap, newsize * sizeof(*log->linemap));
+	if (newmap == NULL)
+		return -1;
+
+	log->nr_alloc = newsize;
+	log->linemap = newmap;
+	return 0;
+}
+
+static int __add_to_linemap(struct perf_log *log, off_t index)
+{
+	if (log->lines == log->nr_alloc)
+		if (grow_linemap(log) < 0)
+			return -1;
+
+	log->linemap[log->lines++] = index;
+	return 0;
+}
+
+static void add_to_linemap(struct perf_log *log, const char *msg, off_t base)
+{
+	const char *pos;
+
+	if (strlen(msg) == 0)
+		return;
+
+	if (log->seen_newline) {
+		if (__add_to_linemap(log, base) < 0)
+			return;
+	}
+
+	if ((pos = strchr(msg, '\n')) != NULL) {
+		log->seen_newline = true;
+		pos++;
+		add_to_linemap(log, pos, base + (pos - msg));
+	} else {
+		log->seen_newline = false;
+	}
+}
+
+void perf_log_add(const char *msg)
+{
+	FILE *fp = perf_log.fp;
+	off_t offset = ftello(fp);
+
+	add_to_linemap(&perf_log, msg, offset);
+
+	fwrite(msg, 1, strlen(msg), fp);
+}
+
+void perf_log_addv(const char *fmt, va_list ap)
+{
+	char buf[4096];
+
+	vsnprintf(buf, sizeof(buf), fmt, ap);
+	perf_log_add(buf);
+}
-- 
1.7.11.7

[PATCH 3/6] perf tools: Get rid of a duplicate va_end()

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

The va_end() in _eprintf() should be removed since the caller also
invokes va_end().

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/debug.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
index 8640a9121e72..299b55586502 100644
--- a/tools/perf/util/debug.c
+++ b/tools/perf/util/debug.c
@@ -25,7 +25,6 @@ static int _eprintf(int level, const char *fmt, va_list args)
 			ui_helpline__vshow(fmt, args);
 		else
 			ret = vfprintf(stderr, fmt, args);
-		va_end(args);
 	}
 
 	return ret;
-- 
1.7.11.7

[PATCH 4/6] perf tools: Save message when pr_*() was called

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

The message will be saved in a temp file so that it can be shown at a
UI dialog at any time.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/debug.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
index 299b55586502..f55b499c93fb 100644
--- a/tools/perf/util/debug.c
+++ b/tools/perf/util/debug.c
@@ -19,12 +19,18 @@ bool dump_trace = false, quiet = false;
 static int _eprintf(int level, const char *fmt, va_list args)
 {
 	int ret = 0;
+	va_list ap;
 
 	if (verbose >= level) {
+		va_copy(ap, args);
+
 		if (use_browser >= 1)
 			ui_helpline__vshow(fmt, args);
 		else
 			ret = vfprintf(stderr, fmt, args);
+
+		perf_log_addv(fmt, ap);
+		va_end(ap);
 	}
 
 	return ret;
-- 
1.7.11.7

[PATCH 5/6] perf ui/tui: Implement log window

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Implement a simple, full-screen log window which shows error messages
saved so far.  Press 'l' (lower-case 'L') key to display the log
window.  It'll be used usually with -v option.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Makefile.perf       |  1 +
 tools/perf/ui/browser.h        |  1 +
 tools/perf/ui/browsers/hists.c |  4 ++
 tools/perf/ui/browsers/log.c   | 92 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 98 insertions(+)
 create mode 100644 tools/perf/ui/browsers/log.c

diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index ec8184014a76..30aabace33a0 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -490,6 +490,7 @@ ifndef NO_SLANG
   LIB_OBJS += $(OUTPUT)ui/browsers/hists.o
   LIB_OBJS += $(OUTPUT)ui/browsers/map.o
   LIB_OBJS += $(OUTPUT)ui/browsers/scripts.o
+  LIB_OBJS += $(OUTPUT)ui/browsers/log.o
   LIB_OBJS += $(OUTPUT)ui/tui/setup.o
   LIB_OBJS += $(OUTPUT)ui/tui/util.o
   LIB_OBJS += $(OUTPUT)ui/tui/helpline.o
diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
index 7d45d2f53601..fff46942a8c7 100644
--- a/tools/perf/ui/browser.h
+++ b/tools/perf/ui/browser.h
@@ -59,6 +59,7 @@ int ui_browser__help_window(struct ui_browser *browser, const char *text);
 bool ui_browser__dialog_yesno(struct ui_browser *browser, const char *text);
 int ui_browser__input_window(const char *title, const char *text, char *input,
 			     const char *exit_msg, int delay_sec);
+int tui__log_window(void);
 
 void ui_browser__argv_seek(struct ui_browser *browser, off_t offset, int whence);
 unsigned int ui_browser__argv_refresh(struct ui_browser *browser);
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index a440e03cd8c2..8cc6f5eaa2cf 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1484,6 +1484,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			if (is_report_browser(hbt))
 				goto do_data_switch;
 			continue;
+		case 'l':
+			tui__log_window();
+			continue;
 		case K_F1:
 		case 'h':
 		case '?':
@@ -1506,6 +1509,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 					"s             Switch to another data file in PWD ('perf report' only)\n"
 					"P             Print histograms to perf.hist.N\n"
 					"V             Verbose (DSO names in callchains, etc)\n"
+					"l             Show log messages\n"
 					"/             Filter symbol by name");
 			continue;
 		case K_ENTER:
diff --git a/tools/perf/ui/browsers/log.c b/tools/perf/ui/browsers/log.c
new file mode 100644
index 000000000000..f1610d6bb9d2
--- /dev/null
+++ b/tools/perf/ui/browsers/log.c
@@ -0,0 +1,92 @@
+#include <stdio.h>
+
+#include "perf.h"
+#include "util/util.h"
+#include "util/cache.h"
+#include "util/debug.h"
+#include "ui/ui.h"
+#include "ui/util.h"
+#include "ui/browser.h"
+#include "ui/libslang.h"
+#include "ui/keysyms.h"
+#include "ui/tui/tui.h"
+
+static void ui_browser__file_seek(struct ui_browser *browser __maybe_unused,
+				  off_t offset __maybe_unused,
+				  int whence __maybe_unused)
+{
+	/* do nothing */
+}
+
+static void ui_browser__file_write(struct ui_browser *browser,
+				   void *entry, int row)
+{
+	char buf[1024];
+	FILE *fp = perf_log.fp;
+	bool current_entry = ui_browser__is_current_entry(browser, row);
+	off_t *linemap = perf_log.linemap;
+	unsigned int idx = *(unsigned int *)entry;
+
+	fseek(fp, linemap[idx], SEEK_SET);
+	if (fgets(buf, sizeof(buf), fp) == NULL)
+		return;
+
+	ui_browser__set_color(browser, current_entry ? HE_COLORSET_SELECTED :
+						       HE_COLORSET_NORMAL);
+	slsmg_write_nstring(buf, browser->width);
+}
+
+static unsigned int ui_browser__file_refresh(struct ui_browser *browser)
+{
+	unsigned int row = 0;
+	unsigned int idx = browser->top_idx;
+
+	while (idx < browser->nr_entries) {
+		ui_browser__gotorc(browser, row, 0);
+		browser->write(browser, &idx, row);
+		if (++row == browser->height)
+			break;
+
+		++idx;
+	}
+
+	return row;
+}
+
+static int log_menu__run(struct ui_browser *menu)
+{
+	int key;
+
+	if (ui_browser__show(menu, "Log messages", "Press 'q' to exit") < 0)
+		return -1;
+
+	while (1) {
+		key = ui_browser__run(menu, 0);
+		switch (key) {
+		case K_ESC:
+		case 'q':
+		case CTRL('c'):
+			key = -1;
+			break;
+		default:
+			continue;
+		}
+
+		break;
+	}
+
+	ui_browser__hide(menu);
+	return key;
+}
+
+int tui__log_window(void)
+{
+	struct ui_browser log_menu = {
+		.refresh    = ui_browser__file_refresh,
+		.seek	    = ui_browser__file_seek,
+		.write	    = ui_browser__file_write,
+		.nr_entries = perf_log.lines,
+	};
+
+	return log_menu__run(&log_menu);
+}
-- 
1.7.11.7

[PATCH 6/6] perf ui/tui: Implement header window

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Implement a simple, full-screen header window which shows session
header (metadata) information.  Press 'i' key to display the header
window.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Makefile.perf        |   1 +
 tools/perf/ui/browser.h         |   2 +
 tools/perf/ui/browsers/header.c | 116 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/ui/browsers/hists.c  |   4 ++
 4 files changed, 123 insertions(+)
 create mode 100644 tools/perf/ui/browsers/header.c

diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 30aabace33a0..4d046e97d0e7 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -491,6 +491,7 @@ ifndef NO_SLANG
   LIB_OBJS += $(OUTPUT)ui/browsers/map.o
   LIB_OBJS += $(OUTPUT)ui/browsers/scripts.o
   LIB_OBJS += $(OUTPUT)ui/browsers/log.o
+  LIB_OBJS += $(OUTPUT)ui/browsers/header.o
   LIB_OBJS += $(OUTPUT)ui/tui/setup.o
   LIB_OBJS += $(OUTPUT)ui/tui/util.o
   LIB_OBJS += $(OUTPUT)ui/tui/helpline.o
diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
index fff46942a8c7..b77d8728b3f0 100644
--- a/tools/perf/ui/browser.h
+++ b/tools/perf/ui/browser.h
@@ -60,6 +60,8 @@ bool ui_browser__dialog_yesno(struct ui_browser *browser, const char *text);
 int ui_browser__input_window(const char *title, const char *text, char *input,
 			     const char *exit_msg, int delay_sec);
 int tui__log_window(void);
+struct perf_session_env;
+int tui__header_window(struct perf_session_env *env);
 
 void ui_browser__argv_seek(struct ui_browser *browser, off_t offset, int whence);
 unsigned int ui_browser__argv_refresh(struct ui_browser *browser);
diff --git a/tools/perf/ui/browsers/header.c b/tools/perf/ui/browsers/header.c
new file mode 100644
index 000000000000..b5f0961bf793
--- /dev/null
+++ b/tools/perf/ui/browsers/header.c
@@ -0,0 +1,116 @@
+#include "util/cache.h"
+#include "util/debug.h"
+#include "ui/browser.h"
+#include "ui/ui.h"
+#include "ui/util.h"
+#include "ui/libslang.h"
+#include "util/header.h"
+#include "util/session.h"
+
+static void ui_browser__argv_write(struct ui_browser *browser,
+				   void *entry, int row)
+{
+	char **arg = entry;
+	char *str = *arg;
+	char empty[] = " ";
+	bool current_entry = ui_browser__is_current_entry(browser, row);
+	unsigned long offset = (unsigned long)browser->priv;
+
+	if (offset >= strlen(str))
+		str = empty;
+	else
+		str = str + offset;
+
+	ui_browser__set_color(browser, current_entry ? HE_COLORSET_SELECTED :
+						       HE_COLORSET_NORMAL);
+
+	slsmg_write_nstring(str, browser->width);
+}
+
+static int list_menu__run(struct ui_browser *menu)
+{
+	int key;
+	unsigned long offset;
+
+	if (ui_browser__show(menu, "Header information", "Press 'q' to exit") < 0)
+		return -1;
+
+	while (1) {
+		key = ui_browser__run(menu, 0);
+
+		switch (key) {
+		case K_RIGHT:
+			offset = (unsigned long)menu->priv;
+			offset += 10;
+			menu->priv = (void *)offset;
+			continue;
+		case K_LEFT:
+			offset = (unsigned long)menu->priv;
+			if (offset >= 10)
+				offset -= 10;
+			menu->priv = (void *)offset;
+			continue;
+		case K_ESC:
+		case 'q':
+		case CTRL('c'):
+			key = -1;
+			break;
+		default:
+			continue;
+		}
+
+		break;
+	}
+
+	ui_browser__hide(menu);
+	return key;
+}
+
+static int ui__list_menu(int argc, char * const argv[])
+{
+	struct ui_browser menu = {
+		.entries    = (void *)argv,
+		.refresh    = ui_browser__argv_refresh,
+		.seek	    = ui_browser__argv_seek,
+		.write	    = ui_browser__argv_write,
+		.nr_entries = argc,
+	};
+
+	return list_menu__run(&menu);
+}
+
+int tui__header_window(struct perf_session_env *env)
+{
+	int i, argc = 0;
+	char **argv;
+	struct perf_session *session;
+	char *ptr, *pos;
+	size_t size;
+	FILE *fp = open_memstream(&ptr, &size);
+
+	session = container_of(env, struct perf_session, header.env);
+	perf_header__fprintf_info(session, fp, true);
+	fclose(fp);
+
+	for (pos = ptr, argc = 0; (pos = strchr(pos, '\n')) != NULL; pos++)
+		argc++;
+
+	argv = calloc(argc + 1, sizeof(*argv));
+	if (argv == NULL)
+		goto out;
+
+	argv[0] = pos = ptr;
+	for (i = 1; (pos = strchr(pos, '\n')) != NULL; i++) {
+		*pos++ = '\0';
+		argv[i] = pos;
+	}
+
+	BUG_ON(i != argc + 1);
+
+	ui__list_menu(argc, argv);
+
+out:
+	free(argv);
+	free(ptr);
+	return 0;
+}
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 8cc6f5eaa2cf..d710c6403a14 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1487,6 +1487,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		case 'l':
 			tui__log_window();
 			continue;
+		case 'i':
+			tui__header_window(env);
+			continue;
 		case K_F1:
 		case 'h':
 		case '?':
@@ -1510,6 +1513,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 					"P             Print histograms to perf.hist.N\n"
 					"V             Verbose (DSO names in callchains, etc)\n"
 					"l             Show log messages\n"
+					"i             Show header information\n"
 					"/             Filter symbol by name");
 			continue;
 		case K_ENTER:
-- 
1.7.11.7

Re: [PATCHSET 0/6] perf tools: A couple of TUI improvements

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> Hello,
> 
> I was playing with TUI code and added two new windows.  One for
> showing log messages and another for showing header information.
> (Maybe they can be implemented on the GTK code too someday.)
> 
> Please try to use it! :)
> 
> I put the patches on 'perf/tui-v1' branch in my tree:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 
> Any feedbacks are more than welcome, thanks
> Namhyung

In the morning haze it took me some time to figure out that typing 'l' 
gives leads to the log window, 'i' to the header information.

'l' seems to work (no log messages though :-), but in perf-top 'i' 
segfaulted:

  comet:~/tip/tools/perf> perf top
  perf: Segmentation fault

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff0d39700 (LWP 14785)]
perf_header__fprintf_info (session=session@entry=0x957bf0, fp=fp@entry=0x7fffec000a30, full=full@entry=true) at util/header.c:2195
2195            int fd = perf_data_file__fd(session->file);
Missing separate debuginfos, use: debuginfo-install audit-libs-2.3.2-1.fc19.x86_64 bzip2-libs-1.0.6-8.fc19.x86_64 elfutils-libelf-0.156-5.fc19.x86_64 elfutils-libs-0.156-5.fc19.x86_64 glibc-2.17-19.fc19.x86_64 libgcc-4.8.2-1.fc19.x86_64 libunwind-1.1-2.fc19.x86_64 nss-softokn-freebl-3.15.2-2.fc19.x86_64 numactl-libs-2.0.8-4.fc19.x86_64 perl-libs-5.16.3-266.fc19.x86_64 python-libs-2.7.5-9.fc19.x86_64 slang-2.2.4-8.fc19.x86_64 xz-libs-5.1.2-4alpha.fc19.x86_64 zlib-1.2.7-10.fc19.x86_64
(gdb) bt
#0  perf_header__fprintf_info (session=session@entry=0x957bf0, fp=fp@entry=0x7fffec000a30, full=full@entry=true) at util/header.c:2195
#1  0x00000000004ce3fe in tui__header_window (env=env@entry=0x957c30) at ui/browsers/header.c:92
#2  0x00000000004cbdfd in perf_evsel__hists_browse (evsel=evsel@entry=0x957560, nr_events=nr_events@entry=1, 
    helpline=helpline@entry=0x57db88 "For a higher level overview, try: perf top --sort comm,dso", ev_name=0x957870 "cycles", left_exits=left_exits@entry=false, 
    hbt=hbt@entry=0x7ffff0d38ea0, min_pcnt=min_pcnt@entry=0, env=env@entry=0x957c30) at ui/browsers/hists.c:1491
#3  0x00000000004cd5a5 in perf_evlist__tui_browse_hists (evlist=0x8e5930, help=help@entry=0x57db88 "For a higher level overview, try: perf top --sort comm,dso", 
    hbt=hbt@entry=0x7ffff0d38ea0, min_pcnt=0, env=0x957c30) at ui/browsers/hists.c:1957
#4  0x0000000000433b48 in display_thread_tui (arg=0x7fffffffb150) at builtin-top.c:584
#5  0x0000003e59407c53 in start_thread () from /lib64/libpthread.so.0
#6  0x0000003e590f5dbd in clone () from /lib64/libc.so.6
(gdb) 

I suspect it got surprised by perf top not having a header per se? 
Still it might make sense to also robustify 
perf_header__fprintf_info() against segfaulting and such.

But it's a nice feature nevertheless!

Btw., it would be nice if 'P' worked on these screens, so any 
interesting data can be extracted! Cut & paste is usually a PITA due 
to the graphical TUI characters.

Btw., in case you are taking TUI usability bugreports, here's a few I 
noticed while playing with your changes:

1)

In histogram view it would be nice if 'P' gave some status bar 
indication that it just wrote to perf.hist.0 or so - otherwise the 
user is kept in the dark.

2)

Likewise, in a TUI every keypress must produce some tangible feedback 
to the user. Try hitting 'o' for example - it should probably output 
into the status bar that 'o' is not a bound keypress or so.

3)

Same goes for page up / page down in histogram view if we are at the 
end of the list: some low-key, single-character feedback should be 
given that the keypress was seen but we are at the end of the list. 
For exampe the scrollbar 'diamond' character could briefly 
inverse-flash or so.

Thanks,

	Ingo

Re: [PATCH 2/6] perf tools: Introduce struct perf_log

[Arnaldo Carvalho de Melo]

Em Thu, Dec 19, 2013 at 04:00:07PM +0900, Namhyung Kim escreveu:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Add new functions to save error messages in a temp file.  It'll be
> used by some UI front-ends to see the messages.

Do we really have to grow a logging facility like this? Can't we somehow
openlog/syslog/closelog? Just a first gut reaction, continuing to read
the series...

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/Makefile.perf |   1 +
>  tools/perf/perf.c        |   3 ++
>  tools/perf/util/debug.h  |  15 +++++++
>  tools/perf/util/log.c    | 105 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 124 insertions(+)
>  create mode 100644 tools/perf/util/log.c
> 
> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> index 97a2145e4cc6..ec8184014a76 100644
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
> @@ -372,6 +372,7 @@ LIB_OBJS += $(OUTPUT)util/stat.o
>  LIB_OBJS += $(OUTPUT)util/record.o
>  LIB_OBJS += $(OUTPUT)util/srcline.o
>  LIB_OBJS += $(OUTPUT)util/data.o
> +LIB_OBJS += $(OUTPUT)util/log.o
>  
>  LIB_OBJS += $(OUTPUT)ui/setup.o
>  LIB_OBJS += $(OUTPUT)ui/helpline.o
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index 431798a4110d..bdf7bd8e4394 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -10,6 +10,7 @@
>  
>  #include "util/exec_cmd.h"
>  #include "util/cache.h"
> +#include "util/debug.h"
>  #include "util/quote.h"
>  #include "util/run-command.h"
>  #include "util/parse-events.h"
> @@ -524,6 +525,7 @@ int main(int argc, const char **argv)
>  	 */
>  	pthread__block_sigwinch();
>  
> +	perf_log_init();
>  	while (1) {
>  		static int done_help;
>  		int was_alias = run_argv(&argc, &argv);
> @@ -543,6 +545,7 @@ int main(int argc, const char **argv)
>  		} else
>  			break;
>  	}
> +	perf_log_exit();
>  
>  	fprintf(stderr, "Failed to run command '%s': %s\n",
>  		cmd, strerror(errno));
> diff --git a/tools/perf/util/debug.h b/tools/perf/util/debug.h
> index 443694c36b03..ea160abc2ae0 100644
> --- a/tools/perf/util/debug.h
> +++ b/tools/perf/util/debug.h
> @@ -19,4 +19,19 @@ int ui__warning(const char *format, ...) __attribute__((format(printf, 1, 2)));
>  
>  void pr_stat(const char *fmt, ...);
>  
> +struct perf_log {
> +	FILE *fp;
> +	off_t *linemap;
> +	u32 lines;
> +	u32 nr_alloc;
> +	bool seen_newline;
> +};
> +
> +extern struct perf_log perf_log;
> +
> +int perf_log_init(void);
> +int perf_log_exit(void);
> +void perf_log_add(const char *msg);
> +void perf_log_addv(const char *fmt, va_list ap);
> +
>  #endif	/* __PERF_DEBUG_H */
> diff --git a/tools/perf/util/log.c b/tools/perf/util/log.c
> new file mode 100644
> index 000000000000..4a34bed2af44
> --- /dev/null
> +++ b/tools/perf/util/log.c
> @@ -0,0 +1,105 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include "util/debug.h"
> +
> +#define LINEMAP_GROW  128
> +
> +struct perf_log perf_log = {
> +	.seen_newline = true,
> +};
> +
> +int perf_log_init(void)
> +{
> +	FILE *fp;
> +	char name[] = "/tmp/perf-tui-log-XXXXXX";
> +	int fd = mkstemp(name);
> +
> +	if (fd < 0)
> +		return -1;
> +
> +	fp = fdopen(fd, "r+");
> +	if (fp == NULL) {
> +		close(fd);
> +		return -1;
> +	}
> +
> +	perf_log.fp = fp;
> +
> +	return 0;
> +}
> +
> +int perf_log_exit(void)
> +{
> +	FILE *fp = perf_log.fp;
> +	if (fp)
> +		fclose(fp);
> +
> +	free(perf_log.linemap);
> +
> +	perf_log.fp = NULL;
> +	perf_log.linemap = NULL;
> +	return 0;
> +}
> +
> +static int grow_linemap(struct perf_log *log)
> +{
> +	off_t *newmap;
> +	int newsize = log->nr_alloc + LINEMAP_GROW;
> +
> +	newmap = realloc(log->linemap, newsize * sizeof(*log->linemap));
> +	if (newmap == NULL)
> +		return -1;
> +
> +	log->nr_alloc = newsize;
> +	log->linemap = newmap;
> +	return 0;
> +}
> +
> +static int __add_to_linemap(struct perf_log *log, off_t index)
> +{
> +	if (log->lines == log->nr_alloc)
> +		if (grow_linemap(log) < 0)
> +			return -1;
> +
> +	log->linemap[log->lines++] = index;
> +	return 0;
> +}
> +
> +static void add_to_linemap(struct perf_log *log, const char *msg, off_t base)
> +{
> +	const char *pos;
> +
> +	if (strlen(msg) == 0)
> +		return;
> +
> +	if (log->seen_newline) {
> +		if (__add_to_linemap(log, base) < 0)
> +			return;
> +	}
> +
> +	if ((pos = strchr(msg, '\n')) != NULL) {
> +		log->seen_newline = true;
> +		pos++;
> +		add_to_linemap(log, pos, base + (pos - msg));
> +	} else {
> +		log->seen_newline = false;
> +	}
> +}
> +
> +void perf_log_add(const char *msg)
> +{
> +	FILE *fp = perf_log.fp;
> +	off_t offset = ftello(fp);
> +
> +	add_to_linemap(&perf_log, msg, offset);
> +
> +	fwrite(msg, 1, strlen(msg), fp);
> +}
> +
> +void perf_log_addv(const char *fmt, va_list ap)
> +{
> +	char buf[4096];
> +
> +	vsnprintf(buf, sizeof(buf), fmt, ap);
> +	perf_log_add(buf);
> +}
> -- 
> 1.7.11.7

Re: [PATCH 1/6] perf report: Use pr_*() functions if possible

[Arnaldo Carvalho de Melo]

Em Thu, Dec 19, 2013 at 04:00:06PM +0900, Namhyung Kim escreveu:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> There're some places printing a message to stdout/err directly.  It
> should be converted to use proper error printing functions instead.
> 
> If it's not possible, just do it when --stdio was enabled only.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-report.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 3a14dbed387c..a1f72b471a96 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -321,8 +321,8 @@ static int process_sample_event(struct perf_tool *tool,
>  	int ret;
>  
>  	if (perf_event__preprocess_sample(event, machine, &al, sample) < 0) {
> -		fprintf(stderr, "problem processing %d event, skipping it.\n",
> -			event->header.type);
> +		pr_debug("problem processing %d event, skipping it.\n",
> +			 event->header.type);
>  		return -1;
>  	}
>  
> @@ -552,15 +552,17 @@ static int __cmd_report(struct perf_report *rep)
>  		desc);
>  	}
>  
> -	if (verbose > 3)
> -		perf_session__fprintf(session, stdout);
> +	if (use_browser == 0) {
> +		if (verbose > 3)
> +			perf_session__fprintf(session, stdout);
>  
> -	if (verbose > 2)
> -		perf_session__fprintf_dsos(session, stdout);
> +		if (verbose > 2)
> +			perf_session__fprintf_dsos(session, stdout);
>  
> -	if (dump_trace) {
> -		perf_session__fprintf_nr_events(session, stdout);
> -		return 0;
> +		if (dump_trace) {
> +			perf_session__fprintf_nr_events(session, stdout);
> +			return 0;
> +		}
>  	}

This should be in a different patch, one that is related just to
dump_trace, i.e. if -D, aka dump_trace (right?) is enabled, then
probably we won't be initializing the TUI/GUI, no?

So having a patch that deals just with conversions of fprintf ->
pr_{err,etc} and another that deals with dump_trace related seems to be
the best course of action.

- Arnaldo
  
>  	nr_samples = 0;
> @@ -720,7 +722,7 @@ parse_callchain_opt(const struct option *opt, const char *arg, int unset)
>  		return -1;
>  setup:
>  	if (callchain_register_param(&callchain_param) < 0) {
> -		fprintf(stderr, "Can't register callchain params\n");
> +		pr_err("Can't register callchain params\n");
>  		return -1;
>  	}
>  	return 0;
> @@ -942,7 +944,7 @@ repeat:
>  	}
>  	if (report.mem_mode) {
>  		if (sort__mode == SORT_MODE__BRANCH) {
> -			fprintf(stderr, "branch and mem mode incompatible\n");
> +			pr_err("branch and mem mode incompatible\n");
>  			goto error;
>  		}
>  		sort__mode = SORT_MODE__MEMORY;
> -- 
> 1.7.11.7

Re: [PATCH 1/6] perf report: Use pr_*() functions if possible

[Jiri Olsa]

On Thu, Dec 19, 2013 at 04:00:06PM +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> There're some places printing a message to stdout/err directly.  It
> should be converted to use proper error printing functions instead.
> 
> If it's not possible, just do it when --stdio was enabled only.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Maybe it'd be worthwhile to factor perf_session__fprintf*
functions so the info could end up in the log window.

Acked-by: Jiri Olsa <jolsa@redhat.com>

jirka

Re: [PATCH 3/6] perf tools: Get rid of a duplicate va_end()

[Jiri Olsa]

On Thu, Dec 19, 2013 at 04:00:08PM +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> The va_end() in _eprintf() should be removed since the caller also
> invokes va_end().
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Jiri Olsa <jolsa@redhat.com>

jirka

> ---
>  tools/perf/util/debug.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
> index 8640a9121e72..299b55586502 100644
> --- a/tools/perf/util/debug.c
> +++ b/tools/perf/util/debug.c
> @@ -25,7 +25,6 @@ static int _eprintf(int level, const char *fmt, va_list args)
>  			ui_helpline__vshow(fmt, args);
>  		else
>  			ret = vfprintf(stderr, fmt, args);
> -		va_end(args);
>  	}
>  
>  	return ret;
> -- 
> 1.7.11.7
> 

Re: [PATCH 3/6] perf tools: Get rid of a duplicate va_end()

[Arnaldo Carvalho de Melo]

Em Thu, Dec 19, 2013 at 04:00:08PM +0900, Namhyung Kim escreveu:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> The va_end() in _eprintf() should be removed since the caller also
> invokes va_end().

Thanks, applied.
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/debug.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
> index 8640a9121e72..299b55586502 100644
> --- a/tools/perf/util/debug.c
> +++ b/tools/perf/util/debug.c
> @@ -25,7 +25,6 @@ static int _eprintf(int level, const char *fmt, va_list args)
>  			ui_helpline__vshow(fmt, args);
>  		else
>  			ret = vfprintf(stderr, fmt, args);
> -		va_end(args);
>  	}
>  
>  	return ret;
> -- 
> 1.7.11.7

Re: [PATCHSET 0/6] perf tools: A couple of TUI improvements

[Jiri Olsa]

On Thu, Dec 19, 2013 at 04:00:05PM +0900, Namhyung Kim wrote:
> Hello,
> 
> I was playing with TUI code and added two new windows.  One for
> showing log messages and another for showing header information.
> (Maybe they can be implemented on the GTK code too someday.)
> 
> Please try to use it! :)

hi,
I'm sure the 'i' header display will be appreciated!

Any chance of searching capability in the 'l' log window?
mutt-like limit('l') command would be great ;-)

thanks,
jirka

Re: [PATCHSET 0/6] perf tools: A couple of TUI improvements

[Ingo Molnar]

* Jiri Olsa <jolsa@redhat.com> wrote:

> On Thu, Dec 19, 2013 at 04:00:05PM +0900, Namhyung Kim wrote:
> > Hello,
> > 
> > I was playing with TUI code and added two new windows.  One for
> > showing log messages and another for showing header information.
> > (Maybe they can be implemented on the GTK code too someday.)
> > 
> > Please try to use it! :)
> 
> hi,
> I'm sure the 'i' header display will be appreciated!
> 
> Any chance of searching capability in the 'l' log window?
> mutt-like limit('l') command would be great ;-)

+1 :-)

	Ingo

Re: [PATCHSET 0/6] perf tools: A couple of TUI improvements

[Ingo Molnar]

* Jiri Olsa <jolsa@redhat.com> wrote:

> On Thu, Dec 19, 2013 at 04:00:05PM +0900, Namhyung Kim wrote:
> > Hello,
> > 
> > I was playing with TUI code and added two new windows.  One for
> > showing log messages and another for showing header information.
> > (Maybe they can be implemented on the GTK code too someday.)
> > 
> > Please try to use it! :)
> 
> hi,
> I'm sure the 'i' header display will be appreciated!
> 
> Any chance of searching capability in the 'l' log window?
> mutt-like limit('l') command would be great ;-)

A bit like how '/' works in histogram view, it will filter on partial 
matches as well, making it really easy to filter down to larger 
classes of functions (in projects that have good function name 
namespaces).

Thanks,

	Ingo

Re: [PATCHSET 0/6] perf tools: A couple of TUI improvements

[Namhyung Kim]

Hi Ingo,

On Thu, 19 Dec 2013 13:14:35 +0100, Ingo Molnar wrote:
> * Namhyung Kim <namhyung@kernel.org> wrote:
>
>> Hello,
>> 
>> I was playing with TUI code and added two new windows.  One for
>> showing log messages and another for showing header information.
>> (Maybe they can be implemented on the GTK code too someday.)
>> 
>> Please try to use it! :)
>> 
>> I put the patches on 'perf/tui-v1' branch in my tree:
>> 
>>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>> 
>> Any feedbacks are more than welcome, thanks
>> Namhyung
>
> In the morning haze it took me some time to figure out that typing 'l' 
> gives leads to the log window, 'i' to the header information.
>
> 'l' seems to work (no log messages though :-), but in perf-top 'i' 
> segfaulted:
>
>   comet:~/tip/tools/perf> perf top
>   perf: Segmentation fault

Ah, forgot to test perf top - I just played with perf report at this
time. :-/

>
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7ffff0d39700 (LWP 14785)]
> perf_header__fprintf_info (session=session@entry=0x957bf0, fp=fp@entry=0x7fffec000a30, full=full@entry=true) at util/header.c:2195
> 2195            int fd = perf_data_file__fd(session->file);
> Missing separate debuginfos, use: debuginfo-install audit-libs-2.3.2-1.fc19.x86_64 bzip2-libs-1.0.6-8.fc19.x86_64 elfutils-libelf-0.156-5.fc19.x86_64 elfutils-libs-0.156-5.fc19.x86_64 glibc-2.17-19.fc19.x86_64 libgcc-4.8.2-1.fc19.x86_64 libunwind-1.1-2.fc19.x86_64 nss-softokn-freebl-3.15.2-2.fc19.x86_64 numactl-libs-2.0.8-4.fc19.x86_64 perl-libs-5.16.3-266.fc19.x86_64 python-libs-2.7.5-9.fc19.x86_64 slang-2.2.4-8.fc19.x86_64 xz-libs-5.1.2-4alpha.fc19.x86_64 zlib-1.2.7-10.fc19.x86_64
> (gdb) bt
> #0  perf_header__fprintf_info (session=session@entry=0x957bf0, fp=fp@entry=0x7fffec000a30, full=full@entry=true) at util/header.c:2195
> #1  0x00000000004ce3fe in tui__header_window (env=env@entry=0x957c30) at ui/browsers/header.c:92
> #2  0x00000000004cbdfd in perf_evsel__hists_browse (evsel=evsel@entry=0x957560, nr_events=nr_events@entry=1, 
>     helpline=helpline@entry=0x57db88 "For a higher level overview, try: perf top --sort comm,dso", ev_name=0x957870 "cycles", left_exits=left_exits@entry=false, 
>     hbt=hbt@entry=0x7ffff0d38ea0, min_pcnt=min_pcnt@entry=0, env=env@entry=0x957c30) at ui/browsers/hists.c:1491
> #3  0x00000000004cd5a5 in perf_evlist__tui_browse_hists (evlist=0x8e5930, help=help@entry=0x57db88 "For a higher level overview, try: perf top --sort comm,dso", 
>     hbt=hbt@entry=0x7ffff0d38ea0, min_pcnt=0, env=0x957c30) at ui/browsers/hists.c:1957
> #4  0x0000000000433b48 in display_thread_tui (arg=0x7fffffffb150) at builtin-top.c:584
> #5  0x0000003e59407c53 in start_thread () from /lib64/libpthread.so.0
> #6  0x0000003e590f5dbd in clone () from /lib64/libc.so.6
> (gdb) 
>
> I suspect it got surprised by perf top not having a header per se? 
> Still it might make sense to also robustify 
> perf_header__fprintf_info() against segfaulting and such.

Right.  To be precise, perf top do have header.env but not set the
actual info in it.  I had to check it, sorry.  The patch below will fix
the problem.


>From 451a267fb2243860f62390151cc156a9eab7d317 Mon Sep 17 00:00:00 2001
From: Namhyung Kim <namhyung.kim@lge.com>
Date: Fri, 20 Dec 2013 09:14:23 +0900
Subject: [PATCH 1/3] perf ui/tui: fixup for header window on perf-top

It should check whether header.env is valid.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index d710c6403a14..c25859cae32d 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1488,7 +1488,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			tui__log_window();
 			continue;
 		case 'i':
-			tui__header_window(env);
+			/* env->arch is NULL for live-mode (i.e. perf top) */
+			if (env->arch)
+				tui__header_window(env);
 			continue;
 		case K_F1:
 		case 'h':
-- 
1.7.11.7


I'll fold it to the original patch.

>
> But it's a nice feature nevertheless!

Thanks! :)

>
> Btw., it would be nice if 'P' worked on these screens, so any 
> interesting data can be extracted! Cut & paste is usually a PITA due 
> to the graphical TUI characters.

Ah, okay.  I think it should be easy, will cook a patch.

>
> Btw., in case you are taking TUI usability bugreports, here's a few I 
> noticed while playing with your changes:

Hmm.. I'll take a look at them next week too. :)

>
> 1)
>
> In histogram view it would be nice if 'P' gave some status bar 
> indication that it just wrote to perf.hist.0 or so - otherwise the 
> user is kept in the dark.
>
> 2)
>
> Likewise, in a TUI every keypress must produce some tangible feedback 
> to the user. Try hitting 'o' for example - it should probably output 
> into the status bar that 'o' is not a bound keypress or so.
>
> 3)
>
> Same goes for page up / page down in histogram view if we are at the 
> end of the list: some low-key, single-character feedback should be 
> given that the keypress was seen but we are at the end of the list. 
> For exampe the scrollbar 'diamond' character could briefly 
> inverse-flash or so.
>
> Thanks,
>
> 	Ingo

Re: [PATCHSET 0/6] perf tools: A couple of TUI improvements

[Namhyung Kim]

On Thu, 19 Dec 2013 16:53:16 +0100, Ingo Molnar wrote:
> * Jiri Olsa <jolsa@redhat.com> wrote:
>
>> On Thu, Dec 19, 2013 at 04:00:05PM +0900, Namhyung Kim wrote:
>> > Hello,
>> > 
>> > I was playing with TUI code and added two new windows.  One for
>> > showing log messages and another for showing header information.
>> > (Maybe they can be implemented on the GTK code too someday.)
>> > 
>> > Please try to use it! :)
>> 
>> hi,
>> I'm sure the 'i' header display will be appreciated!
>> 
>> Any chance of searching capability in the 'l' log window?
>> mutt-like limit('l') command would be great ;-)
>
> A bit like how '/' works in histogram view, it will filter on partial 
> matches as well, making it really easy to filter down to larger 
> classes of functions (in projects that have good function name 
> namespaces).

Yes, I was thinking about that too.  Will send a patch soon.

Thanks,
Namhyung

Re: [PATCH 2/6] perf tools: Introduce struct perf_log

[Namhyung Kim]

Hi Arnaldo,

On Thu, 19 Dec 2013 10:28:06 -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Dec 19, 2013 at 04:00:07PM +0900, Namhyung Kim escreveu:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> Add new functions to save error messages in a temp file.  It'll be
>> used by some UI front-ends to see the messages.
>
> Do we really have to grow a logging facility like this? Can't we somehow
> openlog/syslog/closelog? Just a first gut reaction, continuing to read
> the series...

I'm not sure.  It's not really a logging facility - I just wanted a way to
save log messages somewhere in order to show them in a TUI window.  My
feeling is that it doesn't need to grow since perf is not a service or so..

Thanks,
Namhyung

Re: [PATCH 1/6] perf report: Use pr_*() functions if possible

[Namhyung Kim]

On Thu, 19 Dec 2013 10:30:47 -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Dec 19, 2013 at 04:00:06PM +0900, Namhyung Kim escreveu:
>> -	if (verbose > 3)
>> -		perf_session__fprintf(session, stdout);
>> +	if (use_browser == 0) {
>> +		if (verbose > 3)
>> +			perf_session__fprintf(session, stdout);
>>  
>> -	if (verbose > 2)
>> -		perf_session__fprintf_dsos(session, stdout);
>> +		if (verbose > 2)
>> +			perf_session__fprintf_dsos(session, stdout);
>>  
>> -	if (dump_trace) {
>> -		perf_session__fprintf_nr_events(session, stdout);
>> -		return 0;
>> +		if (dump_trace) {
>> +			perf_session__fprintf_nr_events(session, stdout);
>> +			return 0;
>> +		}
>>  	}
>
> This should be in a different patch, one that is related just to
> dump_trace, i.e. if -D, aka dump_trace (right?) is enabled, then
> probably we won't be initializing the TUI/GUI, no?

Yes, the dump_trace will make use_browser = 0.  So I just moved it under
the if (use_browser == 0).

>
> So having a patch that deals just with conversions of fprintf ->
> pr_{err,etc} and another that deals with dump_trace related seems to be
> the best course of action.

Okay, I'll split this into two.

Thanks,
Namhyung

Re: [PATCH 1/6] perf report: Use pr_*() functions if possible

[Namhyung Kim]

Hi Jiri,

On Thu, 19 Dec 2013 14:31:54 +0100, Jiri Olsa wrote:
> On Thu, Dec 19, 2013 at 04:00:06PM +0900, Namhyung Kim wrote:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> There're some places printing a message to stdout/err directly.  It
>> should be converted to use proper error printing functions instead.
>> 
>> If it's not possible, just do it when --stdio was enabled only.
>> 
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Maybe it'd be worthwhile to factor perf_session__fprintf*
> functions so the info could end up in the log window.

I thought about it too.  And then considered something like below:

  FILE *sfp = open_memstream(&ptr, &size);
  perf_session__fprintf*(session, sfp, ...);
  fclose(sfp);

  perf_log_add(ptr);
  fprintf(orig_fp, "%s", ptr);

  free(ptr);


What do you think?

>
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks!
Namhyung

[tip:perf/core] perf tools: Get rid of a duplicate va_end() in error reporting routine

[tip-bot for Namhyung Kim]

Commit-ID:  73db8f82619b7538d9b4badfe13f3ab2fef7d9b3
Gitweb:     http://git.kernel.org/tip/73db8f82619b7538d9b4badfe13f3ab2fef7d9b3
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Thu, 19 Dec 2013 16:00:08 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 19 Dec 2013 11:38:42 -0300

perf tools: Get rid of a duplicate va_end() in error reporting routine

The va_end() in _eprintf() should be removed since the caller also
invokes va_end().

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1387436411-20160-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/debug.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
index 8640a91..299b555 100644
--- a/tools/perf/util/debug.c
+++ b/tools/perf/util/debug.c
@@ -25,7 +25,6 @@ static int _eprintf(int level, const char *fmt, va_list args)
 			ui_helpline__vshow(fmt, args);
 		else
 			ret = vfprintf(stderr, fmt, args);
-		va_end(args);
 	}
 
 	return ret;