Add support for remote unwind

This is a preparation for cross-platform vdso lookup.

There is a naming confusion about vdso name, vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

This patch uses different names when storing the buildid, i.e. vdso64 for 64-bit machine and vdso32 for 32-bit machine, and eliminates this naming confusion.

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/util/vdso.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

tools/perf/util/vdso.h +6 -1

index cdc4fab..45e9ef4 100644
@@ -4,10 +4,15 @@
 #include <linux/types.h>
 #include <string.h>
 #include <stdbool.h>
+#include "util.h"
 
 #define VDSO__MAP_NAME "[vdso]"
 
-#define DSO__NAME_VDSO    "[vdso]"
+#if BITS_PER_LONG == 64
+#define DSO__NAME_VDSO    "[vdso64]"
+#else
+#define DSO__NAME_VDSO    "[vdso32]"
+#endif
 #define DSO__NAME_VDSO32  "[vdso32]"
 #define DSO__NAME_VDSOX32 "[vdsox32]"
 

There's a problem in machine__findnew_vdso(), vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

This patch tries to find the exsiting dsos in machine->dsos by thread dso_type. 64-bit thread tries to find vdso with name 'vdso', because all 64-bit vdso is named as that. 32-bit thread first tries to find vdso with name 'vdso32' if this thread was run on 64-bit machine, if failed, then it tries 'vdso' which indicates that the thread was run on 32-bit machine when recording.

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/util/vdso.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

tools/perf/util/vdso.c +37 -3

index 44d440d..99f4a3d 100644
@@ -134,8 +134,6 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
 	return dso;
 }
 
