perf tools: Some fixes

Hi

Here are some fixes I had to do while testing Intel PT on 32-bit. They relate to changes that are currently queued.

I emailed Jiri about the first one, but he wrote he couldn't reproduce it, so I have just made a patch.

Adrian Hunter (5):
perf tools: Fix function declarations needed by parse-events.y perf build: Fix libunwind feature detection on 32-bit x86 perf tools: Fix parse_events_error dereferences perf session: Fix perf_session__peek_event()
perf tools: Fix data_read_offset() file opening

 tools/perf/config/Makefile     |  2 +-
 tools/perf/util/dso.c          |  2 +-
 tools/perf/util/parse-events.c | 18 ++++++++++--------
 tools/perf/util/parse-events.h |  6 ++++++
 tools/perf/util/parse-events.y |  6 ++++--
 tools/perf/util/session.c      |  6 +++---
 6 files changed, 25 insertions(+), 15 deletions(-)

Regards
Adrian

Patch "perf tools: Add location to pmu event terms" moved declarations for parse_events_term__num() and parse_events_term__str() so that they were no longer visible in parse-events.y. That can result in segfaults as the arguments no longer need match the function prototype.

Move the declarations back, changing YYLTYPE pointers to pointers-to-void because YYLTYPE is not generated until parse-events.y is processed.

---
 tools/perf/util/parse-events.c | 16 ++++++++--------
 tools/perf/util/parse-events.h |  6 ++++++
 2 files changed, 14 insertions(+), 8 deletions(-)

index 80a50fd..78032d8 100644
@@ -25,12 +25,6 @@
 extern int parse_events_debug;
 #endif
 int parse_events_parse(void *data, void *scanner);
