perf tools: Preparation for call graph from Intel BTS

Events like sched_switch do not provide a pid (tgid) which can result in threads with an unknown pid. If the pid is later discovered, join the map groups. Note the thread's map groups should be empty because they are populated by MMAP events which do provide the pid and tid.

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/machine.c | 22 ++++++++++++++++------
 tools/perf/util/map.c     | 14 ++++++++++++++
 tools/perf/util/map.h     |  1 +
 tools/perf/util/thread.c  | 35 +++++++++++++++++++++++++++++++++++
 tools/perf/util/thread.h  |  1 +
 5 files changed, 67 insertions(+), 6 deletions(-)

tools/perf/util/machine.c +1 -0

index 5b80877..0fa93c1 100644
@@ -272,6 +272,17 @@ void machines__set_id_hdr_size(struct machines *machines, u16 id_hdr_size)
 	return;
 }
 
+static void machine__update_thread_pid(struct machine *machine,

tools/perf/util/machine.c +5 -6

 static struct thread *__machine__findnew_thread(struct machine *machine,
 						pid_t pid, pid_t tid,
 						bool create)

@@ -285,10 +296,10 @@ static struct thread *__machine__findnew_thread(struct machine *machine,
 	 * so most of the time we dont have to look up
 	 * the full rbtree:
 	 */
-	if (machine->last_match && machine->last_match->tid == tid) {
-		if (pid != -1 && pid != machine->last_match->pid_)
-			machine->last_match->pid_ = pid;
-		return machine->last_match;
+	th = machine->last_match;
+	if (th && th->tid == tid) {
+		machine__update_thread_pid(machine, th, pid);
+		return th;
 	}
 
 	while (*p != NULL) {

@@ -297,8 +308,7 @@ static struct thread *__machine__findnew_thread(struct machine *machine,
 
 		if (th->tid == tid) {
 			machine->last_match = th;
-			if (pid != -1 && pid != th->pid_)
-				th->pid_ = pid;
+			machine__update_thread_pid(machine, th, pid);
 			return th;
 		}
 

tools/perf/util/map.c +14 -0

index 25c571f..7af1480 100644
@@ -454,6 +454,20 @@ void map_groups__exit(struct map_groups *mg)
 	}
 }
 
+bool map_groups__empty(struct map_groups *mg)
+{
+	int i;
+
+	for (i = 0; i < MAP__NR_TYPES; ++i) {
+		if (maps__first(&mg->maps[i]))
+			return false;
+		if (!list_empty(&mg->removed_maps[i]))
+			return false;
+	}
+
+	return true;
+}
+
 struct map_groups *map_groups__new(void)
 {
 	struct map_groups *mg = malloc(sizeof(*mg));

tools/perf/util/map.h +1 -0

index 7758c72..5806a90 100644
@@ -66,6 +66,7 @@ struct map_groups {
 
 struct map_groups *map_groups__new(void);
 void map_groups__delete(struct map_groups *mg);
+bool map_groups__empty(struct map_groups *mg);
 
 static inline struct map_groups *map_groups__get(struct map_groups *mg)
 {

tools/perf/util/thread.c +17 -0

index 7a32f44..ca94295 100644
@@ -24,6 +24,41 @@ int thread__init_map_groups(struct thread *thread, struct machine *machine)
 	return thread->mg ? 0 : -1;
 }
 
+int thread__join_map_groups(struct thread *thread, struct machine *machine)
+{
+	struct thread *leader;
+	pid_t pid = thread->pid_;
+
+	if (pid == thread->tid)
+		return 0;
+
+	leader = machine__findnew_thread(machine, pid, pid);
+	if (!leader)
+		return -ENOMEM;
+
+	if (!leader->mg)
+		leader->mg = map_groups__new();
+
+	if (!leader->mg)
+		return -ENOMEM;

tools/perf/util/thread.c +8 -0

+		map_groups__delete(thread->mg);
+	}
+
+	thread->mg = map_groups__get(leader->mg);
+
+	return 0;
+}
+
 struct thread *thread__new(pid_t pid, pid_t tid)
 {
 	char *comm_str;

tools/perf/util/thread.h +1 -0

index 3c0c272..9de0629 100644
@@ -31,6 +31,7 @@ struct comm;
 
 struct thread *thread__new(pid_t pid, pid_t tid);
 int thread__init_map_groups(struct thread *thread, struct machine *machine);
+int thread__join_map_groups(struct thread *thread, struct machine *machine);
 void thread__delete(struct thread *thread);
 static inline void thread__exited(struct thread *thread)
 {

Signature · 1 line

1.8.3.2

Em Mon, Jul 14, 2014 at 01:02:26PM +0300, Adrian Hunter escreveu:

Quoted 25 lines

Events like sched_switch do not provide a pid (tgid) which can result in threads with an unknown pid. If the pid is later discovered, join the map groups. Note the thread's map groups should be empty because they are populated by MMAP events which do provide the pid and tid.

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/machine.c | 22 ++++++++++++++++------
 tools/perf/util/map.c     | 14 ++++++++++++++
 tools/perf/util/map.h     |  1 +
 tools/perf/util/thread.c  | 35 +++++++++++++++++++++++++++++++++++
 tools/perf/util/thread.h  |  1 +
 5 files changed, 67 insertions(+), 6 deletions(-)

tools/perf/util/machine.c +1 -0

index 5b80877..0fa93c1 100644
@@ -272,6 +272,17 @@ void machines__set_id_hdr_size(struct machines *machines, u16 id_hdr_size)
 	return;
 }
 
+static void machine__update_thread_pid(struct machine *machine,

tools/perf/util/machine.c void machines__set_id_hdr_size(struct machines *machines, u16 id_hdr_size) · line 285

Quoted 10 lines

tools/perf/util/machine.c +10 -0

+				       struct thread *th, pid_t pid)
+{
+	if (pid != th->pid_ && pid != -1 && th->pid_ == -1) {
+		th->pid_ = pid;
+		if (thread__join_map_groups(th, machine))
+			pr_err("Failed to join map groups for %d:%d\n",
+			       th->pid_, th->tid);
+	}
+}
+

I think you should combine the above function with thread__join_map_groups, and I think that the method belongs to the 'machine' class, see below for further comments on this...

Quoted 25 lines

tools/perf/util/thread.c +17 -0

index 7a32f44..ca94295 100644
@@ -24,6 +24,41 @@ int thread__init_map_groups(struct thread *thread, struct machine *machine)
 	return thread->mg ? 0 : -1;
 }
 
+int thread__join_map_groups(struct thread *thread, struct machine *machine)
+{
+	struct thread *leader;
+	pid_t pid = thread->pid_;
+
+	if (pid == thread->tid)
+		return 0;
+
+	leader = machine__findnew_thread(machine, pid, pid);
+	if (!leader)
+		return -ENOMEM;
+
+	if (!leader->mg)
+		leader->mg = map_groups__new();
+
+	if (!leader->mg)
+		return -ENOMEM;

tools/perf/util/thread.c int thread__init_map_groups(struct thread *thread, struct machine *machine) · line 53

Quoted 10 lines

tools/perf/util/thread.c +10 -0

+
+	if (thread->mg) {
+		/*
+		 * Maps are created from MMAP events which provide the pid and
+		 * tid.  Consequently there never should be any maps on a thread
+		 * with an unknown pid.  Just print an error if there are.
+		 */
+		if (!map_groups__empty(thread->mg))
+			pr_err("Discarding thread maps for %d:%d\n",
+			       thread->pid_, thread->tid);

Because there is nothing in this function (thread__join_map_groups), checking if it is operating on a thread that has an unknown pid.

So, please consider merging this and its sole caller, machine__update_thread_pid, so that the test is in the same function as the assumption that it is operating on a thread with an unknown pid.

Quoted 6 lines

Because there is nothing in this function (thread__join_map_groups), checking if it is operating on a thread that has an unknown pid.

So, please consider merging this and its sole caller, machine__update_thread_pid, so that the test is in the same function as the assumption that it is operating on a thread with an unknown pid.

You have it in your tree now. Do you want another patch to do that?

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/comm.c    |  7 +++++--
 tools/perf/util/comm.h    |  6 ++++--
 tools/perf/util/machine.c |  4 +++-
 tools/perf/util/thread.c  | 24 +++++++++++++++++++-----
 tools/perf/util/thread.h  | 10 +++++++++-
 5 files changed, 40 insertions(+), 11 deletions(-)

tools/perf/util/comm.c +3 -2

index f9e7776..5e1e80e 100644
@@ -74,7 +74,7 @@ static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root)
 	return new;
 }
 
-struct comm *comm__new(const char *str, u64 timestamp)
+struct comm *comm__new(const char *str, u64 timestamp, bool exec)
 {
 	struct comm *comm = zalloc(sizeof(*comm));
 

@@ -82,6 +82,7 @@ struct comm *comm__new(const char *str, u64 timestamp)
 		return NULL;
 
 	comm->start = timestamp;
+	comm->exec = exec;
 
 	comm->comm_str = comm_str__findnew(str, &comm_str_root);
 	if (!comm->comm_str) {

@@ -94,7 +95,7 @@ struct comm *comm__new(const char *str, u64 timestamp)
 	return comm;
 }
 
-int comm__override(struct comm *comm, const char *str, u64 timestamp)
+int comm__override(struct comm *comm, const char *str, u64 timestamp, bool exec)
 {
 	struct comm_str *new, *old = comm->comm_str;
 

tools/perf/util/comm.c +0 -0

 	return 0;
 }

tools/perf/util/comm.h +4 -2

index fac5bd5..51c10ab 100644
@@ -11,11 +11,13 @@ struct comm {
 	struct comm_str *comm_str;
 	u64 start;
 	struct list_head list;
+	bool exec;
 };
 
 void comm__free(struct comm *comm);
-struct comm *comm__new(const char *str, u64 timestamp);
+struct comm *comm__new(const char *str, u64 timestamp, bool exec);
 const char *comm__str(const struct comm *comm);
-int comm__override(struct comm *comm, const char *str, u64 timestamp);
+int comm__override(struct comm *comm, const char *str, u64 timestamp,
+		   bool exec);
 
 #endif  /* __PERF_COMM_H */

tools/perf/util/machine.c +3 -1

index 0fa93c1..2513204 100644
@@ -360,11 +360,13 @@ int machine__process_comm_event(struct machine *machine, union perf_event *event
 	struct thread *thread = machine__findnew_thread(machine,
 							event->comm.pid,
 							event->comm.tid);
+	bool exec = event->header.misc & PERF_RECORD_MISC_COMM_EXEC;
 
 	if (dump_trace)
 		perf_event__fprintf_comm(event, stdout);
 
-	if (thread == NULL || thread__set_comm(thread, event->comm.comm, sample->time)) {
+	if (thread == NULL ||
+	    __thread__set_comm(thread, event->comm.comm, sample->time, exec)) {
 		dump_printf("problem processing PERF_RECORD_COMM, skipping event.\n");
 		return -1;
 	}

tools/perf/util/thread.c +19 -5

index ca94295..149e417 100644
@@ -76,7 +76,7 @@ struct thread *thread__new(pid_t pid, pid_t tid)
 			goto err_thread;
 
 		snprintf(comm_str, 32, ":%d", tid);
-		comm = comm__new(comm_str, 0);
+		comm = comm__new(comm_str, 0, false);
 		free(comm_str);
 		if (!comm)
 			goto err_thread;

@@ -113,19 +113,33 @@ struct comm *thread__comm(const struct thread *thread)
 	return list_first_entry(&thread->comm_list, struct comm, list);
 }
 
+struct comm *thread__exec_comm(const struct thread *thread)
+{
+	struct comm *comm, *last = NULL;
+
+	list_for_each_entry(comm, &thread->comm_list, list) {
+		if (comm->exec)
+			return comm;
+		last = comm;
+	}
+
+	return last;
+}
+
 /* CHECKME: time should always be 0 if event aren't ordered */
-int thread__set_comm(struct thread *thread, const char *str, u64 timestamp)
+int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp,
+		       bool exec)
 {
 	struct comm *new, *curr = thread__comm(thread);
 	int err;
 
 	/* Override latest entry if it had no specific time coverage */
-	if (!curr->start) {
-		err = comm__override(curr, str, timestamp);
+	if (!curr->start && !curr->exec) {
+		err = comm__override(curr, str, timestamp, exec);
 		if (err)
 			return err;
 	} else {
-		new = comm__new(str, timestamp);
+		new = comm__new(str, timestamp, exec);
 		if (!new)
 			return -ENOMEM;
 		list_add(&new->list, &thread->comm_list);

tools/perf/util/thread.h +0 -0

index 9de0629..b4269af 100644
@@ -38,9 +38,17 @@ static inline void thread__exited(struct thread *thread)
 	thread->dead = true;
 }

tools/perf/util/thread.h +1 -0

 int thread__comm_len(struct thread *thread);
 struct comm *thread__comm(const struct thread *thread);
+struct comm *thread__exec_comm(const struct thread *thread);
 const char *thread__comm_str(const struct thread *thread);
 void thread__insert_map(struct thread *thread, struct map *map);
 int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp);

Signature · 1 line

1.8.3.2

Em Mon, Jul 14, 2014 at 01:02:29PM +0300, Adrian Hunter escreveu:

Quoted 39 lines

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/comm.c    |  7 +++++--
 tools/perf/util/comm.h    |  6 ++++--
 tools/perf/util/machine.c |  4 +++-
 tools/perf/util/thread.c  | 24 +++++++++++++++++++-----
 tools/perf/util/thread.h  | 10 +++++++++-
 5 files changed, 40 insertions(+), 11 deletions(-)

tools/perf/util/comm.c +3 -2

index f9e7776..5e1e80e 100644
@@ -74,7 +74,7 @@ static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root)
 	return new;
 }
 
-struct comm *comm__new(const char *str, u64 timestamp)
+struct comm *comm__new(const char *str, u64 timestamp, bool exec)
 {
 	struct comm *comm = zalloc(sizeof(*comm));
 

@@ -82,6 +82,7 @@ struct comm *comm__new(const char *str, u64 timestamp)
 		return NULL;
 
 	comm->start = timestamp;
+	comm->exec = exec;
 
 	comm->comm_str = comm_str__findnew(str, &comm_str_root);
 	if (!comm->comm_str) {

@@ -94,7 +95,7 @@ struct comm *comm__new(const char *str, u64 timestamp)
 	return comm;
 }
 
-int comm__override(struct comm *comm, const char *str, u64 timestamp)
+int comm__override(struct comm *comm, const char *str, u64 timestamp, bool exec)
 {
 	struct comm_str *new, *old = comm->comm_str;
 

tools/perf/util/comm.c int comm__override(struct comm *comm, const char *str, u64 timestamp) · line 111

Quoted 6 lines

tools/perf/util/comm.c +2 -0

@@ -106,6 +107,8 @@ int comm__override(struct comm *comm, const char *str, u64 timestamp)
 	comm_str__put(old);
 	comm->comm_str = new;
 	comm->start = timestamp;
+	if (exec && !comm->exec)
+		comm->exec = true;

Why do you need the !comm->exec test?

Quoted 100 lines

tools/perf/util/comm.c +0 -0

 	return 0;
 }

tools/perf/util/comm.h +4 -2

index fac5bd5..51c10ab 100644
@@ -11,11 +11,13 @@ struct comm {
 	struct comm_str *comm_str;
 	u64 start;
 	struct list_head list;
+	bool exec;
 };
 
 void comm__free(struct comm *comm);
-struct comm *comm__new(const char *str, u64 timestamp);
+struct comm *comm__new(const char *str, u64 timestamp, bool exec);
 const char *comm__str(const struct comm *comm);
-int comm__override(struct comm *comm, const char *str, u64 timestamp);
+int comm__override(struct comm *comm, const char *str, u64 timestamp,
+		   bool exec);
 
 #endif  /* __PERF_COMM_H */

tools/perf/util/machine.c +3 -1

index 0fa93c1..2513204 100644
@@ -360,11 +360,13 @@ int machine__process_comm_event(struct machine *machine, union perf_event *event
 	struct thread *thread = machine__findnew_thread(machine,
 							event->comm.pid,
 							event->comm.tid);
+	bool exec = event->header.misc & PERF_RECORD_MISC_COMM_EXEC;
 
 	if (dump_trace)
 		perf_event__fprintf_comm(event, stdout);
 
-	if (thread == NULL || thread__set_comm(thread, event->comm.comm, sample->time)) {
+	if (thread == NULL ||
+	    __thread__set_comm(thread, event->comm.comm, sample->time, exec)) {
 		dump_printf("problem processing PERF_RECORD_COMM, skipping event.\n");
 		return -1;
 	}

tools/perf/util/thread.c +19 -5

index ca94295..149e417 100644
@@ -76,7 +76,7 @@ struct thread *thread__new(pid_t pid, pid_t tid)
 			goto err_thread;
 
 		snprintf(comm_str, 32, ":%d", tid);
-		comm = comm__new(comm_str, 0);
+		comm = comm__new(comm_str, 0, false);
 		free(comm_str);
 		if (!comm)
 			goto err_thread;

@@ -113,19 +113,33 @@ struct comm *thread__comm(const struct thread *thread)
 	return list_first_entry(&thread->comm_list, struct comm, list);
 }
 
+struct comm *thread__exec_comm(const struct thread *thread)
+{
+	struct comm *comm, *last = NULL;
+
+	list_for_each_entry(comm, &thread->comm_list, list) {
+		if (comm->exec)
+			return comm;
+		last = comm;
+	}
+
+	return last;
+}
+
 /* CHECKME: time should always be 0 if event aren't ordered */
-int thread__set_comm(struct thread *thread, const char *str, u64 timestamp)
+int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp,
+		       bool exec)
 {
 	struct comm *new, *curr = thread__comm(thread);
 	int err;
 
 	/* Override latest entry if it had no specific time coverage */
-	if (!curr->start) {
-		err = comm__override(curr, str, timestamp);
+	if (!curr->start && !curr->exec) {
+		err = comm__override(curr, str, timestamp, exec);
 		if (err)
 			return err;
 	} else {
-		new = comm__new(str, timestamp);
+		new = comm__new(str, timestamp, exec);
 		if (!new)
 			return -ENOMEM;
 		list_add(&new->list, &thread->comm_list);

tools/perf/util/thread.h +0 -0

index 9de0629..b4269af 100644
@@ -38,9 +38,17 @@ static inline void thread__exited(struct thread *thread)
 	thread->dead = true;
 }

tools/perf/util/thread.h static inline void thread__exited(struct thread *thread) · line 48

Quoted 10 lines

tools/perf/util/thread.h +8 -1

 
-int thread__set_comm(struct thread *thread, const char *comm, u64 timestamp);
+int __thread__set_comm(struct thread *thread, const char *comm, u64 timestamp,
+		       bool exec);
+static inline int thread__set_comm(struct thread *thread, const char *comm,
+				   u64 timestamp)
+{
+	return __thread__set_comm(thread, comm, timestamp, false);
+}
+

So this is nice, you leave the existing function, setting exec to false, so that you don't have to change the existing codepaths where it is not from 'exec', and provide a __ prefixed variant where 'exec' can be set.

Why not to do the same thing for comm__new() ? No uses, i.e. in all cases you will need to pass 'exec' as a variable?

I thought about doing the same thing for that machine__findnew_thread, i.e. provide a variant where tid and pid is passed and one where just the tid is passed, because the pid is unknown.

BTW, I'll update my perf/core branch with the set of patches that I merged, so that you can have where to look for a consolidated tree with the things already processed.

Quoted 8 lines

tools/perf/util/comm.c int comm__override(struct comm *comm, const char *str, u64 timestamp) · line 111

Quoted 6 lines

tools/perf/util/comm.c +2 -0

@@ -106,6 +107,8 @@ int comm__override(struct comm *comm, const char *str, u64 timestamp)
 	comm_str__put(old);
 	comm->comm_str = new;
 	comm->start = timestamp;
+	if (exec && !comm->exec)
+		comm->exec = true;

Why do you need the !comm->exec test?

Dunno

Em Tue, Jul 15, 2014 at 02:43:08PM +0300, Adrian Hunter escreveu:

Quoted 8 lines

Quoted 8 lines

tools/perf/util/comm.c int comm__override(struct comm *comm, const char *str, u64 timestamp) · line 111

Quoted 6 lines

tools/perf/util/comm.c +2 -0

@@ -106,6 +107,8 @@ int comm__override(struct comm *comm, const char *str, u64 timestamp)
 	comm_str__put(old);
 	comm->comm_str = new;
 	comm->start = timestamp;
+	if (exec && !comm->exec)
+		comm->exec = true;

Why do you need the !comm->exec test?

Quoted 1 line

Dunno

I saw that you kept it in the last patchkit submitted, I'll just drop it, needless obfuscation.

Em Wed, Jul 23, 2014 at 11:07:52AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 14 lines

Em Tue, Jul 15, 2014 at 02:43:08PM +0300, Adrian Hunter escreveu:

Quoted 10 lines

Quoted 8 lines

tools/perf/util/comm.c int comm__override(struct comm *comm, const char *str, u64 timestamp) · line 111

Quoted 6 lines

tools/perf/util/comm.c +2 -0

@@ -106,6 +107,8 @@ int comm__override(struct comm *comm, const char *str, u64 timestamp)
  	comm_str__put(old);
  	comm->comm_str = new;
  	comm->start = timestamp;
+	if (exec && !comm->exec)
+		comm->exec = true;

Why do you need the !comm->exec test?

Dunno

I saw that you kept it in the last patchkit submitted, I'll just drop it, needless obfuscation.

Also why do you need to "identify which comms are from exec", was it good for?

Quoted 15 lines

Em Wed, Jul 23, 2014 at 11:07:52AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 14 lines

Em Tue, Jul 15, 2014 at 02:43:08PM +0300, Adrian Hunter escreveu:

Quoted 10 lines

Quoted 8 lines

tools/perf/util/comm.c int comm__override(struct comm *comm, const char *str, u64 timestamp) · line 111

Quoted 6 lines

tools/perf/util/comm.c +2 -0

@@ -106,6 +107,8 @@ int comm__override(struct comm *comm, const char *str, u64 timestamp)
 	comm_str__put(old);
 	comm->comm_str = new;
 	comm->start = timestamp;
+	if (exec && !comm->exec)
+		comm->exec = true;

Why do you need the !comm->exec test?

Dunno

I saw that you kept it in the last patchkit submitted, I'll just drop it, needless obfuscation.

Sorry I forgot.

Quoted 2 lines

Also why do you need to "identify which comms are from exec", was it good for?

It is for grouping together all the data from a single execution, which is needed for pairing calls and returns e.g. any outstanding calls when a process exec's will never return.

label symbols are missing because elf_sec__is_a() fails to find the section because the section strings do not match the section headers because the sections headers are from the 'runtime' object and the sections strings are from the 'symbol source' object.

Fix by getting the section strings from the 'runtime' object so that they match the section headers.

Signed-off-by Adrian Hunter <[email protected]>

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

tools/perf/util/symbol-elf.c +1 -1

index 6864661..8c96817 100644
@@ -732,7 +732,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 	if (symstrs == NULL)
 		goto out_elf_end;
 
-	sec_strndx = elf_getscn(elf, ehdr.e_shstrndx);
+	sec_strndx = elf_getscn(runtime_ss->elf, runtime_ss->ehdr.e_shstrndx);
 	if (sec_strndx == NULL)
 		goto out_elf_end;
 

Signature · 1 line

1.8.3.2

Em Mon, Jul 14, 2014 at 01:02:32PM +0300, Adrian Hunter escreveu:

Quoted 8 lines

label symbols are missing because elf_sec__is_a() fails to find the section because the section strings do not match the section headers because the sections headers are from the 'runtime' object and the sections strings are from the 'symbol source' object.

Fix by getting the section strings from the 'runtime' object so that they match the section headers.

Would be great to have a 'perf test' entry that fail when looking for those 'label symbols', i.e. looking for one such symbol in vmlinux that we know is always there, in some written in stone area of the kernel, where this label vanishing is unlikely to happen.

Can you elaborate on the case that fails? So that I can test it here, and then after the fix, check that it indeed fixes the problem? I may well write that test myself and insert it in my branch just before this one.

- Arnaldo

Quoted 20 lines

Signed-off-by Adrian Hunter <[email protected]>

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

tools/perf/util/symbol-elf.c +1 -1

index 6864661..8c96817 100644
@@ -732,7 +732,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 	if (symstrs == NULL)
 		goto out_elf_end;
 
-	sec_strndx = elf_getscn(elf, ehdr.e_shstrndx);
+	sec_strndx = elf_getscn(runtime_ss->elf, runtime_ss->ehdr.e_shstrndx);
 	if (sec_strndx == NULL)
 		goto out_elf_end;
 

Signature · 1 line

1.8.3.2

__machine__findnew_thread() creates a 'struct thread' but does not free it on the error path.

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/machine.c | 4 +++-
 tools/perf/util/thread.c  | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

tools/perf/util/machine.c +3 -1

index c1c5ca3..18eaf31 100644
@@ -349,8 +349,10 @@ static struct thread *__machine__findnew_thread(struct machine *machine,
 		 * within thread__init_map_groups to find the thread
 		 * leader and that would screwed the rb tree.
 		 */
-		if (thread__init_map_groups(th, machine))
+		if (thread__init_map_groups(th, machine)) {
+			thread__delete(th);
 			return NULL;
+		}
 	}
 
 	return th;

tools/perf/util/thread.c +4 -2

index 149e417..b9a3ee4 100644
@@ -95,8 +95,10 @@ void thread__delete(struct thread *thread)
 {
 	struct comm *comm, *tmp;
 
-	map_groups__put(thread->mg);
-	thread->mg = NULL;
+	if (thread->mg) {
+		map_groups__put(thread->mg);
+		thread->mg = NULL;
+	}
 	list_for_each_entry_safe(comm, tmp, &thread->comm_list, list) {
 		list_del(&comm->list);
 		comm__free(comm);

Signature · 1 line

1.8.3.2

__machine__findnew_thread() creates a 'struct thread' but does not free it on the error path.

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/machine.c | 4 +++-
 tools/perf/util/thread.c  | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

tools/perf/util/machine.c +3 -1

index c1c5ca3..18eaf31 100644
@@ -349,8 +349,10 @@ static struct thread *__machine__findnew_thread(struct machine *machine,
 		 * within thread__init_map_groups to find the thread
 		 * leader and that would screwed the rb tree.
 		 */
-		if (thread__init_map_groups(th, machine))
+		if (thread__init_map_groups(th, machine)) {
+			thread__delete(th);
 			return NULL;
+		}
 	}
 
 	return th;

tools/perf/util/thread.c +0 -0

index 149e417..b9a3ee4 100644

Quoted 33 lines · Adrian Hunter

__machine__findnew_thread() creates a 'struct thread' but does not free it on the error path.

Signed-off-by Adrian Hunter <[email protected]>

---
  tools/perf/util/machine.c | 4 +++-
  tools/perf/util/thread.c  | 6 ++++--
  2 files changed, 7 insertions(+), 3 deletions(-)

tools/perf/util/machine.c +3 -1

index c1c5ca3..18eaf31 100644
@@ -349,8 +349,10 @@ static struct thread *__machine__findnew_thread(struct machine *machine,
  		 * within thread__init_map_groups to find the thread
  		 * leader and that would screwed the rb tree.
  		 */
-		if (thread__init_map_groups(th, machine))
+		if (thread__init_map_groups(th, machine)) {
+			thread__delete(th);
  			return NULL;
+		}
  	}
 
  	return th;

tools/perf/util/thread.c +0 -0

index 149e417..b9a3ee4 100644
@@ -95,8 +95,10 @@ void thread__delete(struct thread *thread)
  {
  	struct comm *comm, *tmp;
 

tools/perf/util/thread.c void thread__delete(struct thread *thread) · line 101

Quoted 6 lines

tools/perf/util/thread.c +4 -2

-	map_groups__put(thread->mg);
-	thread->mg = NULL;
+	if (thread->mg) {
+		map_groups__put(thread->mg);
+		thread->mg = NULL;
+	}

I dont think this is ^^^ needed.. each thread has ->mg defined

tools/perf/util/machine.c +12 -0

+
+int machine__get_kernel_start(struct machine *machine)
+{
+	struct map *map = machine__kernel_map(machine, MAP__FUNCTION);
+	int err = 0;
+
+	/*
+	 * The only addresses above 2^63 are kernel addresses of a 64-bit
+	 * kernel.  Note that addresses are unsigned so that on a 32-bit system
+	 * all addresses including kernel addresses are less than 2^32.  In
+	 * that case (32-bit system), if the kernel mapping is unknown, all
+	 * addresses will be assumed to be in user space - see

On Mon, Jul 14, 2014 at 01:02:37PM +0300, Adrian Hunter wrote:

SNIP

Quoted 17 lines

tools/perf/util/machine.c +17 -0

+
+int machine__get_kernel_start(struct machine *machine)
+{
+	struct map *map = machine__kernel_map(machine, MAP__FUNCTION);
+	int err = 0;
+
+	/*
+	 * The only addresses above 2^63 are kernel addresses of a 64-bit
+	 * kernel.  Note that addresses are unsigned so that on a 32-bit system
+	 * all addresses including kernel addresses are less than 2^32.  In
+	 * that case (32-bit system), if the kernel mapping is unknown, all
+	 * addresses will be assumed to be in user space - see
+	 * machine__kernel_ip().
+	 */
+	machine->kernel_start = 1ULL << 63;
+	if (map) {
+		err = map__load(map, machine->symbol_filter);

tools/perf/util/machine.c int __machine__synthesize_threads(struct machine *machine, struct perf_tool *too · line 1499

Quoted 5 lines

tools/perf/util/machine.c +5 -0

+		if (map->start)
+			machine->kernel_start = map->start;
+	}
+	return err;
+}

we just recently got machine__get_kernel_start_addr function, I think we want just one function for this

Quoted 21 lines · Jiri Olsa

On Mon, Jul 14, 2014 at 01:02:37PM +0300, Adrian Hunter wrote:

SNIP

Quoted 17 lines

tools/perf/util/machine.c +17 -0

+
+int machine__get_kernel_start(struct machine *machine)
+{
+	struct map *map = machine__kernel_map(machine, MAP__FUNCTION);
+	int err = 0;
+
+	/*
+	 * The only addresses above 2^63 are kernel addresses of a 64-bit
+	 * kernel.  Note that addresses are unsigned so that on a 32-bit system
+	 * all addresses including kernel addresses are less than 2^32.  In
+	 * that case (32-bit system), if the kernel mapping is unknown, all
+	 * addresses will be assumed to be in user space - see
+	 * machine__kernel_ip().
+	 */
+	machine->kernel_start = 1ULL << 63;
+	if (map) {
+		err = map__load(map, machine->symbol_filter);

Quoted 8 lines

tools/perf/util/machine.c int __machine__synthesize_threads(struct machine *machine, struct perf_tool *too · line 1499

Quoted 5 lines

tools/perf/util/machine.c +5 -0

+		if (map->start)
+			machine->kernel_start = map->start;
+	}
+	return err;
+}

we just recently got machine__get_kernel_start_addr function, I think we want just one function for this

They do quite different things.

Quoted 31 lines · Adrian Hunter

Quoted 29 lines · Jiri Olsa

On Mon, Jul 14, 2014 at 01:02:37PM +0300, Adrian Hunter wrote:

SNIP

Quoted 12 lines

tools/perf/util/machine.c +12 -0

+
+int machine__get_kernel_start(struct machine *machine)
+{
+	struct map *map = machine__kernel_map(machine, MAP__FUNCTION);
+	int err = 0;
+
+	/*
+	 * The only addresses above 2^63 are kernel addresses of a 64-bit
+	 * kernel.  Note that addresses are unsigned so that on a 32-bit system
+	 * all addresses including kernel addresses are less than 2^32.  In
+	 * that case (32-bit system), if the kernel mapping is unknown, all
+	 * addresses will be assumed to be in user space - see

tools/perf/util/machine.c int __machine__synthesize_threads(struct machine *machine, struct perf_tool *too · line 1499

Quoted 10 lines

tools/perf/util/machine.c +10 -0

+	 * machine__kernel_ip().
+	 */
+	machine->kernel_start = 1ULL << 63;
+	if (map) {
+		err = map__load(map, machine->symbol_filter);
+		if (map->start)
+			machine->kernel_start = map->start;
+	}
+	return err;
+}

we just recently got machine__get_kernel_start_addr function, I think we want just one function for this

Quoted 2 lines

They do quite different things.

so one of those two should have different name

Quoted 32 lines · Jiri Olsa

Quoted 31 lines · Adrian Hunter

Quoted 29 lines · Jiri Olsa

On Mon, Jul 14, 2014 at 01:02:37PM +0300, Adrian Hunter wrote:

SNIP

Quoted 12 lines

tools/perf/util/machine.c +12 -0

+
+int machine__get_kernel_start(struct machine *machine)
+{
+	struct map *map = machine__kernel_map(machine, MAP__FUNCTION);
+	int err = 0;
+
+	/*
+	 * The only addresses above 2^63 are kernel addresses of a 64-bit
+	 * kernel.  Note that addresses are unsigned so that on a 32-bit system
+	 * all addresses including kernel addresses are less than 2^32.  In
+	 * that case (32-bit system), if the kernel mapping is unknown, all
+	 * addresses will be assumed to be in user space - see

tools/perf/util/machine.c int __machine__synthesize_threads(struct machine *machine, struct perf_tool *too · line 1499

Quoted 10 lines

tools/perf/util/machine.c +10 -0

+	 * machine__kernel_ip().
+	 */
+	machine->kernel_start = 1ULL << 63;
+	if (map) {
+		err = map__load(map, machine->symbol_filter);
+		if (map->start)
+			machine->kernel_start = map->start;
+	}
+	return err;
+}

we just recently got machine__get_kernel_start_addr function, I think we want just one function for this

Quoted 4 lines

Quoted 2 lines

They do quite different things.

so one of those two should have different name

Such as?

Quoted 36 lines · Adrian Hunter

Quoted 35 lines · Jiri Olsa

Quoted 33 lines · Adrian Hunter

Quoted 21 lines · Jiri Olsa

On Mon, Jul 14, 2014 at 01:02:37PM +0300, Adrian Hunter wrote:

SNIP

Quoted 17 lines

tools/perf/util/machine.c +17 -0

+
+int machine__get_kernel_start(struct machine *machine)
+{
+	struct map *map = machine__kernel_map(machine, MAP__FUNCTION);
+	int err = 0;
+
+	/*
+	 * The only addresses above 2^63 are kernel addresses of a 64-bit
+	 * kernel.  Note that addresses are unsigned so that on a 32-bit system
+	 * all addresses including kernel addresses are less than 2^32.  In
+	 * that case (32-bit system), if the kernel mapping is unknown, all
+	 * addresses will be assumed to be in user space - see
+	 * machine__kernel_ip().
+	 */
+	machine->kernel_start = 1ULL << 63;
+	if (map) {
+		err = map__load(map, machine->symbol_filter);

Quoted 8 lines

tools/perf/util/machine.c int __machine__synthesize_threads(struct machine *machine, struct perf_tool *too · line 1499

Quoted 5 lines

tools/perf/util/machine.c +5 -0

+		if (map->start)
+			machine->kernel_start = map->start;
+	}
+	return err;
+}

we just recently got machine__get_kernel_start_addr function, I think we want just one function for this

They do quite different things.

Quoted 3 lines

Quoted 1 line

so one of those two should have different name

Such as?

dont know ;-) but if they do 'quite different things' as u said, I expect to see that at least in the function name

Quoted 39 lines · Jiri Olsa

Quoted 38 lines · Adrian Hunter

Quoted 36 lines · Jiri Olsa

Quoted 31 lines · Adrian Hunter

Quoted 21 lines · Jiri Olsa

On Mon, Jul 14, 2014 at 01:02:37PM +0300, Adrian Hunter wrote:

SNIP

Quoted 17 lines

(quoted diff) +17 -0

+
+int machine__get_kernel_start(struct machine *machine)
+{
+	struct map *map = machine__kernel_map(machine, MAP__FUNCTION);
+	int err = 0;
+
+	/*
+	 * The only addresses above 2^63 are kernel addresses of a 64-bit
+	 * kernel.  Note that addresses are unsigned so that on a 32-bit system
+	 * all addresses including kernel addresses are less than 2^32.  In
+	 * that case (32-bit system), if the kernel mapping is unknown, all
+	 * addresses will be assumed to be in user space - see
+	 * machine__kernel_ip().
+	 */
+	machine->kernel_start = 1ULL << 63;
+	if (map) {
+		err = map__load(map, machine->symbol_filter);

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+		if (map->start)
+			machine->kernel_start = map->start;
+	}
+	return err;
+}

we just recently got machine__get_kernel_start_addr function, I think we want just one function for this

Quoted 2 lines

They do quite different things.

so one of those two should have different name

Quoted 4 lines

Quoted 1 line

Such as?

dont know ;-) but if they do 'quite different things' as u said, I expect to see that at least in the function name

How about:

	machine__get_kernel_start_addr -> machine__get_running_kernel_start

Change the order of the output to put the srcline last. e.g. old format:

          4028fc main+0x2c (/bin/ls)

/build/buildd/coreutils-8.20/src/ls.c:1269 => 40d8a0 set_program_name+0x0 (/bin/ls)

new format:

          4028fc main+0x2c (/bin/ls) =>           40d8a0 set_program_name+0x0 (/bin/ls)

/build/buildd/coreutils-8.20/src/ls.c:1269

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/builtin-script.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

tools/perf/builtin-script.c +15 -5

index 9e9c91f..333b15e 100644
@@ -427,15 +427,22 @@ static void print_sample_bts(union perf_event *event,
 			     struct addr_location *al)
 {
 	struct perf_event_attr *attr = &evsel->attr;
+	bool print_srcline_last = false;
 
 	/* print branch_from information */
 	if (PRINT_FIELD(IP)) {
-		if (!symbol_conf.use_callchain)
-			printf(" ");
-		else
+		unsigned int print_opts = output[attr->type].print_ip_opts;
+
+		if (symbol_conf.use_callchain && sample->callchain) {
 			printf("\n");
-		perf_evsel__print_ip(evsel, sample, al,
-				     output[attr->type].print_ip_opts,
+		} else {
+			printf(" ");
+			if (print_opts & PRINT_IP_OPT_SRCLINE) {
+				print_srcline_last = true;
+				print_opts &= ~PRINT_IP_OPT_SRCLINE;
+			}
+		}
+		perf_evsel__print_ip(evsel, sample, al, print_opts,
 				     PERF_MAX_STACK_DEPTH);
 	}
 

@@ -447,6 +454,9 @@ static void print_sample_bts(union perf_event *event,
 	     !output[attr->type].user_set))
 		print_sample_addr(event, sample, al->machine, thread, attr);
 
+	if (print_srcline_last)
+		map__fprintf_srcline(al->map, al->addr, "\n  ", stdout);
+
 	printf("\n");
 }
 

Signature · 1 line

1.8.3.2

Em Mon, Jul 14, 2014 at 01:02:39PM +0300, Adrian Hunter escreveu:

Quoted 10 lines

Change the order of the output to put the srcline last. e.g. old format:

           4028fc main+0x2c (/bin/ls)

/build/buildd/coreutils-8.20/src/ls.c:1269 => 40d8a0 set_program_name+0x0 (/bin/ls)

new format:

           4028fc main+0x2c (/bin/ls) =>           40d8a0 set_program_name+0x0 (/bin/ls)

/build/buildd/coreutils-8.20/src/ls.c:1269

David,

Can I have your acked-by?

Adrian,

Can you elaborate on why this is an improvement?

- Arnaldo

Quoted 49 lines

Signed-off-by Adrian Hunter <[email protected]>

---
  tools/perf/builtin-script.c | 20 +++++++++++++++-----
  1 file changed, 15 insertions(+), 5 deletions(-)

tools/perf/builtin-script.c +15 -6

index 9e9c91f..333b15e 100644
@@ -427,15 +427,22 @@ static void print_sample_bts(union perf_event *event,
  			     struct addr_location *al)
  {
  	struct perf_event_attr *attr = &evsel->attr;
+	bool print_srcline_last = false;
 
  	/* print branch_from information */
  	if (PRINT_FIELD(IP)) {
-		if (!symbol_conf.use_callchain)
-			printf(" ");
-		else
+		unsigned int print_opts = output[attr->type].print_ip_opts;
+
+		if (symbol_conf.use_callchain && sample->callchain) {
  			printf("\n");
-		perf_evsel__print_ip(evsel, sample, al,
-				     output[attr->type].print_ip_opts,
+		} else {
+			printf(" ");
+			if (print_opts & PRINT_IP_OPT_SRCLINE) {
+				print_srcline_last = true;
+				print_opts &= ~PRINT_IP_OPT_SRCLINE;
+			}
+		}
+		perf_evsel__print_ip(evsel, sample, al, print_opts,
  				     PERF_MAX_STACK_DEPTH);
  	}
 

@@ -447,6 +454,9 @@ static void print_sample_bts(union perf_event *event,
  	     !output[attr->type].user_set))
  		print_sample_addr(event, sample, al->machine, thread, attr);
 
+	if (print_srcline_last)
+		map__fprintf_srcline(al->map, al->addr, "\n  ", stdout);
+
  	printf("\n");
  }
 
--

1.8.3.2