Re: [PATCH] perf report: Fix invalid memory accessing

[Arnaldo Carvalho de Melo <acme@redhat.com>]

Em Tue, Sep 08, 2015 at 09:37:47AM +0200, Jiri Olsa escreveu:
> On Mon, Sep 07, 2015 at 09:27:26PM +0800, Wangnan (F) wrote:
> 
> SNIP
> 
> > 
> > I found the problem.
> > 
> > perf relies on build_cpu_topology() to fetch CPU_TOPOLOGY from sysfs. It
> > depend on
> > the existance of
> > 
> > /sys/devices/system/cpu/cpu%d/topology/core_siblings_list
> > 
> > However, CPU can be canceled by hotcpu subsystem. After that the directory
> > of
> > /sys/devices/system/cpu/cpu%d/topology is gone, which causes perf's
> > write_cpu_topology() --> uild_cpu_topology() to fail, result in the above
> > perf.data.
> > 
> > So I think my patch is required.
> 
> no question there.. I just meant it should be placed in
> perf_event__preprocess_sample function with the rest of
> the 'al' initialization, like in the patch below?
> 
> it does not compile, because there're many places calling
> it and it'd need changing all callers to pass env, which
> seems to require more changes..

Humm, I think that we can have a pointer to the current perf_env, be it
from the current machine, or from the machine environment in the
perf.data file in struct machine, that way we don't need to change that
function prototype, I'm prototyping this now, will post a patch.

- Arnaldo
 
> also I'm not sure about removing:
> -	al->socket = cpu_map__get_socket_id(al->cpu);
> 
> 
> Does any command actually need this initialized from current system?
> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 0bf8c9889fc0..3339d2579bfc 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -990,7 +990,8 @@ void thread__find_addr_location(struct thread *thread,
>  int perf_event__preprocess_sample(const union perf_event *event,
>  				  struct machine *machine,
>  				  struct addr_location *al,
> -				  struct perf_sample *sample)
> +				  struct perf_sample *sample,
> +				  struct perf_env *env)
>  {
>  	u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
>  	struct thread *thread = machine__findnew_thread(machine, sample->pid,
> @@ -1021,7 +1022,10 @@ int perf_event__preprocess_sample(const union perf_event *event,
>  
>  	al->sym = NULL;
>  	al->cpu = sample->cpu;
> -	al->socket = cpu_map__get_socket_id(al->cpu);
> +
> +	al.socket = -1;
> +	if (env->cpu && al->cpu >= 0)
> +		al.socket = env->cpu[al->cpu].socket_id;
>  
>  	if (al->map) {
>  		struct dso *dso = al->map->dso;