-int parse_events_term__num(struct parse_events_term **term,
-			   int type_term, char *config, u64 num,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val);
-int parse_events_term__str(struct parse_events_term **term,
-			   int type_term, char *config, char *str,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val);
 
 static struct perf_pmu_event_symbol *perf_pmu_events_list;
 /*

@@ -1601,8 +1595,11 @@ static int new_term(struct parse_events_term **_term, int type_val,
 
 int parse_events_term__num(struct parse_events_term **term,
 			   int type_term, char *config, u64 num,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val)
+			   void *loc_term_, void *loc_val_)
 {
+	YYLTYPE *loc_term = loc_term_;
+	YYLTYPE *loc_val = loc_val_;
+
 	return new_term(term, PARSE_EVENTS__TERM_TYPE_NUM, type_term,
 			config, NULL, num,
 			loc_term ? loc_term->first_column : 0,

@@ -1611,8 +1608,11 @@ int parse_events_term__num(struct parse_events_term **term,
 
 int parse_events_term__str(struct parse_events_term **term,
 			   int type_term, char *config, char *str,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val)
+			   void *loc_term_, void *loc_val_)
 {
+	YYLTYPE *loc_term = loc_term_;
+	YYLTYPE *loc_val = loc_val_;
+
 	return new_term(term, PARSE_EVENTS__TERM_TYPE_STR, type_term,
 			config, str, 0,
 			loc_term ? loc_term->first_column : 0,

index e236f1b..131f29b 100644
@@ -98,6 +98,12 @@ struct parse_events_terms {
 };
 
 int parse_events__is_hardcoded_term(struct parse_events_term *term);
+int parse_events_term__num(struct parse_events_term **term,
+			   int type_term, char *config, u64 num,
+			   void *loc_term, void *loc_val);
+int parse_events_term__str(struct parse_events_term **term,
+			   int type_term, char *config, char *str,
+			   void *loc_term, void *loc_val);
 int parse_events_term__sym_hw(struct parse_events_term **term,
 			      char *config, unsigned idx);
 int parse_events_term__clone(struct parse_events_term **new,

1.9.1

The libunwind feature would never detect because of the following error:

$ cat tools/build/feature/test-libunwind.make.output
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_stream_buffer_decode'
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_uncompressed_size'
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_end'
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_buffer_decode'
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_stream_footer_decode'
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_size'
collect2: error: ld returned 1 exit status

Fix by adding -llzma and re-ordering to match the dependencies.

---
 tools/perf/config/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1b957a1..e3b3724 100644
@@ -32,7 +32,7 @@ ifeq ($(ARCH),x86)
     LIBUNWIND_LIBS = -lunwind -lunwind-x86_64
     $(call detected,CONFIG_X86_64)
   else
-    LIBUNWIND_LIBS = -lunwind -lunwind-x86
+    LIBUNWIND_LIBS = -lunwind-x86 -llzma -lunwind
   endif
   NO_PERF_REGS := 0
 endif

1.9.1

Parse errors can be reported in struct parse_events_error but the pointer passed is optional and can be NULL. Ensure it is not NULL before dereferencing it.

---
 tools/perf/util/parse-events.c | 2 ++
 tools/perf/util/parse-events.y | 6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

index 78032d8..2a4d1ec 100644
@@ -1659,6 +1659,8 @@ void parse_events_evlist_error(struct parse_events_evlist *data,
 {
 	struct parse_events_error *err = data->error;
 
+	if (!err)
+		return;
 	err->idx = idx;
 	err->str = strdup(str);
 	WARN_ONCE(!err->str, "WARNING: failed to allocate error string");

index 3d11e00..591905a 100644
@@ -389,8 +389,10 @@ PE_NAME ':' PE_NAME
 	if (parse_events_add_tracepoint(list, &data->idx, $1, $3)) {
 		struct parse_events_error *error = data->error;
 
-		error->idx = @1.first_column;
-		error->str = strdup("unknown tracepoint");
+		if (error) {
+			error->idx = @1.first_column;
+			error->str = strdup("unknown tracepoint");
+		}
 		return -1;
 	}
 	$$ = list;

1.9.1

perf_session__peek_event() generally leverages there being a single mmap of the perf.data file, however on 32-bit platforms when there is more that 32MiB of data, then there are multiple mmaps, so perf_session__peek_event() reads from the file.

In that case a couple of bugs were exposed (note
how the seg. fault appears with >32M of data):

   $ perf record --per-thread -e intel_bts// ../rtit-tests/loopy 1000000
   [ perf record: Woken up 13 times to write data ]
   [ perf record: Captured and wrote 24.568 MB perf.data ]
   $ perf script > /dev/null
   $ perf record --per-thread -e intel_bts// ../rtit-tests/loopy 10000000
   [ perf record: Woken up 136 times to write data ]
   [ perf record: Captured and wrote 270.794 MB perf.data ]
   $ perf script > /dev/null
   Segmentation fault (core dumped)

The wrong address was being passed to the readn() function and the buffer size was not being checked.

---
 tools/perf/util/session.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index e722107..39fe09d 100644
@@ -1182,7 +1182,7 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
 		return -1;
 
 	if (lseek(fd, file_offset, SEEK_SET) == (off_t)-1 ||
-	    readn(fd, &buf, hdr_sz) != (ssize_t)hdr_sz)
+	    readn(fd, buf, hdr_sz) != (ssize_t)hdr_sz)
 		return -1;
 
 	event = (union perf_event *)buf;

@@ -1190,12 +1190,12 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
 	if (session->header.needs_swap)
 		perf_event_header__bswap(&event->header);
 
-	if (event->header.size < hdr_sz)
+	if (event->header.size < hdr_sz || event->header.size > buf_sz)
 		return -1;
 
 	rest = event->header.size - hdr_sz;
 
-	if (readn(fd, &buf, rest) != (ssize_t)rest)
+	if (readn(fd, buf, rest) != (ssize_t)rest)
 		return -1;
 
 	if (session->header.needs_swap)

1.9.1

Patch "perf tools: Protect dso cache fd with a mutex" changed data_file_size() to open the data file if it was not open.

data_read_offset() was calling data_file_size() to read the data file size, but data_file_size() can fail to open the file because the binary_type has not been set up.

The correct function to call is dso__data_size() which uses dso__data_fd() to open the file correctly.

---
 tools/perf/util/dso.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1b96c8d..e248f56 100644
@@ -756,7 +756,7 @@ off_t dso__data_size(struct dso *dso, struct machine *machine)
 static ssize_t data_read_offset(struct dso *dso, struct machine *machine,
 				u64 offset, u8 *data, ssize_t size)
 {
-	if (data_file_size(dso, machine))
+	if (dso__data_size(dso, machine) < 0)
 		return -1;
 
 	/* Check the offset sanity. */

1.9.1

Hi Adrian,

Right, but I worried about the locking overhead. By using dso__data_size() it'll call dso__data_fd() everytime which grabs the dso__data_open_lock that is a global mutex. It can be a performance bottleneck on multi-thread report IMHO.

I assumed that correct code should check the data fd or size before calling read function as I read the comment in dso.h file.

What about adding the missing check in a proper place instead?

Thanks,
Namhyung

It looks to me like you need to change data_file_size() and dso_cache__read() so that they open the fd properly i.e. call dso__data_fd() instead of open_dso().

dso__data_fd() should not be taking a lock because the lock must be held while the fd in being used.

Perhaps there should be dso__get_fd() and dso__put_fd() that lock/open and the unlock.

But I will have to leave this to you because I am snowed with my own problems.

OK, I'll change it to have a proper binary type before calling open_dso() like dso__data_fd() does. But I still think it's good to open data fd explicitly before reading from dso cache.

That is already a part of my patchset but not merged yet.

No problem. I can see you're doing hardwork ;-)

Thanks,
Namhyung

Em Wed, May 20, 2015 at 09:44:37AM +0900, Namhyung Kim escreveu:

Ok, I have Adrian's patch in my perf/core branch, tomorrow I'll check what you guys ended up agreeing on so that I can do the adjustments before pushing to Ingo, ok?

:-)

- Arnaldo

Em Tue, May 19, 2015 at 04:05:41PM +0300, Adrian Hunter escreveu:

All looks reasonable, applied to my local tree.

Would be good to get Acks from Namhyung on the mutex one and from Jiri on the parse events stuff, probably the data_readd_offset() as well (the mutex one).

- Arnaldo