Re: [PATCH v3 1/7 UPDATE] perf tools: Find vdso with the consider of cross-platform

[Hekuang]

å 2016/5/17 15:33, Adrian Hunter åé:
> On 13/05/16 11:51, He Kuang wrote:
> > 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 <hekuang@xxxxxxxxxx>
> > ---
> >   tools/perf/util/vdso.c | 40 +++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 37 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/vdso.c b/tools/perf/util/vdso.c
> > index 44d440d..99f4a3d 100644
> > --- a/tools/perf/util/vdso.c
> > +++ b/tools/perf/util/vdso.c
> > @@ -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);
> 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;
>
> > +		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.

Thanks, update
> > +		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;