[PATCH 0/7] perf tool: pipe-mode fixes

[PATCH 0/7] perf tool: pipe-mode fixes

[David Carrillo-Cisneros]

Various fixes for perf tool pipe-mode for bugs that arose while trying
to make this work:

    perf record -o - noploop | perf inject -b | perf annotate -i -

David Carrillo-Cisneros (7):
  perf inject: don't proceed if perf_session__process_event fails
  perf inject: copy events when reordering events in pipe mode
  perf tool: describe pipe mode in perf.data-file-fomat.txt
  perf annotate: process attr and build_id records
  perf session: don't rely on evlist in pipe mode
  perf tool: protect empty evlists
  perf tool: do not print missing features in pipe-mode

 tools/perf/Documentation/perf.data-file-format.txt | 19 +++++++++++++++++--
 tools/perf/builtin-annotate.c                      |  2 ++
 tools/perf/builtin-inject.c                        |  2 ++
 tools/perf/util/evlist.h                           |  4 ++++
 tools/perf/util/header.c                           | 13 +++++++------
 tools/perf/util/ordered-events.c                   |  3 ++-
 tools/perf/util/session.c                          | 17 ++++++++++++++---
 7 files changed, 48 insertions(+), 12 deletions(-)

-- 
2.12.2.715.g7642488e1d-goog

[PATCH 1/7] perf inject: don't proceed if perf_session__process_event fails

[David Carrillo-Cisneros]

All paths following perf_session__process_event in __cmd_inject are
useless if __cmd_inject is to fail, some depend on a correct
session->evlist.

First commit to add code that depends on session->evlist without checking
error was commmit e558a5bd8b ("perf inject: Work with files"). It has
grown since then.

Change __cmd_inject to fail immediately after perf_session__process_event
fails.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 tools/perf/builtin-inject.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 42dff0b1375a..65e1c026a2f0 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -694,6 +694,8 @@ static int __cmd_inject(struct perf_inject *inject)
 		lseek(fd, output_data_offset, SEEK_SET);
 
 	ret = perf_session__process_events(session);