-#if BITS_PER_LONG == 64
-
 static enum dso_type machine__thread_dso_type(struct machine *machine,
 					      struct thread *thread)
 {

@@ -156,6 +154,8 @@ static enum dso_type machine__thread_dso_type(struct machine *machine,
 	return dso_type;
 }
 
+#if BITS_PER_LONG == 64
+
 static int vdso__do_copy_compat(FILE *f, int fd)
 {
 	char buf[4096];

@@ -283,8 +283,38 @@ static int __machine__findnew_vdso_compat(struct machine *machine,
 
 #endif
 
+static struct dso *machine__find_vdso(struct machine *machine,
+				      struct thread *thread)
+{
+	struct dso *dso = NULL;
+	enum dso_type dso_type;
+
+	dso_type = machine__thread_dso_type(machine, thread);
+	switch (dso_type) {
+	case DSO__TYPE_32BIT:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO32, true);
+		if (!dso)
+			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
+					   true);
+		break;
+	case DSO__TYPE_X32BIT:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSOX32, true);
+		if (!dso)
+			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
+					   true);
+		break;
+	case DSO__TYPE_64BIT:
+	case DSO__TYPE_UNKNOWN:
+	default:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
+		break;
+	}
+
+	return dso;
+}
+
 struct dso *machine__findnew_vdso(struct machine *machine,
-				  struct thread *thread __maybe_unused)
+				  struct thread *thread)
 {
 	struct vdso_info *vdso_info;
 	struct dso *dso = NULL;

@@ -297,6 +327,10 @@ struct dso *machine__findnew_vdso(struct machine *machine,
 	if (!vdso_info)
 		goto out_unlock;
 
+	dso = machine__find_vdso(machine, thread);
+	if (dso)
+		goto out_unlock;
+
 #if BITS_PER_LONG == 64
 	if (__machine__findnew_vdso_compat(machine, thread, vdso_info, &dso))
 		goto out_unlock;

Signature · 1 line

1.8.5.2

Em Fri, May 13, 2016 at 08:51:49AM +0000, He Kuang escreveu:

Quoted 5 lines

There's a problem in machine__findnew_vdso(), vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

Quoted 6 lines

This patch tries to find the exsiting dsos in machine->dsos by thread dso_type. 64-bit thread tries to find vdso with name 'vdso', because all 64-bit vdso is named as that. 32-bit thread first tries to find vdso with name 'vdso32' if this thread was run on 64-bit machine, if failed, then it tries 'vdso' which indicates that the thread was run on 32-bit machine when recording.

Adrian, are you ok now?

There's a problem in machine__findnew_vdso(), vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

This patch tries to find the exsiting dsos in machine->dsos by thread dso_type. 64-bit thread tries to find vdso with name 'vdso', because all 64-bit vdso is named as that. 32-bit thread first tries to find vdso with name 'vdso32' if this thread was run on 64-bit machine, if failed, then it tries 'vdso' which indicates that the thread was run on 32-bit machine when recording.

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/util/vdso.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

tools/perf/util/vdso.c +5 -2

index 44d440d..99f4a3d 100644
@@ -134,8 +134,6 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
 	return dso;
 }
 
-#if BITS_PER_LONG == 64
-
 static enum dso_type machine__thread_dso_type(struct machine *machine,
 					      struct thread *thread)
 {

@@ -156,6 +154,8 @@ static enum dso_type machine__thread_dso_type(struct machine *machine,
 	return dso_type;
 }
 
+#if BITS_PER_LONG == 64
+
 static int vdso__do_copy_compat(FILE *f, int fd)
 {
 	char buf[4096];

@@ -283,8 +283,38 @@ static int __machine__findnew_vdso_compat(struct machine *machine,
 
 #endif
 
+static struct dso *machine__find_vdso(struct machine *machine,
+				      struct thread *thread)
+{

tools/perf/util/vdso.c +16 -1

+		break;
+	case DSO__TYPE_64BIT:
+	case DSO__TYPE_UNKNOWN:
+	default:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
+		break;
+	}
+
+	return dso;
+}
+
 struct dso *machine__findnew_vdso(struct machine *machine,
-				  struct thread *thread __maybe_unused)
+				  struct thread *thread)
 {
 	struct vdso_info *vdso_info;
 	struct dso *dso = NULL;

@@ -297,6 +327,10 @@ struct dso *machine__findnew_vdso(struct machine *machine,
 	if (!vdso_info)
 		goto out_unlock;
 
+	dso = machine__find_vdso(machine, thread);
+	if (dso)
+		goto out_unlock;
+
 #if BITS_PER_LONG == 64
 	if (__machine__findnew_vdso_compat(machine, thread, vdso_info, &dso))
 		goto out_unlock;

There's a problem in machine__findnew_vdso(), vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

This patch tries to find the exsiting dsos in machine->dsos by thread dso_type. 64-bit thread tries to find vdso with name 'vdso', because all 64-bit vdso is named as that. 32-bit thread first tries to find vdso with name 'vdso32' if this thread was run on 64-bit machine, if failed, then it tries 'vdso' which indicates that the thread was run on 32-bit machine when recording.

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/util/vdso.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

tools/perf/util/vdso.c +37 -3

index 44d440d..8f81c41 100644
@@ -134,8 +134,6 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
 	return dso;
 }
 
-#if BITS_PER_LONG == 64
-
 static enum dso_type machine__thread_dso_type(struct machine *machine,
 					      struct thread *thread)
 {

@@ -156,6 +154,8 @@ static enum dso_type machine__thread_dso_type(struct machine *machine,
 	return dso_type;
 }
 
+#if BITS_PER_LONG == 64
+
 static int vdso__do_copy_compat(FILE *f, int fd)
 {
 	char buf[4096];

@@ -283,8 +283,38 @@ static int __machine__findnew_vdso_compat(struct machine *machine,
 
 #endif
 
+static struct dso *machine__find_vdso(struct machine *machine,
+				      struct thread *thread)
+{
+	struct dso *dso = NULL;
+	enum dso_type dso_type;
+
+	dso_type = machine__thread_dso_type(machine, thread);
+	switch (dso_type) {
+	case DSO__TYPE_32BIT:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO32, true);
+		if (!dso) {
+			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
+					   true);
+			if (dso_type != dso__type(dso, machine))
+				dso = NULL;
+		}
+		break;
+	case DSO__TYPE_X32BIT:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSOX32, true);
+		break;
+	case DSO__TYPE_64BIT:
+	case DSO__TYPE_UNKNOWN:
+	default:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
+		break;
+	}
+
+	return dso;
+}
+
 struct dso *machine__findnew_vdso(struct machine *machine,
-				  struct thread *thread __maybe_unused)
+				  struct thread *thread)
 {
 	struct vdso_info *vdso_info;
 	struct dso *dso = NULL;

@@ -297,6 +327,10 @@ struct dso *machine__findnew_vdso(struct machine *machine,
 	if (!vdso_info)
 		goto out_unlock;
 
+	dso = machine__find_vdso(machine, thread);
+	if (dso)
+		goto out_unlock;
+
 #if BITS_PER_LONG == 64
 	if (__machine__findnew_vdso_compat(machine, thread, vdso_info, &dso))
 		goto out_unlock;

Quoted 11 lines

There's a problem in machine__findnew_vdso(), vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

This patch tries to find the exsiting dsos in machine->dsos by thread dso_type. 64-bit thread tries to find vdso with name 'vdso', because all 64-bit vdso is named as that. 32-bit thread first tries to find vdso with name 'vdso32' if this thread was run on 64-bit machine, if failed, then it tries 'vdso' which indicates that the thread was run on 32-bit machine when recording.

Quoted 1 line

Signed-off-by He Kuang <[email protected]>

Looks OK.

Acked-by Adrian Hunter <[email protected]>

There's a problem in machine__findnew_vdso(), vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

This patch tries to find the exsiting dsos in machine->dsos by thread dso_type. 64-bit thread tries to find vdso with name 'vdso', because all 64-bit vdso is named as that. 32-bit thread first tries to find vdso with name 'vdso32' if this thread was run on 64-bit machine, if failed, then it tries 'vdso' which indicates that the thread was run on 32-bit machine when recording.

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/util/vdso.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

tools/perf/util/vdso.c +37 -3

index 44d440d..8f81c41 100644
@@ -134,8 +134,6 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
 	return dso;
 }
 
-#if BITS_PER_LONG == 64
-
 static enum dso_type machine__thread_dso_type(struct machine *machine,
 					      struct thread *thread)
 {

@@ -156,6 +154,8 @@ static enum dso_type machine__thread_dso_type(struct machine *machine,
 	return dso_type;
 }
 
+#if BITS_PER_LONG == 64
+
 static int vdso__do_copy_compat(FILE *f, int fd)
 {
 	char buf[4096];

@@ -283,8 +283,38 @@ static int __machine__findnew_vdso_compat(struct machine *machine,
 
 #endif
 
+static struct dso *machine__find_vdso(struct machine *machine,
+				      struct thread *thread)
+{
+	struct dso *dso = NULL;
+	enum dso_type dso_type;
+
+	dso_type = machine__thread_dso_type(machine, thread);
+	switch (dso_type) {
+	case DSO__TYPE_32BIT:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO32, true);
+		if (!dso) {
+			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
+					   true);
+			if (dso_type != dso__type(dso, machine))
+				dso = NULL;
+		}
+		break;
+	case DSO__TYPE_X32BIT:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSOX32, true);
+		break;
+	case DSO__TYPE_64BIT:
+	case DSO__TYPE_UNKNOWN:
+	default:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
+		break;
+	}
+
+	return dso;
+}
+
 struct dso *machine__findnew_vdso(struct machine *machine,
-				  struct thread *thread __maybe_unused)
+				  struct thread *thread)
 {
 	struct vdso_info *vdso_info;
 	struct dso *dso = NULL;

@@ -297,6 +327,10 @@ struct dso *machine__findnew_vdso(struct machine *machine,
 	if (!vdso_info)
 		goto out_unlock;
 
+	dso = machine__find_vdso(machine, thread);
+	if (dso)
+		goto out_unlock;
+
 #if BITS_PER_LONG == 64
 	if (__machine__findnew_vdso_compat(machine, thread, vdso_info, &dso))
 		goto out_unlock;

Signature · 1 line

1.8.5.2

Em Tue, May 17, 2016 at 09:04:54AM +0000, He Kuang escreveu:

Quoted 4 lines

There's a problem in machine__findnew_vdso(), vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

Without looking at the code, just trying to understand your patch by reading your description: Why would we look for names if we have build-ids? All this type and name comparasions seems wrong if we have a build-id, no?

Adrian?

- Arnaldo

Quoted 87 lines

This patch tries to find the exsiting dsos in machine->dsos by thread dso_type. 64-bit thread tries to find vdso with name 'vdso', because all 64-bit vdso is named as that. 32-bit thread first tries to find vdso with name 'vdso32' if this thread was run on 64-bit machine, if failed, then it tries 'vdso' which indicates that the thread was run on 32-bit machine when recording.

Signed-off-by He Kuang <[email protected]>

---
  tools/perf/util/vdso.c | 40 +++++++++++++++++++++++++++++++++++++---
  1 file changed, 37 insertions(+), 3 deletions(-)

tools/perf/util/vdso.c +37 -3

index 44d440d..8f81c41 100644
@@ -134,8 +134,6 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
  	return dso;
  }
 
-#if BITS_PER_LONG == 64
-
  static enum dso_type machine__thread_dso_type(struct machine *machine,
  					      struct thread *thread)
  {

@@ -156,6 +154,8 @@ static enum dso_type machine__thread_dso_type(struct machine *machine,
  	return dso_type;
  }
 
+#if BITS_PER_LONG == 64
+
  static int vdso__do_copy_compat(FILE *f, int fd)
  {
  	char buf[4096];

@@ -283,8 +283,38 @@ static int __machine__findnew_vdso_compat(struct machine *machine,
 
  #endif
 
+static struct dso *machine__find_vdso(struct machine *machine,
+				      struct thread *thread)
+{
+	struct dso *dso = NULL;
+	enum dso_type dso_type;
+
+	dso_type = machine__thread_dso_type(machine, thread);
+	switch (dso_type) {
+	case DSO__TYPE_32BIT:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO32, true);
+		if (!dso) {
+			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
+					   true);
+			if (dso_type != dso__type(dso, machine))
+				dso = NULL;
+		}
+		break;
+	case DSO__TYPE_X32BIT:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSOX32, true);
+		break;
+	case DSO__TYPE_64BIT:
+	case DSO__TYPE_UNKNOWN:
+	default:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
+		break;
+	}
+
+	return dso;
+}
+
  struct dso *machine__findnew_vdso(struct machine *machine,
-				  struct thread *thread __maybe_unused)
+				  struct thread *thread)
  {
  	struct vdso_info *vdso_info;
  	struct dso *dso = NULL;

@@ -297,6 +327,10 @@ struct dso *machine__findnew_vdso(struct machine *machine,
  	if (!vdso_info)
  		goto out_unlock;
 
+	dso = machine__find_vdso(machine, thread);
+	if (dso)
+		goto out_unlock;
+
  #if BITS_PER_LONG == 64
  	if (__machine__findnew_vdso_compat(machine, thread, vdso_info, &dso))
  		goto out_unlock;

Signature · 1 line

1.8.5.2

Em Tue, May 17, 2016 at 09:04:54AM +0000, He Kuang escreveu:

Quoted 4 lines

There's a problem in machine__findnew_vdso(), vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

Without looking at the code, just trying to understand your patch by reading your description: Why would we look for names if we have build-ids? All this type and name comparasions seems wrong if we have a build-id, no?

Adrian?

Quoted 6 lines

Em Tue, May 17, 2016 at 09:04:54AM +0000, He Kuang escreveu:

Quoted 4 lines

There's a problem in machine__findnew_vdso(), vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

Quoted 6 lines

Without looking at the code, just trying to understand your patch by reading your description: Why would we look for names if we have build-ids? All this type and name comparasions seems wrong if we have a build-id, no?

Adrian?

We match maps to builds ids using the file name - consider machine__findnew_[v]dso() called in map__new(). So in the context of a perf data file, we consider the file name to be unique.

A vdso map does not have a file name - all we know is that it is vdso. We look at the thread to tell if it is 32-bit, 64-bit or x32. Then we need to get the build id which has been recorded using short name "[vdso]" or "[vdso32]" or "[vdsox32]".

The problem is that on a 32-bit machine, we use the name "[vdso]". If you take a 32-bit perf data file to a 64-bit machine, it gets hard to figure out if "[vdso]" is 32-bit or 64-bit.

This patch solves that problem.

I acked it in another email.

Quoted 46 lines · He Kuang

There's a problem in machine__findnew_vdso(), vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

This patch tries to find the exsiting dsos in machine->dsos by thread dso_type. 64-bit thread tries to find vdso with name 'vdso', because all 64-bit vdso is named as that. 32-bit thread first tries to find vdso with name 'vdso32' if this thread was run on 64-bit machine, if failed, then it tries 'vdso' which indicates that the thread was run on 32-bit machine when recording.

Signed-off-by He Kuang <[email protected]>

---
  tools/perf/util/vdso.c | 40 +++++++++++++++++++++++++++++++++++++---
  1 file changed, 37 insertions(+), 3 deletions(-)

tools/perf/util/vdso.c +5 -2

index 44d440d..99f4a3d 100644
@@ -134,8 +134,6 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
  	return dso;
  }
 
-#if BITS_PER_LONG == 64
-
  static enum dso_type machine__thread_dso_type(struct machine *machine,
  					      struct thread *thread)
  {

@@ -156,6 +154,8 @@ static enum dso_type machine__thread_dso_type(struct machine *machine,
  	return dso_type;
  }
 
+#if BITS_PER_LONG == 64
+
  static int vdso__do_copy_compat(FILE *f, int fd)
  {
  	char buf[4096];

@@ -283,8 +283,38 @@ static int __machine__findnew_vdso_compat(struct machine *machine,
 
  #endif
 
+static struct dso *machine__find_vdso(struct machine *machine,
+				      struct thread *thread)
+{

tools/perf/util/vdso.c static int __machine__findnew_vdso_compat(struct machine *machine, · line 298

Quoted 10 lines

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

+	struct dso *dso = NULL;
+	enum dso_type dso_type;
+
+	dso_type = machine__thread_dso_type(machine, thread);
+	switch (dso_type) {
+	case DSO__TYPE_32BIT:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO32, true);
+		if (!dso)
+			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
+					   true);

So if we have not yet added the 32-bit vdso but have added the 64-bit vdso, we will return the wrong one.

Can we check it? e.g.

		if (!dso)
			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
					   true);
			if (dso_type != dso__type(dso, machine))
				dso = NULL;

tools/perf/util/vdso.c static int __machine__findnew_vdso_compat(struct machine *machine, · line 304

Quoted 6 lines

tools/perf/util/vdso.c +6 -0

+		break;
+	case DSO__TYPE_X32BIT:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSOX32, true);
+		if (!dso)
+			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
+					   true);

The x32 vdso is never called DSO__NAME_VDSO so this is not correct, but for the same reason we don't need this __dsos__find() anyway.

Quoted 29 lines

tools/perf/util/vdso.c +16 -1

+		break;
+	case DSO__TYPE_64BIT:
+	case DSO__TYPE_UNKNOWN:
+	default:
+		dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
+		break;
+	}
+
+	return dso;
+}
+
  struct dso *machine__findnew_vdso(struct machine *machine,
-				  struct thread *thread __maybe_unused)
+				  struct thread *thread)
  {
  	struct vdso_info *vdso_info;
  	struct dso *dso = NULL;

@@ -297,6 +327,10 @@ struct dso *machine__findnew_vdso(struct machine *machine,
  	if (!vdso_info)
  		goto out_unlock;
 
+	dso = machine__find_vdso(machine, thread);
+	if (dso)
+		goto out_unlock;
+
  #if BITS_PER_LONG == 64
  	if (__machine__findnew_vdso_compat(machine, thread, vdso_info, &dso))
  		goto out_unlock;

Quoted 7 lines · He Kuang

This is a preparation for cross-platform vdso lookup.

There is a naming confusion about vdso name, vdso buildid generated by a 32-bit machine stores it with the name 'vdso', but when processing buildid on a 64-bit machine with the same 'perf.data', perf will search for vdso named as 'vdso32' and get failed.

Quoted 3 lines

This patch uses different names when storing the buildid, i.e. vdso64 for 64-bit machine and vdso32 for 32-bit machine, and eliminates this naming confusion.

That looks like it will break existing perf.data files because they will have a different name recorded in the buildid section.

Also it doesn't look like it would work the other way around i.e. recording on a 64-bit machine and processing on a 32-bit machine.

Quoted 27 lines

Signed-off-by He Kuang <[email protected]>

---
  tools/perf/util/vdso.h | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

tools/perf/util/vdso.h +6 -1

index cdc4fab..45e9ef4 100644
@@ -4,10 +4,15 @@
  #include <linux/types.h>
  #include <string.h>
  #include <stdbool.h>
+#include "util.h"
 
  #define VDSO__MAP_NAME "[vdso]"
 
-#define DSO__NAME_VDSO    "[vdso]"
+#if BITS_PER_LONG == 64
+#define DSO__NAME_VDSO    "[vdso64]"
+#else
+#define DSO__NAME_VDSO    "[vdso32]"
+#endif
  #define DSO__NAME_VDSO32  "[vdso32]"
  #define DSO__NAME_VDSOX32 "[vdsox32]"
 

When unwinding callchains on a different machine, vdso info should be provided so the unwind process won't be interrupted if address falls into vdso region. But in most cases, the addresses of sample events are not in vdso range, the buildid of a zero hit vdso won't be stored into perf.data.

This patch stores vdso buildid regardless of whether the vdso is hit or not.

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/util/build-id.c | 2 +-
 tools/perf/util/dso.c      | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

tools/perf/util/build-id.c +1 -1

index 0573c2e..bdc7580 100644
@@ -256,7 +256,7 @@ static int machine__write_buildid_table(struct machine *machine, int fd)
 		size_t name_len;
 		bool in_kernel = false;
 
-		if (!pos->hit)
+		if (!pos->hit && !dso__is_vdso(pos))
 			continue;
 
 		if (dso__is_vdso(pos)) {

tools/perf/util/dso.c +2 -1

index 8e639543..b39b80c 100644
@@ -7,6 +7,7 @@
 #include "auxtrace.h"
 #include "util.h"
 #include "debug.h"
+#include "vdso.h"
 
 char dso__symtab_origin(const struct dso *dso)
 {

@@ -1169,7 +1170,7 @@ bool __dsos__read_build_ids(struct list_head *head, bool with_hits)
 	struct dso *pos;
 
 	list_for_each_entry(pos, head, node) {
-		if (with_hits && !pos->hit)
+		if (with_hits && !pos->hit && !dso__is_vdso(pos))
 			continue;
 		if (pos->has_build_id) {
 			have_build_id = true;

Signature · 1 line

1.8.5.2

Symfs dir and buildid dir are two places that perf looks into for symbols, currently, if symfs dir is given, buildid-cache is skipped.

In the cross-platform perf record/script scenario, we need vdsos in buildid-cache dir and other libs in symfs dir at the same time. And consider that the binaries indexed by buildid do not cause ambiguity, this patch simply removes that logical.

Signed-off-by He Kuang <[email protected]>

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

tools/perf/util/dso.c +1 -2

index b39b80c..a07166c5 100644
@@ -64,8 +64,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
 		break;
 	case DSO_BINARY_TYPE__BUILD_ID_CACHE:
 		/* skip the locally configured cache if a symfs is given */
-		if (symbol_conf.symfs[0] ||
-		    (dso__build_id_filename(dso, filename, size) == NULL))
+		if (dso__build_id_filename(dso, filename, size) == NULL)
 			ret = -1;
 		break;
 

Signature · 1 line

1.8.5.2

Em Thu, May 12, 2016 at 08:43:12AM +0000, He Kuang escreveu:

Quoted 7 lines

Symfs dir and buildid dir are two places that perf looks into for symbols, currently, if symfs dir is given, buildid-cache is skipped.

In the cross-platform perf record/script scenario, we need vdsos in buildid-cache dir and other libs in symfs dir at the same time. And consider that the binaries indexed by buildid do not cause ambiguity, this patch simply removes that logical.

Makes perfect sense, David, do you have any concern? Can I have your Acked-by?

Quoted 11 lines

Em Thu, May 12, 2016 at 08:43:12AM +0000, He Kuang escreveu:

Quoted 7 lines

Symfs dir and buildid dir are two places that perf looks into for symbols, currently, if symfs dir is given, buildid-cache is skipped.

In the cross-platform perf record/script scenario, we need vdsos in buildid-cache dir and other libs in symfs dir at the same time. And consider that the binaries indexed by buildid do not cause ambiguity, this patch simply removes that logical.

Makes perfect sense, David, do you have any concern? Can I have your Acked-by?

seems odd to me you want to look in the buildid-cache when a symfs is given. The point of symfs was "go look for everything under here."

I believe dso__load is going to hit DSO_BINARY_TYPE__BUILD_ID_CACHE before any of the others and there are probably cases where a stale cache entry would be hit before a build tree entry (e.g., symfs).

What about putting the build id cache under the symfs? so instead of dropping the symfs check and it to the path for the build id cache.

Quoted 9 lines · Arnaldo Carvalho de Melo

Em Thu, May 12, 2016 at 08:43:12AM +0000, He Kuang escreveu:

Quoted 7 lines

Symfs dir and buildid dir are two places that perf looks into for symbols, currently, if symfs dir is given, buildid-cache is skipped.

In the cross-platform perf record/script scenario, we need vdsos in buildid-cache dir and other libs in symfs dir at the same time. And consider that the binaries indexed by buildid do not cause ambiguity, this patch simply removes that logical.

Quoted 2 lines

Makes perfect sense, David, do you have any concern? Can I have your Acked-by?

seems odd to me you want to look in the buildid-cache when a symfs is given. The point of symfs was "go look for everything under here."

I believe dso__load is going to hit DSO_BINARY_TYPE__BUILD_ID_CACHE before any of the others and there are probably cases where a stale cache entry would be hit before a build tree entry (e.g., symfs).

What about putting the build id cache under the symfs? so instead of dropping the symfs check and it to the path for the build id cache.

Quoted 4 lines

What about putting the build id cache under the symfs? so instead of dropping the symfs check and it to the path for the build id cache.

I think your intention is to reference symbol files in one place instead of two. So there're two possible approaches, one is all in buildid-cache, but in practice, I found lots of binaries in symfs even not contains valid buildid, so this way is not work.

The other one is all in symfs. It seems ok, but one problem I should point out, with my test environment as an example, the symfsdir is $(TARGET_ROOTFS),and by default buildid_dir is $(TARGET_ROOTFS)/$(HOME)/.debug/, host perf does not know $(HOME) folder in target and we should copy the debug folder
to $(TARGET_ROOTFS), which is readonly in the target. For me, it's
easier to use 'buildid-cache -a vdso-xxxx' to add that into host buildid-cache than copy debug folder from $(HOME) to readonly $(TARGET_ROOTFS).

Without the stale concern, I prefer the two places(buildid-dir in host and target symfs) way.

Quoted 17 lines

Quoted 2 lines

What about putting the build id cache under the symfs? so instead of dropping the symfs check and it to the path for the build id cache.

I think your intention is to reference symbol files in one place instead of two. So there're two possible approaches, one is all in buildid-cache, but in practice, I found lots of binaries in symfs even not contains valid buildid, so this way is not work.

The other one is all in symfs. It seems ok, but one problem I should point out, with my test environment as an example, the symfsdir is $(TARGET_ROOTFS),and by default buildid_dir is $(TARGET_ROOTFS)/$(HOME)/.debug/, host perf does not know $(HOME) folder in target and we should copy the debug folder
to $(TARGET_ROOTFS), which is readonly in the target. For me, it's
easier to use 'buildid-cache -a vdso-xxxx' to add that into host buildid-cache than copy debug folder from $(HOME) to readonly $(TARGET_ROOTFS).

Quoted 2 lines

Without the stale concern, I prefer the two places(buildid-dir in host and target symfs) way.

The intention of symfs is every single file opened by perf is relative to that directory. As I recall when I added that option in early 2011 I made sure that statement is true. I think it is best to maintain that design.

In the cross-platform perf record/script scenario, we need vdsos in buildid-cache dir and other libs in symfs dir at the same time. For the reason that to have every single file opened by perf is relative to symfs dirctory, perf skips the buildid dir if symfs is given.

This patch references the buildid dir under symfs if '--symfs' is used, and adds new option '--dso-prefix' to specify the subdir path in symfs which contains the buildid dsos.

Quoted 5 lines · He Kuang

In the cross-platform perf record/script scenario, we need vdsos in buildid-cache dir and other libs in symfs dir at the same time. For the reason that to have every single file opened by perf is relative to symfs dirctory, perf skips the buildid dir if symfs is given.

Quoted 3 lines

This patch references the buildid dir under symfs if '--symfs' is used, and adds new option '--dso-prefix' to specify the subdir path in symfs which contains the buildid dsos.

In the previous version of this patch you just wanted to drop the symfs check. That means there is a path that perf searches for buildid files and reading it worked for you. Why is adding symsfs to that path not enough? ie., Why do you need to specify a different location under the symfs?

In previous patch, I use 'perf buildid-cache -a' to add vdso binary into the HOST buildid dir.

Quoted 2 lines · Hekuang

In previous patch, I use 'perf buildid-cache -a' to add vdso binary into the HOST buildid dir.

So 'perf buildid-cache' needs the symfs option?

Quoted 2 lines · Hekuang

In previous patch, I use 'perf buildid-cache -a' to add vdso binary into the HOST buildid dir.

So 'perf buildid-cache' needs the symfs option?

Quoted 7 lines · David Ahern

Quoted 2 lines · Hekuang

In previous patch, I use 'perf buildid-cache -a' to add vdso binary into the HOST buildid dir.

So 'perf buildid-cache' needs the symfs option?

With this patch 'PATCH v3 3/7 UPDATE', the tree of symfs dir is like this:

├── debug($(dso-prefix))
│   ├── .build-id
│   │   ├── 3a
│   │   │   └── e5ba6d4e532ad529e43ccf1ce1ddf8a64a4fdd ->
../../[kernel.kallsyms]/3ae5ba6d4e532ad529e43ccf1ce1ddf8a64a4fdd
│   │   └── 84
│   │       └── dbd75729adba57cc42f5544b25de571c0c8731 ->
../../[vdso32]/84dbd75729adba57cc42f5544b25de571c0c8731
│   ├── [kernel.kallsyms]
│   │   └── 3ae5ba6d4e532ad529e43ccf1ce1ddf8a64a4fdd
│   ├── [vdso]
│   │   └── 84dbd75729adba57cc42f5544b25de571c0c8731
│   └── [vdso32]
│       └── 84dbd75729adba57cc42f5544b25de571c0c8731
├── lib
│   ├── ld-2.22.so
│   └── libc-2.22.so
├── tmp
│   └── hello
└── xxx

So all binaries we need are included in the symfs dir. I think this is consistent with your idea explained in previous mails.

With this symfs, we do not need buildid dir anymore and what's your idea on 'perf buildid-cache' needs symfs option? after all, that only effects on buildid dir.

Quoted 34 lines · Hekuang

Quoted 7 lines · David Ahern

Quoted 2 lines · Hekuang

In previous patch, I use 'perf buildid-cache -a' to add vdso binary into the HOST buildid dir.

So 'perf buildid-cache' needs the symfs option?

With this patch 'PATCH v3 3/7 UPDATE', the tree of symfs dir is like this:

├── debug($(dso-prefix))
│   ├── .build-id
│   │   ├── 3a
│   │   │   └── e5ba6d4e532ad529e43ccf1ce1ddf8a64a4fdd ->
../../[kernel.kallsyms]/3ae5ba6d4e532ad529e43ccf1ce1ddf8a64a4fdd
│   │   └── 84
│   │       └── dbd75729adba57cc42f5544b25de571c0c8731 ->
../../[vdso32]/84dbd75729adba57cc42f5544b25de571c0c8731
│   ├── [kernel.kallsyms]
│   │   └── 3ae5ba6d4e532ad529e43ccf1ce1ddf8a64a4fdd
│   ├── [vdso]
│   │   └── 84dbd75729adba57cc42f5544b25de571c0c8731
│   └── [vdso32]
│       └── 84dbd75729adba57cc42f5544b25de571c0c8731
├── lib
│   ├── ld-2.22.so
│   └── libc-2.22.so
├── tmp
│   └── hello
└── xxx

Quoted 6 lines

So all binaries we need are included in the symfs dir. I think this is consistent with your idea explained in previous mails.

With this symfs, we do not need buildid dir anymore and what's your idea on 'perf buildid-cache' needs symfs option? after all, that only effects on buildid dir.

I don't understand why dso-prefix option is needed? Why make me type yet more options to the analysis command? Why can't the directory be located under the symfs tree in a known location and populated the same way it is without symfs?

Quoted 6 lines

I don't understand why dso-prefix option is needed? Why make me type yet more options to the analysis command? Why can't the directory be located under the symfs tree in a known location and populated the same way it is without symfs?

Because the default buidid folder path is $HOME/.debug/.buildid, and this $HOME is on the target machine, not the same as $HOME on the host. Without that option, we need to copy $HOME/.debug/.buildid to the 'known location in symfs', that's also an extra work.

We should check if 'dso' is a null pointer before passing it to the function dso__type(), otherwise a segfault will be raised in dso__data_get_fd(). In function machine__find_vdso(), the return value checking of 'dso' is missed and this patch fixes this issue.

Signed-off-by He Kuang <[email protected]>

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

tools/perf/util/vdso.c +1 -1

index 8f81c41..7bdcad4 100644
@@ -296,7 +296,7 @@ static struct dso *machine__find_vdso(struct machine *machine,
  		if (!dso) {
  			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
  					   true);
-			if (dso_type != dso__type(dso, machine))
+			if (dso && dso_type != dso__type(dso, machine))
  				dso = NULL;
  		}
  		break;

Hi,

This patch fixes a real crash problem when we do 'perf report' on an arm64 platform with arm32 program.
It is introduced by commit f9b2bdf228 ("perf tools: Find vdso with the consider of cross-platform"). From dmesg report, perf crashes in dso__type() because dso is NULL.

Still don't know why on x86 it never crash, but it is obviously

that we need to check the return vaule from __dso__find(): it can be NULL.

So please consider pulling.

Thank you.

Quoted 24 lines · He Kuang

We should check if 'dso' is a null pointer before passing it to the function dso__type(), otherwise a segfault will be raised in dso__data_get_fd(). In function machine__find_vdso(), the return value checking of 'dso' is missed and this patch fixes this issue.

Signed-off-by He Kuang <[email protected]>

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

tools/perf/util/vdso.c +1 -1

index 8f81c41..7bdcad4 100644
@@ -296,7 +296,7 @@ static struct dso *machine__find_vdso(struct 
 machine *machine,
          if (!dso) {
              dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
                         true);
-            if (dso_type != dso__type(dso, machine))
+            if (dso && dso_type != dso__type(dso, machine))
                  dso = NULL;
          }

          break;

Hi,

This patch fixes a real crash problem when we do 'perf report' on an arm64 platform with arm32 program.
It is introduced by commit f9b2bdf228 ("perf tools: Find vdso with the consider of cross-platform"). From dmesg report, perf crashes in dso__type() because dso is NULL.

Still don't know why on x86 it never crash, but it is obviously that we need to check the return vaule from __dso__find(): it can be NULL.

So please consider pulling.

Thank you.

Quoted 23 lines · He Kuang

We should check if 'dso' is a null pointer before passing it to the function dso__type(), otherwise a segfault will be raised in dso__data_get_fd(). In function machine__find_vdso(), the return value checking of 'dso' is missed and this patch fixes this issue.

Signed-off-by He Kuang <[email protected]>

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

tools/perf/util/vdso.c +1 -1

index 8f81c41..7bdcad4 100644
@@ -296,7 +296,7 @@ static struct dso *machine__find_vdso(struct machine *machine,
  		if (!dso) {
  			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
  					   true);
-			if (dso_type != dso__type(dso, machine))
+			if (dso && dso_type != dso__type(dso, machine))
  				dso = NULL;
  		}
  		break;

Em Thu, Jun 23, 2016 at 10:02:39AM +0800, Wangnan (F) escreveu:

Quoted 7 lines

Hi,

This patch fixes a real crash problem when we do 'perf report' on an arm64 platform with arm32 program.
It is introduced by commit f9b2bdf228 ("perf tools: Find vdso with the consider of cross-platform"). From dmesg report, perf crashes in dso__type() because dso is NULL.

Ok, I removed f9b2bdf228 because of this crash, Ingo reported it, now that I see this new patch and checked that the crash was introduced by f9b2bdf228 I'll just combine those two and test again using a 32-bit hackbench on top of f9b2bdf228, to reproduce the crash, then on top of both patches combined.

- Arnaldo

Quoted 33 lines

Still don't know why on x86 it never crash, but it is obviously that we need to check the return vaule from __dso__find(): it can be NULL.

So please consider pulling.

Thank you.

Quoted 23 lines · He Kuang

We should check if 'dso' is a null pointer before passing it to the function dso__type(), otherwise a segfault will be raised in dso__data_get_fd(). In function machine__find_vdso(), the return value checking of 'dso' is missed and this patch fixes this issue.

Signed-off-by He Kuang <[email protected]>

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

tools/perf/util/vdso.c +1 -1

index 8f81c41..7bdcad4 100644
@@ -296,7 +296,7 @@ static struct dso *machine__find_vdso(struct machine *machine,
  		if (!dso) {
  			dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
  					   true);
-			if (dso_type != dso__type(dso, machine))
+			if (dso && dso_type != dso__type(dso, machine))
  				dso = NULL;
  		}
  		break;

hi,

Patch 1-3 fix wrongly used PERF_REG_SP/IP by redefining those macros in the wrapper file according to the target platform, for example in "util/libunwind/x86_32.c".
The first 3 patches have been acked-by "Jiri Olsa" and not touched.

Patch 4 catches an error on python build_ext build.

Patch 5 fixes a NULL pointer deference which can cause segfault when the desired dso is not found.

Thank you.