+	if (ret)
+		return ret;
 
 	if (!file_out->is_pipe) {
 		if (inject->build_ids)
-- 
2.12.2.715.g7642488e1d-goog

[tip:perf/core] perf inject: Don't proceed if perf_session__process_event() fails

[tip-bot for David Carrillo-Cisneros]

Commit-ID:  bb8d521f77f3e68a713456b7fb1e99f52ff3342c
Gitweb:     http://git.kernel.org/tip/bb8d521f77f3e68a713456b7fb1e99f52ff3342c
Author:     David Carrillo-Cisneros <davidcc@google.com>
AuthorDate: Mon, 10 Apr 2017 13:14:26 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 11 Apr 2017 15:23:40 -0300

perf inject: Don't proceed if perf_session__process_event() fails

All paths following perf_session__process_event() in __cmd_inject() are
useless if __cmd_inject() is to fail, some depend on a correct
session->evlist.

First commit to add code that depends on session->evlist without checking
error was commmit e558a5bd8b ("perf inject: Work with files"). It has
grown since then.

Change __cmd_inject() to fail immediately after
perf_session__process_event() fails.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Andrew Vagin <avagin@openvz.org>
Cc: He Kuang <hekuang@huawei.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Paul Turner <pjt@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Simon Que <sque@chromium.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Fixes: e558a5bd8b74 ("perf inject: Work with files")
Link: http://lkml.kernel.org/r/20170410201432.24807-2-davidcc@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-inject.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 42dff0b..65e1c02 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -694,6 +694,8 @@ static int __cmd_inject(struct perf_inject *inject)
 		lseek(fd, output_data_offset, SEEK_SET);
 
 	ret = perf_session__process_events(session);
+	if (ret)
+		return ret;
 
 	if (!file_out->is_pipe) {
 		if (inject->build_ids)

[PATCH 2/7] perf inject: copy events when reordering events in pipe mode

[David Carrillo-Cisneros]

__perf_session__process_pipe_events reuses the same memory buffer to
process all events in the pipe.

When reordering is needed (e.g. -b option), events are not immediately
flushed, but kept around until reordering is possible, causing
memory corruption.

The problem is usually observed by a "Unknown sample error" output. It
can easily be reproduced by:

  perf record -o - noploop | perf inject -b > output

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 tools/perf/util/ordered-events.c | 3 ++-
 tools/perf/util/session.c        | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
index fe84df1875aa..e70e935b1841 100644
--- a/tools/perf/util/ordered-events.c
+++ b/tools/perf/util/ordered-events.c
@@ -79,7 +79,7 @@ static union perf_event *dup_event(struct ordered_events *oe,
 
 static void free_dup_event(struct ordered_events *oe, union perf_event *event)
 {
-	if (oe->copy_on_queue) {
+	if (event && oe->copy_on_queue) {
 		oe->cur_alloc_size -= event->header.size;
 		free(event);
 	}
@@ -150,6 +150,7 @@ void ordered_events__delete(struct ordered_events *oe, struct ordered_event *eve
 	list_move(&event->list, &oe->cache);
 	oe->nr_events--;
 	free_dup_event(oe, event->event);
+	event->event = NULL;
 }
 
 int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 24259bc2c598..a25302bc55a8 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1656,6 +1656,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session)
 	buf = malloc(cur_size);
 	if (!buf)
 		return -errno;
+	ordered_events__set_copy_on_queue(oe, true);
 more:
 	event = buf;
 	err = readn(fd, event, sizeof(struct perf_event_header));
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCH 2/7] perf inject: copy events when reordering events in pipe mode

[Jiri Olsa]

On Mon, Apr 10, 2017 at 01:14:27PM -0700, David Carrillo-Cisneros wrote:
> __perf_session__process_pipe_events reuses the same memory buffer to
> process all events in the pipe.
> 
> When reordering is needed (e.g. -b option), events are not immediately
> flushed, but kept around until reordering is possible, causing
> memory corruption.
> 
> The problem is usually observed by a "Unknown sample error" output. It
> can easily be reproduced by:
> 
>   perf record -o - noploop | perf inject -b > output
> 
> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> ---
>  tools/perf/util/ordered-events.c | 3 ++-
>  tools/perf/util/session.c        | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
> index fe84df1875aa..e70e935b1841 100644
> --- a/tools/perf/util/ordered-events.c
> +++ b/tools/perf/util/ordered-events.c
> @@ -79,7 +79,7 @@ static union perf_event *dup_event(struct ordered_events *oe,
>  
>  static void free_dup_event(struct ordered_events *oe, union perf_event *event)
>  {
> -	if (oe->copy_on_queue) {
> +	if (event && oe->copy_on_queue) {
>  		oe->cur_alloc_size -= event->header.size;
>  		free(event);
>  	}
> @@ -150,6 +150,7 @@ void ordered_events__delete(struct ordered_events *oe, struct ordered_event *eve
>  	list_move(&event->list, &oe->cache);
>  	oe->nr_events--;
>  	free_dup_event(oe, event->event);
> +	event->event = NULL;

based on the changelog I understand the need for the change below,
but I dont get why do we need this cleanu and check above

thanks,
jirka

>  }
>  
>  int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 24259bc2c598..a25302bc55a8 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1656,6 +1656,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session)
>  	buf = malloc(cur_size);
>  	if (!buf)
>  		return -errno;
> +	ordered_events__set_copy_on_queue(oe, true);
>  more:
>  	event = buf;
>  	err = readn(fd, event, sizeof(struct perf_event_header));
> -- 
> 2.12.2.715.g7642488e1d-goog
> 

Re: [PATCH 2/7] perf inject: copy events when reordering events in pipe mode

[David Carrillo-Cisneros]

free_dup_event is called in two places
  1) ordered_events__free
  2) ordered_events__delete

without this patch, calling 1) after 2) would cause a double
de-allocation, but that's exactly what happens in
__perf_session__process_pipe_events. It wasn't triggered before
because events where not copied when queued.

With this patch, the event->event = NULL in ordered_events__delete
will clear event pointer that is checked in free_dup_event, avoiding
de-allocating the event a second time.

Thanks,
David

On Tue, Apr 11, 2017 at 2:40 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Mon, Apr 10, 2017 at 01:14:27PM -0700, David Carrillo-Cisneros wrote:
>> __perf_session__process_pipe_events reuses the same memory buffer to
>> process all events in the pipe.
>>
>> When reordering is needed (e.g. -b option), events are not immediately
>> flushed, but kept around until reordering is possible, causing
>> memory corruption.
>>
>> The problem is usually observed by a "Unknown sample error" output. It
>> can easily be reproduced by:
>>
>>   perf record -o - noploop | perf inject -b > output
>>
>> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
>> ---
>>  tools/perf/util/ordered-events.c | 3 ++-
>>  tools/perf/util/session.c        | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
>> index fe84df1875aa..e70e935b1841 100644
>> --- a/tools/perf/util/ordered-events.c
>> +++ b/tools/perf/util/ordered-events.c
>> @@ -79,7 +79,7 @@ static union perf_event *dup_event(struct ordered_events *oe,
>>
>>  static void free_dup_event(struct ordered_events *oe, union perf_event *event)
>>  {
>> -     if (oe->copy_on_queue) {
>> +     if (event && oe->copy_on_queue) {
>>               oe->cur_alloc_size -= event->header.size;
>>               free(event);
>>       }
>> @@ -150,6 +150,7 @@ void ordered_events__delete(struct ordered_events *oe, struct ordered_event *eve
>>       list_move(&event->list, &oe->cache);
>>       oe->nr_events--;
>>       free_dup_event(oe, event->event);
>> +     event->event = NULL;
>
> based on the changelog I understand the need for the change below,
> but I dont get why do we need this cleanu and check above
>
> thanks,
> jirka
>
>>  }
>>
>>  int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> index 24259bc2c598..a25302bc55a8 100644
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -1656,6 +1656,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session)
>>       buf = malloc(cur_size);
>>       if (!buf)
>>               return -errno;
>> +     ordered_events__set_copy_on_queue(oe, true);
>>  more:
>>       event = buf;
>>       err = readn(fd, event, sizeof(struct perf_event_header));
>> --
>> 2.12.2.715.g7642488e1d-goog
>>

[tip:perf/core] perf inject: Copy events when reordering events in pipe mode

[tip-bot for David Carrillo-Cisneros]

Commit-ID:  1e0d4f0200e4dbdfc38d818f329d8a0955f7c6f5
Gitweb:     http://git.kernel.org/tip/1e0d4f0200e4dbdfc38d818f329d8a0955f7c6f5
Author:     David Carrillo-Cisneros <davidcc@google.com>
AuthorDate: Mon, 10 Apr 2017 13:14:27 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 11 Apr 2017 15:23:41 -0300

perf inject: Copy events when reordering events in pipe mode

__perf_session__process_pipe_events reuses the same memory buffer to
process all events in the pipe.

When reordering is needed (e.g. -b option), events are not immediately
flushed, but kept around until reordering is possible, causing
memory corruption.

The problem is usually observed by a "Unknown sample error" output. It
can easily be reproduced by:

  perf record -o - noploop | perf inject -b > output

Committer testing:

Before:

  $ perf record -o - stress -t 2 -c 2 | perf inject -b > /dev/null
  stress: info: [8297] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd
  stress: info: [8297] successful run completed in 2s
  [ perf record: Woken up 3 times to write data ]
  [ perf record: Captured and wrote 0.000 MB - ]
  Warning:
  Found 1 unknown events!

  Is this an older tool processing a perf.data file generated by a more recent tool?

  If that is not the case, consider reporting to linux-kernel@vger.kernel.org.

  $

After:

  $ perf record -o - stress -t 2 -c 2 | perf inject -b > /dev/null
  stress: info: [9027] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd
  stress: info: [9027] successful run completed in 2s
  [ perf record: Woken up 3 times to write data ]
  [ perf record: Captured and wrote 0.000 MB - ]
  no symbols found in /usr/bin/stress, maybe install a debug package?
  no symbols found in /usr/bin/stress, maybe install a debug package?
  $

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Paul Turner <pjt@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Simon Que <sque@chromium.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20170410201432.24807-3-davidcc@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/ordered-events.c | 3 ++-
 tools/perf/util/session.c        | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
index fe84df1..e70e935 100644
--- a/tools/perf/util/ordered-events.c
+++ b/tools/perf/util/ordered-events.c
@@ -79,7 +79,7 @@ static union perf_event *dup_event(struct ordered_events *oe,
 
 static void free_dup_event(struct ordered_events *oe, union perf_event *event)
 {
-	if (oe->copy_on_queue) {
+	if (event && oe->copy_on_queue) {
 		oe->cur_alloc_size -= event->header.size;
 		free(event);
 	}
@@ -150,6 +150,7 @@ void ordered_events__delete(struct ordered_events *oe, struct ordered_event *eve
 	list_move(&event->list, &oe->cache);
 	oe->nr_events--;
 	free_dup_event(oe, event->event);
+	event->event = NULL;
 }
 
 int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 24259bc2..a25302b 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1656,6 +1656,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session)
 	buf = malloc(cur_size);
 	if (!buf)
 		return -errno;
+	ordered_events__set_copy_on_queue(oe, true);
 more:
 	event = buf;
 	err = readn(fd, event, sizeof(struct perf_event_header));

[PATCH 3/7] perf tool: describe pipe mode in perf.data-file-fomat.txt

[David Carrillo-Cisneros]

Add a minimal description of pipe's data format.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 tools/perf/Documentation/perf.data-file-format.txt | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf.data-file-format.txt b/tools/perf/Documentation/perf.data-file-format.txt
index b664b18d3991..fa2a9132f0a9 100644
--- a/tools/perf/Documentation/perf.data-file-format.txt
+++ b/tools/perf/Documentation/perf.data-file-format.txt
@@ -11,8 +11,8 @@ All fields are in native-endian of the machine that generated the perf.data.
 
 When perf is writing to a pipe it uses a special version of the file
 format that does not rely on seeking to adjust data offsets.  This
-format is not described here. The pipe version can be converted to
-normal perf.data with perf inject.
+format is described in "Pipe-mode data" section. The pipe data version can be
+augmented with additional events using perf inject.
 
 The file starts with a perf_header:
 
@@ -411,6 +411,21 @@ An array bound by the perf_file_section size.
 
 ids points to a array of uint64_t defining the ids for event attr attr.
 
+Pipe-mode data
+
+Pipe-mode avoid seeks in the file by removing the perf_file_section and flags
+from the struct perf_header. The trimmed header is:
+
+struct perf_pipe_file_header {
+	u64				magic;
+	u64				size;
+};
+
+The information about attrs, data, and event_types is instead in the
+synthesized events PERF_RECORD_ATTR, PERF_RECORD_HEADER_TRACING_DATA and
+PERF_RECORD_HEADER_EVENT_TYPE that are generated by perf record in pipe-mode.
+
+
 References:
 
 include/uapi/linux/perf_event.h
-- 
2.12.2.715.g7642488e1d-goog

[tip:perf/core] perf tools: Describe pipe mode in perf.data-file-fomat.txt

[tip-bot for David Carrillo-Cisneros]

Commit-ID:  6d13491e2d4944180c9b4fb6ddca4e34b1537836
Gitweb:     http://git.kernel.org/tip/6d13491e2d4944180c9b4fb6ddca4e34b1537836
Author:     David Carrillo-Cisneros <davidcc@google.com>
AuthorDate: Mon, 10 Apr 2017 13:14:28 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 11 Apr 2017 15:23:41 -0300

perf tools: Describe pipe mode in perf.data-file-fomat.txt

Add a minimal description of pipe's data format.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Paul Turner <pjt@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Simon Que <sque@chromium.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20170410201432.24807-4-davidcc@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf.data-file-format.txt | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf.data-file-format.txt b/tools/perf/Documentation/perf.data-file-format.txt
index b664b18..fa2a913 100644
--- a/tools/perf/Documentation/perf.data-file-format.txt
+++ b/tools/perf/Documentation/perf.data-file-format.txt
@@ -11,8 +11,8 @@ All fields are in native-endian of the machine that generated the perf.data.
 
 When perf is writing to a pipe it uses a special version of the file
 format that does not rely on seeking to adjust data offsets.  This
-format is not described here. The pipe version can be converted to
-normal perf.data with perf inject.
+format is described in "Pipe-mode data" section. The pipe data version can be
+augmented with additional events using perf inject.
 
 The file starts with a perf_header:
 
@@ -411,6 +411,21 @@ An array bound by the perf_file_section size.
 
 ids points to a array of uint64_t defining the ids for event attr attr.
 
+Pipe-mode data
+
+Pipe-mode avoid seeks in the file by removing the perf_file_section and flags
+from the struct perf_header. The trimmed header is:
+
+struct perf_pipe_file_header {
+	u64				magic;
+	u64				size;
+};
+
+The information about attrs, data, and event_types is instead in the
+synthesized events PERF_RECORD_ATTR, PERF_RECORD_HEADER_TRACING_DATA and
+PERF_RECORD_HEADER_EVENT_TYPE that are generated by perf record in pipe-mode.
+
+
 References:
 
 include/uapi/linux/perf_event.h

[PATCH 4/7] perf annotate: process attr and build_id records

[David Carrillo-Cisneros]

perf annotate did not get some love for pipe-mode, and did not have
.attr and .buil_id setup (while record and inject did. Fix that.

It can easily be reproduced by:

  perf record -o - noploop | perf annotate

that in my system shows:
    0xd8 [0x28]: failed to process type: 9

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 tools/perf/builtin-annotate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 56a7c8d210b9..b2b2722f6bb7 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -394,6 +394,8 @@ int cmd_annotate(int argc, const char **argv)
 			.exit	= perf_event__process_exit,
 			.fork	= perf_event__process_fork,
 			.namespaces = perf_event__process_namespaces,
+			.attr	= perf_event__process_attr,
+			.build_id = perf_event__process_build_id,
 			.ordered_events = true,
 			.ordering_requires_timestamps = true,
 		},
-- 
2.12.2.715.g7642488e1d-goog

[tip:perf/core] perf annotate: Process attr and build_id records

[tip-bot for David Carrillo-Cisneros]

Commit-ID:  6ab11f3a35aa07be2ff167b9de37e6c1eb58396b
Gitweb:     http://git.kernel.org/tip/6ab11f3a35aa07be2ff167b9de37e6c1eb58396b
Author:     David Carrillo-Cisneros <davidcc@google.com>
AuthorDate: Mon, 10 Apr 2017 13:14:29 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 11 Apr 2017 15:23:42 -0300

perf annotate: Process attr and build_id records

perf annotate did not get some love for pipe-mode, and did not have
.attr and .buil_id setup (while record and inject did. Fix that.

It can easily be reproduced by:

  perf record -o - noploop | perf annotate

that in my system shows:
    0xd8 [0x28]: failed to process type: 9

Committer Testing:

Before:

  $ perf record -o - stress -t 2 -c 2 | perf annotate --stdio
  stress: info: [11060] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd
  0x4470 [0x28]: failed to process type: 9
  $ stress: info: [11060] successful run completed in 2s

  $

After:

  $ perf record -o - stress -t 2 -c 2 | perf annotate --stdio
  stress: info: [11871] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd
  stress: info: [11871] successful run completed in 2s
  [ perf record: Woken up 2 times to write data ]
  [ perf record: Captured and wrote 0.000 MB - ]
  no symbols found in /usr/bin/stress, maybe install a debug package?
   Percent |      Source code & Disassembly of libc-2.24.so for cycles:uhH (6117 samples)
  ---------------------------------------------------------------------------------------
           :
           :      Disassembly of section .text:
           :
           :      000000000003b050 <random_r>:
           :      __random_r():
     10.56 :        3b050:       test   %rdi,%rdi
      0.00 :        3b053:       je     3b0d0 <random_r+0x80>
      0.34 :        3b055:       test   %rsi,%rsi
      0.00 :        3b058:       je     3b0d0 <random_r+0x80>
      0.46 :        3b05a:       mov    0x18(%rdi),%eax
     12.44 :        3b05d:       mov    0x10(%rdi),%r8
      0.18 :        3b061:       test   %eax,%eax
      0.00 :        3b063:       je     3b0b0 <random_r+0x60>
<SNIP>

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Paul Turner <pjt@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Simon Que <sque@chromium.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20170410201432.24807-5-davidcc@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-annotate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 56a7c8d..b2b2722 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -394,6 +394,8 @@ int cmd_annotate(int argc, const char **argv)
 			.exit	= perf_event__process_exit,
 			.fork	= perf_event__process_fork,
 			.namespaces = perf_event__process_namespaces,
+			.attr	= perf_event__process_attr,
+			.build_id = perf_event__process_build_id,
 			.ordered_events = true,
 			.ordering_requires_timestamps = true,
 		},

[PATCH 5/7] perf session: don't rely on evlist in pipe mode

[David Carrillo-Cisneros]

Session sets a number parameters that rely on evlist. These parameters
are not used in pipe-mode and should not be set, since evlist is
unavailable. Fix that.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 tools/perf/util/session.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index a25302bc55a8..db554b7461b8 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 			if (perf_session__open(session) < 0)
 				goto out_close;
 
-			perf_session__set_id_hdr_size(session);
-			perf_session__set_comm_exec(session);
+			/*
+			 * set session attributes that are present in perf.data
+			 * but not in pipe-mode.
+			 */
+			if (!file->is_pipe) {
+				perf_session__set_id_hdr_size(session);
+				perf_session__set_comm_exec(session);
+			}
 		}
 	} else  {
 		session->machines.host.env = &perf_env;
@@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 			pr_warning("Cannot read kernel map\n");
 	}
 
-	if (tool && tool->ordering_requires_timestamps &&
+	/*
+	 * In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is
+	 * processed, so perf_evlist__sample_id_all is not meaningful here.
+	 */
+	if (!file->is_pipe && tool && tool->ordering_requires_timestamps &&
 	    tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
 		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
 		tool->ordered_events = false;
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCH 5/7] perf session: don't rely on evlist in pipe mode

[Arnaldo Carvalho de Melo]

Em Mon, Apr 10, 2017 at 01:14:30PM -0700, David Carrillo-Cisneros escreveu:
> Session sets a number parameters that rely on evlist. These parameters
> are not used in pipe-mode and should not be set, since evlist is
> unavailable. Fix that.

Ditching this one as well, until further investigation, well, file must
be null in this case...

[acme@jouet linux]$ git bisect bad
5d71c27204d51492abe9fc69064fd36a0bda410b is the first bad commit
commit 5d71c27204d51492abe9fc69064fd36a0bda410b
Author: David Carrillo-Cisneros <davidcc@google.com>
Date:   Mon Apr 10 13:14:30 2017 -0700

    perf session: Don't rely on evlist in pipe mode
    
    Session sets a number parameters that rely on evlist. These parameters
    are not used in pipe-mode and should not be set, since evlist is
    unavailable. Fix that.
    
    Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
    Acked-by: Jiri Olsa <jolsa@kernel.org>
    Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
    Cc: Andi Kleen <ak@linux.intel.com>
    Cc: He Kuang <hekuang@huawei.com>
    Cc: Masami Hiramatsu <mhiramat@kernel.org>
    Cc: Paul Turner <pjt@google.com>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Simon Que <sque@chromium.org>
    Cc: Stephane Eranian <eranian@google.com>
    Cc: Wang Nan <wangnan0@huawei.com>
    Link: http://lkml.kernel.org/r/20170410201432.24807-6-davidcc@google.com
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

:040000 040000 7df28fe9d7c9d962b78c0532f4961caea2737b59 34e4fed0042b1401c1a1a247d8ae40df17f9d272 M	tools
[acme@jouet linux]$
 
> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> ---
>  tools/perf/util/session.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index a25302bc55a8..db554b7461b8 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>  			if (perf_session__open(session) < 0)
>  				goto out_close;
>  
> -			perf_session__set_id_hdr_size(session);
> -			perf_session__set_comm_exec(session);
> +			/*
> +			 * set session attributes that are present in perf.data
> +			 * but not in pipe-mode.
> +			 */
> +			if (!file->is_pipe) {
> +				perf_session__set_id_hdr_size(session);
> +				perf_session__set_comm_exec(session);
> +			}
>  		}
>  	} else  {
>  		session->machines.host.env = &perf_env;
> @@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>  			pr_warning("Cannot read kernel map\n");
>  	}
>  
> -	if (tool && tool->ordering_requires_timestamps &&
> +	/*
> +	 * In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is
> +	 * processed, so perf_evlist__sample_id_all is not meaningful here.
> +	 */
> +	if (!file->is_pipe && tool && tool->ordering_requires_timestamps &&
>  	    tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
>  		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
>  		tool->ordered_events = false;
> -- 
> 2.12.2.715.g7642488e1d-goog

Re: [PATCH 5/7] perf session: don't rely on evlist in pipe mode

[Arnaldo Carvalho de Melo]

Em Tue, Apr 11, 2017 at 03:40:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Apr 10, 2017 at 01:14:30PM -0700, David Carrillo-Cisneros escreveu:
> > Session sets a number parameters that rely on evlist. These parameters
> > are not used in pipe-mode and should not be set, since evlist is
> > unavailable. Fix that.
> 
> Ditching this one as well, until further investigation, well, file must
> be null in this case...

Ah, this is the problem:

[root@jouet ~]# perf top
perf: Segmentation fault
-------- backtrace --------
perf[0x57eddb]
/lib64/libc.so.6(+0x35990)[0x7fb435d06990]
perf(perf_session__new+0xb7)[0x4eb547]
perf(cmd_top+0xde9)[0x44ce89]
perf[0x49953f]
perf[0x499831]
perf(main+0x252)[0x428dd2]
/lib64/libc.so.6(__libc_start_main+0xf1)[0x7fb435cf1401]
perf(_start+0x2a)[0x4292da]
[0x0]
[root@jouet ~]#
 
> [acme@jouet linux]$ git bisect bad
> 5d71c27204d51492abe9fc69064fd36a0bda410b is the first bad commit
> commit 5d71c27204d51492abe9fc69064fd36a0bda410b
> Author: David Carrillo-Cisneros <davidcc@google.com>
> Date:   Mon Apr 10 13:14:30 2017 -0700
> 
>     perf session: Don't rely on evlist in pipe mode
>     
>     Session sets a number parameters that rely on evlist. These parameters
>     are not used in pipe-mode and should not be set, since evlist is
>     unavailable. Fix that.
>     
>     Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
>     Acked-by: Jiri Olsa <jolsa@kernel.org>
>     Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>     Cc: Andi Kleen <ak@linux.intel.com>
>     Cc: He Kuang <hekuang@huawei.com>
>     Cc: Masami Hiramatsu <mhiramat@kernel.org>
>     Cc: Paul Turner <pjt@google.com>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Cc: Simon Que <sque@chromium.org>
>     Cc: Stephane Eranian <eranian@google.com>
>     Cc: Wang Nan <wangnan0@huawei.com>
>     Link: http://lkml.kernel.org/r/20170410201432.24807-6-davidcc@google.com
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> :040000 040000 7df28fe9d7c9d962b78c0532f4961caea2737b59 34e4fed0042b1401c1a1a247d8ae40df17f9d272 M	tools
> [acme@jouet linux]$
>  
> > Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> > ---
> >  tools/perf/util/session.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index a25302bc55a8..db554b7461b8 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
> >  			if (perf_session__open(session) < 0)
> >  				goto out_close;
> >  
> > -			perf_session__set_id_hdr_size(session);
> > -			perf_session__set_comm_exec(session);
> > +			/*
> > +			 * set session attributes that are present in perf.data
> > +			 * but not in pipe-mode.
> > +			 */
> > +			if (!file->is_pipe) {
> > +				perf_session__set_id_hdr_size(session);
> > +				perf_session__set_comm_exec(session);
> > +			}
> >  		}
> >  	} else  {
> >  		session->machines.host.env = &perf_env;
> > @@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
> >  			pr_warning("Cannot read kernel map\n");
> >  	}
> >  
> > -	if (tool && tool->ordering_requires_timestamps &&
> > +	/*
> > +	 * In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is
> > +	 * processed, so perf_evlist__sample_id_all is not meaningful here.
> > +	 */
> > +	if (!file->is_pipe && tool && tool->ordering_requires_timestamps &&
> >  	    tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
> >  		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
> >  		tool->ordered_events = false;
> > -- 
> > 2.12.2.715.g7642488e1d-goog

Re: [PATCH 5/7] perf session: don't rely on evlist in pipe mode

[Arnaldo Carvalho de Melo]

Em Tue, Apr 11, 2017 at 03:40:50PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Apr 11, 2017 at 03:40:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Apr 10, 2017 at 01:14:30PM -0700, David Carrillo-Cisneros escreveu:
> > > Session sets a number parameters that rely on evlist. These parameters
> > > are not used in pipe-mode and should not be set, since evlist is
> > > unavailable. Fix that.
> > 
> > Ditching this one as well, until further investigation, well, file must
> > be null in this case...

This should be enough, right?

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index db554b7461b8..37cf4e6abf9f 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -144,7 +144,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 			 * set session attributes that are present in perf.data
 			 * but not in pipe-mode.
 			 */
-			if (!file->is_pipe) {
+			if (!file || !file->is_pipe) {
 				perf_session__set_id_hdr_size(session);
 				perf_session__set_comm_exec(session);
 			}
@@ -166,7 +166,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 	 * In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is
 	 * processed, so perf_evlist__sample_id_all is not meaningful here.
 	 */
-	if (!file->is_pipe && tool && tool->ordering_requires_timestamps &&
+	if ((!file || !file->is_pipe) && tool && tool->ordering_requires_timestamps &&
 	    tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
 		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
 		tool->ordered_events = false;
 
> Ah, this is the problem:
> 
> [root@jouet ~]# perf top
> perf: Segmentation fault
> -------- backtrace --------
> perf[0x57eddb]
> /lib64/libc.so.6(+0x35990)[0x7fb435d06990]
> perf(perf_session__new+0xb7)[0x4eb547]
> perf(cmd_top+0xde9)[0x44ce89]
> perf[0x49953f]
> perf[0x499831]
> perf(main+0x252)[0x428dd2]
> /lib64/libc.so.6(__libc_start_main+0xf1)[0x7fb435cf1401]
> perf(_start+0x2a)[0x4292da]
> [0x0]
> [root@jouet ~]#
>  
> > [acme@jouet linux]$ git bisect bad
> > 5d71c27204d51492abe9fc69064fd36a0bda410b is the first bad commit
> > commit 5d71c27204d51492abe9fc69064fd36a0bda410b
> > Author: David Carrillo-Cisneros <davidcc@google.com>
> > Date:   Mon Apr 10 13:14:30 2017 -0700
> > 
> >     perf session: Don't rely on evlist in pipe mode
> >     
> >     Session sets a number parameters that rely on evlist. These parameters
> >     are not used in pipe-mode and should not be set, since evlist is
> >     unavailable. Fix that.
> >     
> >     Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> >     Acked-by: Jiri Olsa <jolsa@kernel.org>
> >     Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >     Cc: Andi Kleen <ak@linux.intel.com>
> >     Cc: He Kuang <hekuang@huawei.com>
> >     Cc: Masami Hiramatsu <mhiramat@kernel.org>
> >     Cc: Paul Turner <pjt@google.com>
> >     Cc: Peter Zijlstra <peterz@infradead.org>
> >     Cc: Simon Que <sque@chromium.org>
> >     Cc: Stephane Eranian <eranian@google.com>
> >     Cc: Wang Nan <wangnan0@huawei.com>
> >     Link: http://lkml.kernel.org/r/20170410201432.24807-6-davidcc@google.com
> >     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > :040000 040000 7df28fe9d7c9d962b78c0532f4961caea2737b59 34e4fed0042b1401c1a1a247d8ae40df17f9d272 M	tools
> > [acme@jouet linux]$
> >  
> > > Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> > > ---
> > >  tools/perf/util/session.c | 16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > > index a25302bc55a8..db554b7461b8 100644
> > > --- a/tools/perf/util/session.c
> > > +++ b/tools/perf/util/session.c
> > > @@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
> > >  			if (perf_session__open(session) < 0)
> > >  				goto out_close;
> > >  
> > > -			perf_session__set_id_hdr_size(session);
> > > -			perf_session__set_comm_exec(session);
> > > +			/*
> > > +			 * set session attributes that are present in perf.data
> > > +			 * but not in pipe-mode.
> > > +			 */
> > > +			if (!file->is_pipe) {
> > > +				perf_session__set_id_hdr_size(session);
> > > +				perf_session__set_comm_exec(session);
> > > +			}
> > >  		}
> > >  	} else  {
> > >  		session->machines.host.env = &perf_env;
> > > @@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
> > >  			pr_warning("Cannot read kernel map\n");
> > >  	}
> > >  
> > > -	if (tool && tool->ordering_requires_timestamps &&
> > > +	/*
> > > +	 * In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is
> > > +	 * processed, so perf_evlist__sample_id_all is not meaningful here.
> > > +	 */
> > > +	if (!file->is_pipe && tool && tool->ordering_requires_timestamps &&
> > >  	    tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
> > >  		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
> > >  		tool->ordered_events = false;
> > > -- 
> > > 2.12.2.715.g7642488e1d-goog

Re: [PATCH 5/7] perf session: don't rely on evlist in pipe mode

[David Carrillo-Cisneros]

Only the second one is needed. file is already checked for the first one.

On Tue, Apr 11, 2017 at 11:44 AM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Tue, Apr 11, 2017 at 03:40:50PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Tue, Apr 11, 2017 at 03:40:14PM -0300, Arnaldo Carvalho de Melo escreveu:
>> > Em Mon, Apr 10, 2017 at 01:14:30PM -0700, David Carrillo-Cisneros escreveu:
>> > > Session sets a number parameters that rely on evlist. These parameters
>> > > are not used in pipe-mode and should not be set, since evlist is
>> > > unavailable. Fix that.
>> >
>> > Ditching this one as well, until further investigation, well, file must
>> > be null in this case...
>
> This should be enough, right?
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index db554b7461b8..37cf4e6abf9f 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -144,7 +144,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>                          * set session attributes that are present in perf.data
>                          * but not in pipe-mode.
>                          */
> -                       if (!file->is_pipe) {
> +                       if (!file || !file->is_pipe) {
>                                 perf_session__set_id_hdr_size(session);
>                                 perf_session__set_comm_exec(session);
>                         }
> @@ -166,7 +166,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>          * In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is
>          * processed, so perf_evlist__sample_id_all is not meaningful here.
>          */
> -       if (!file->is_pipe && tool && tool->ordering_requires_timestamps &&
> +       if ((!file || !file->is_pipe) && tool && tool->ordering_requires_timestamps &&
>             tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
>                 dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
>                 tool->ordered_events = false;
>
>> Ah, this is the problem:
>>
>> [root@jouet ~]# perf top
>> perf: Segmentation fault
>> -------- backtrace --------
>> perf[0x57eddb]
>> /lib64/libc.so.6(+0x35990)[0x7fb435d06990]
>> perf(perf_session__new+0xb7)[0x4eb547]
>> perf(cmd_top+0xde9)[0x44ce89]
>> perf[0x49953f]
>> perf[0x499831]
>> perf(main+0x252)[0x428dd2]
>> /lib64/libc.so.6(__libc_start_main+0xf1)[0x7fb435cf1401]
>> perf(_start+0x2a)[0x4292da]
>> [0x0]
>> [root@jouet ~]#
>>
>> > [acme@jouet linux]$ git bisect bad
>> > 5d71c27204d51492abe9fc69064fd36a0bda410b is the first bad commit
>> > commit 5d71c27204d51492abe9fc69064fd36a0bda410b
>> > Author: David Carrillo-Cisneros <davidcc@google.com>
>> > Date:   Mon Apr 10 13:14:30 2017 -0700
>> >
>> >     perf session: Don't rely on evlist in pipe mode
>> >
>> >     Session sets a number parameters that rely on evlist. These parameters
>> >     are not used in pipe-mode and should not be set, since evlist is
>> >     unavailable. Fix that.
>> >
>> >     Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
>> >     Acked-by: Jiri Olsa <jolsa@kernel.org>
>> >     Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> >     Cc: Andi Kleen <ak@linux.intel.com>
>> >     Cc: He Kuang <hekuang@huawei.com>
>> >     Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> >     Cc: Paul Turner <pjt@google.com>
>> >     Cc: Peter Zijlstra <peterz@infradead.org>
>> >     Cc: Simon Que <sque@chromium.org>
>> >     Cc: Stephane Eranian <eranian@google.com>
>> >     Cc: Wang Nan <wangnan0@huawei.com>
>> >     Link: http://lkml.kernel.org/r/20170410201432.24807-6-davidcc@google.com
>> >     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>> >
>> > :040000 040000 7df28fe9d7c9d962b78c0532f4961caea2737b59 34e4fed0042b1401c1a1a247d8ae40df17f9d272 M  tools
>> > [acme@jouet linux]$
>> >
>> > > Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
>> > > ---
>> > >  tools/perf/util/session.c | 16 +++++++++++++---
>> > >  1 file changed, 13 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> > > index a25302bc55a8..db554b7461b8 100644
>> > > --- a/tools/perf/util/session.c
>> > > +++ b/tools/perf/util/session.c
>> > > @@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>> > >                   if (perf_session__open(session) < 0)
>> > >                           goto out_close;
>> > >
>> > > -                 perf_session__set_id_hdr_size(session);
>> > > -                 perf_session__set_comm_exec(session);
>> > > +                 /*
>> > > +                  * set session attributes that are present in perf.data
>> > > +                  * but not in pipe-mode.
>> > > +                  */
>> > > +                 if (!file->is_pipe) {
>> > > +                         perf_session__set_id_hdr_size(session);
>> > > +                         perf_session__set_comm_exec(session);
>> > > +                 }
>> > >           }
>> > >   } else  {
>> > >           session->machines.host.env = &perf_env;
>> > > @@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>> > >                   pr_warning("Cannot read kernel map\n");
>> > >   }
>> > >
>> > > - if (tool && tool->ordering_requires_timestamps &&
>> > > + /*
>> > > +  * In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is
>> > > +  * processed, so perf_evlist__sample_id_all is not meaningful here.
>> > > +  */
>> > > + if (!file->is_pipe && tool && tool->ordering_requires_timestamps &&
>> > >       tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
>> > >           dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
>> > >           tool->ordered_events = false;
>> > > --
>> > > 2.12.2.715.g7642488e1d-goog

Re: [PATCH 5/7] perf session: don't rely on evlist in pipe mode

[Arnaldo Carvalho de Melo]

Em Tue, Apr 11, 2017 at 11:59:18AM -0700, David Carrillo-Cisneros escreveu:
> Only the second one is needed. file is already checked for the first one.

Right, fixed, tested.

- Arnaldo
 

[tip:perf/core] perf session: Don't rely on evlist in pipe mode

[tip-bot for David Carrillo-Cisneros]

Commit-ID:  0973ad97c187e06aece61f685b9c3b2d93290a73
Gitweb:     http://git.kernel.org/tip/0973ad97c187e06aece61f685b9c3b2d93290a73
Author:     David Carrillo-Cisneros <davidcc@google.com>
AuthorDate: Mon, 10 Apr 2017 13:14:30 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 11 Apr 2017 16:22:20 -0300

perf session: Don't rely on evlist in pipe mode

Session sets a number parameters that rely on evlist. These parameters
are not used in pipe-mode and should not be set, since evlist is
unavailable. Fix that.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Paul Turner <pjt@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Simon Que <sque@chromium.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20170410201432.24807-6-davidcc@google.com
[ Check if file != NULL in perf_session__new(), like when used by builtin-top.c ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/session.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index a25302b..7b740a7 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 			if (perf_session__open(session) < 0)
 				goto out_close;
 
-			perf_session__set_id_hdr_size(session);
-			perf_session__set_comm_exec(session);
+			/*
+			 * set session attributes that are present in perf.data
+			 * but not in pipe-mode.
+			 */
+			if (!file->is_pipe) {
+				perf_session__set_id_hdr_size(session);
+				perf_session__set_comm_exec(session);
+			}
 		}
 	} else  {
 		session->machines.host.env = &perf_env;
@@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 			pr_warning("Cannot read kernel map\n");
 	}
 
-	if (tool && tool->ordering_requires_timestamps &&
+	/*
+	 * In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is
+	 * processed, so perf_evlist__sample_id_all is not meaningful here.
+	 */
+	if ((!file || !file->is_pipe) && tool && tool->ordering_requires_timestamps &&
 	    tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
 		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
 		tool->ordered_events = false;

[PATCH 6/7] perf tool: protect empty evlists

[David Carrillo-Cisneros]

A common pattern in in pipe-mode bugs is accessing an empty evlist.
Return NULL to make it easier to catch this problems.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 tools/perf/util/evlist.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 39942995f537..ba4788462325 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -257,11 +257,15 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
 
 static inline struct perf_evsel *perf_evlist__first(struct perf_evlist *evlist)
 {
+	if (list_empty(&evlist->entries))
+		return NULL;
 	return list_entry(evlist->entries.next, struct perf_evsel, node);
 }
 
 static inline struct perf_evsel *perf_evlist__last(struct perf_evlist *evlist)
 {
+	if (list_empty(&evlist->entries))
+		return NULL;
 	return list_entry(evlist->entries.prev, struct perf_evsel, node);
 }
 
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCH 6/7] perf tool: protect empty evlists

[Arnaldo Carvalho de Melo]

Em Mon, Apr 10, 2017 at 01:14:31PM -0700, David Carrillo-Cisneros escreveu:
> A common pattern in in pipe-mode bugs is accessing an empty evlist.
> Return NULL to make it easier to catch this problems.

This one is far reaching, we need to take care handling all places using
these functions, albeit probably most cases will always have an evsel, I
looked at builtin-top.c for instance, and I think we need to be more
careful here, will leave this for later.

- Arnaldo
 
> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> ---
>  tools/perf/util/evlist.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 39942995f537..ba4788462325 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -257,11 +257,15 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
>  
>  static inline struct perf_evsel *perf_evlist__first(struct perf_evlist *evlist)
>  {
> +	if (list_empty(&evlist->entries))
> +		return NULL;
>  	return list_entry(evlist->entries.next, struct perf_evsel, node);
>  }
>  
>  static inline struct perf_evsel *perf_evlist__last(struct perf_evlist *evlist)
>  {
> +	if (list_empty(&evlist->entries))
> +		return NULL;
>  	return list_entry(evlist->entries.prev, struct perf_evsel, node);
>  }
>  
> -- 
> 2.12.2.715.g7642488e1d-goog

Re: [PATCH 6/7] perf tool: protect empty evlists

[David Carrillo-Cisneros]

I did my best to fix those issues in perf inject and perf report, but
yeah, other commands may have issues.

On Tue, Apr 11, 2017 at 11:12 AM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Mon, Apr 10, 2017 at 01:14:31PM -0700, David Carrillo-Cisneros escreveu:
>> A common pattern in in pipe-mode bugs is accessing an empty evlist.
>> Return NULL to make it easier to catch this problems.
>
> This one is far reaching, we need to take care handling all places using
> these functions, albeit probably most cases will always have an evsel, I
> looked at builtin-top.c for instance, and I think we need to be more
> careful here, will leave this for later.
>
> - Arnaldo
>
>> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
>> ---
>>  tools/perf/util/evlist.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
>> index 39942995f537..ba4788462325 100644
>> --- a/tools/perf/util/evlist.h
>> +++ b/tools/perf/util/evlist.h
>> @@ -257,11 +257,15 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
>>
>>  static inline struct perf_evsel *perf_evlist__first(struct perf_evlist *evlist)
>>  {
>> +     if (list_empty(&evlist->entries))
>> +             return NULL;
>>       return list_entry(evlist->entries.next, struct perf_evsel, node);
>>  }
>>
>>  static inline struct perf_evsel *perf_evlist__last(struct perf_evlist *evlist)
>>  {
>> +     if (list_empty(&evlist->entries))
>> +             return NULL;
>>       return list_entry(evlist->entries.prev, struct perf_evsel, node);
>>  }
>>
>> --
>> 2.12.2.715.g7642488e1d-goog

[PATCH 7/7] perf tool: do not print missing features in pipe-mode

[David Carrillo-Cisneros]

Pipe-mode has no perf.data header, hence no upfront knowledge
of presend and missing features, hence, do not print missing
features in pipe-mode.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 tools/perf/util/header.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index ef09f26e67da..2ccc7f06db79 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2270,6 +2270,9 @@ int perf_header__fprintf_info(struct perf_session *session, FILE *fp, bool full)
 	perf_header__process_sections(header, fd, &hd,
 				      perf_file_section__fprintf_info);
 
+	if (session->file->is_pipe)
+		return 0;
+
 	fprintf(fp, "# missing features: ");
 	for_each_clear_bit(bit, header->adds_features, HEADER_LAST_FEATURE) {
 		if (bit)
-- 
2.12.2.715.g7642488e1d-goog

[tip:perf/core] perf tools: Do not print missing features in pipe-mode

[tip-bot for David Carrillo-Cisneros]

Commit-ID:  c9d1c93421e3b3c7051b193c9cf648a3bc55cb3e
Gitweb:     http://git.kernel.org/tip/c9d1c93421e3b3c7051b193c9cf648a3bc55cb3e
Author:     David Carrillo-Cisneros <davidcc@google.com>
AuthorDate: Mon, 10 Apr 2017 13:14:32 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 11 Apr 2017 16:22:22 -0300

perf tools: Do not print missing features in pipe-mode

Pipe-mode has no perf.data header, hence no upfront knowledge of presend
and missing features, hence, do not print missing features in pipe-mode.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Paul Turner <pjt@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Simon Que <sque@chromium.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20170410201432.24807-8-davidcc@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index ef09f26..2ccc7f0 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2270,6 +2270,9 @@ int perf_header__fprintf_info(struct perf_session *session, FILE *fp, bool full)
 	perf_header__process_sections(header, fd, &hd,
 				      perf_file_section__fprintf_info);
 
+	if (session->file->is_pipe)
+		return 0;
+
 	fprintf(fp, "# missing features: ");
 	for_each_clear_bit(bit, header->adds_features, HEADER_LAST_FEATURE) {
 		if (bit)

Re: [PATCH 0/7] perf tool: pipe-mode fixes

[Jiri Olsa]

On Mon, Apr 10, 2017 at 01:14:25PM -0700, David Carrillo-Cisneros wrote:
> Various fixes for perf tool pipe-mode for bugs that arose while trying
> to make this work:
> 
>     perf record -o - noploop | perf inject -b | perf annotate -i -
> 
> David Carrillo-Cisneros (7):
>   perf inject: don't proceed if perf_session__process_event fails
>   perf inject: copy events when reordering events in pipe mode
>   perf tool: describe pipe mode in perf.data-file-fomat.txt
>   perf annotate: process attr and build_id records
>   perf session: don't rely on evlist in pipe mode
>   perf tool: protect empty evlists
>   perf tool: do not print missing features in pipe-mode

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

Re: [PATCH 0/7] perf tool: pipe-mode fixes

[Arnaldo Carvalho de Melo]

Em Tue, Apr 11, 2017 at 11:49:06AM +0200, Jiri Olsa escreveu:
> On Mon, Apr 10, 2017 at 01:14:25PM -0700, David Carrillo-Cisneros wrote:
> > Various fixes for perf tool pipe-mode for bugs that arose while trying
> > to make this work:
> > 
> >     perf record -o - noploop | perf inject -b | perf annotate -i -
> > 
> > David Carrillo-Cisneros (7):
> >   perf inject: don't proceed if perf_session__process_event fails
> >   perf inject: copy events when reordering events in pipe mode
> >   perf tool: describe pipe mode in perf.data-file-fomat.txt
> >   perf annotate: process attr and build_id records
> >   perf session: don't rely on evlist in pipe mode
> >   perf tool: protect empty evlists
> >   perf tool: do not print missing features in pipe-mode
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Applied all, except 6/7, will audit all users of those first() and
last() methods to check if this is ok or what changes are needed to
robustify code assuming they always return an evsel (which is false, and
this patch fixes, as the evlist may be empty).

- Arnaldo.