Re: [PATCH v4 2/2] perf: report/annotate: fix segfault problem.

[Wang Nan <wangnan0@huawei.com>]

On 2015/4/9 19:52, Jiri Olsa wrote:
> On Wed, Apr 08, 2015 at 03:52:18AM +0000, Wang Nan wrote:
> SNIP
> 
>>  
>> -bool is_kernel_module(const char *pathname)
>> +bool is_kernel_module(const char *pathname, int cpumode)
>>  {
>>  	struct kmod_path m;
>>  
>> -	if (kmod_path__parse(&m, pathname))
>> +	if (kmod_path__parse(&m, pathname, cpumode))
>>  		return NULL;

Find another problem here: return NULL for bool function. Will fix in v5.

>>  
>>  	return m.kmod;
>> @@ -210,21 +210,56 @@ bool dso__needs_decompress(struct dso *dso)
>>   * Returns 0 if there's no strdup error, -ENOMEM otherwise.
>>   */
>>  int __kmod_path__parse(struct kmod_path *m, const char *path,
>> -		       bool alloc_name, bool alloc_ext)
>> +		       int cpumode, bool alloc_name, bool alloc_ext)
>>  {
>>  	const char *name = strrchr(path, '/');
>>  	const char *ext  = strrchr(path, '.');
>> +	bool is_simple_name = false;
>> +	bool cpu_mode_kernel, cpu_mode_unknown = false, is_kernel = false;
>> +
>> +	/* treat PERF_RECORD_MISC_CPUMODE_UNKNOWN as kernel. */
>> +	switch (cpumode & PERF_RECORD_MISC_CPUMODE_MASK) {
>> +	case PERF_RECORD_MISC_USER:
>> +	case PERF_RECORD_MISC_HYPERVISOR:
>> +	case PERF_RECORD_MISC_GUEST_USER:
>> +		cpu_mode_kernel = false;
>> +		break;
>> +	case PERF_RECORD_MISC_CPUMODE_UNKNOWN:
>> +		cpu_mode_unknown = true;
>> +		/* fall through */
>> +	default:
>> +		cpu_mode_kernel = true;
>> +	}
> 
> so the cpumode is valid only for the is_kernel_module caller,
> the rest of the users use PERF_RECORD_MISC_CPUMODE_UNKNOWN
> 
> could you move this logic into is_kernel_module function
> and let __kmod_path__parse do only parsing work..?
> 
> also new test for is_kernel_module functionality would be nice ;-)
> 
>>  
>>  	memset(m, 0x0, sizeof(*m));
>>  	name = name ? name + 1 : path;
>>  
>> +	/*
>> +	 * '.' is also a valid character. For example: [aaa.bbb] is a
>> +	 * valid module name. '[' should have higher priority than
>> +	 * '.ko' suffix.
>> +	 *
>> +	 * The kernel names are from machine__mmap_name. Such
>> +	 * name should belong to kernel itself, not kernel module.
>> +	 */
>> +	if (name[0] == '[') {
>> +		is_simple_name = true;
>> +		if ((strncmp(name, "[kernel.kallsyms]", 17) == 0) ||
>> +		    (strncmp(name, "[guest.kernel.kallsyms", 22) == 0)) {
> 
> these checks would stay in here together with checks
> for [vdso] and [vsyscall] right?
> 
> SNIP
> 
>>  bool is_supported_compression(const char *ext);
>> -bool is_kernel_module(const char *pathname);
>> +bool is_kernel_module(const char *pathname, int cpumode);
>>  bool decompress_to_file(const char *ext, const char *filename, int output_fd);
>>  bool dso__needs_decompress(struct dso *dso);
>>  
>> @@ -228,11 +228,13 @@ struct kmod_path {
>>  };
>>  
>>  int __kmod_path__parse(struct kmod_path *m, const char *path,
>> -		     bool alloc_name, bool alloc_ext);
>> +		     int cpumode, bool alloc_name, bool alloc_ext);
>>  
>> -#define kmod_path__parse(__m, __p)      __kmod_path__parse(__m, __p, false, false)
>> -#define kmod_path__parse_name(__m, __p) __kmod_path__parse(__m, __p, true , false)
>> -#define kmod_path__parse_ext(__m, __p)  __kmod_path__parse(__m, __p, false, true)
>> +#define kmod_path__parse(__m, __p, __c)      __kmod_path__parse(__m, __p, __c, false, false)
>> +#define kmod_path__parse_name(__m, __p) __kmod_path__parse(__m, __p, \
>> +		PERF_RECORD_MISC_CPUMODE_UNKNOWN, true , false)
>> +#define kmod_path__parse_ext(__m, __p)       __kmod_path__parse(__m, __p, \
>> +		PERF_RECORD_MISC_CPUMODE_UNKNOWN, false, true)
>>  
>>  /*
>>   * The dso__data_* external interface provides following functions:
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index fb43215..d106e12 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -1266,7 +1266,7 @@ static int __event_process_build_id(struct build_id_event *bev,
>>  
>>  		dso__set_build_id(dso, &bev->build_id);
>>  
>> -		if (!is_kernel_module(filename))
>> +		if (!is_kernel_module(filename, misc))
> 
> please pass either whole misc or just cpumode like you do below
> in machine__process_kernel_mmap_event
> 
> however the is_kernel_module declaration says cpumode so
> I'd expect cpumode, not complete misc
> 
>>  			dso->kernel = dso_type;
>>  
>>  		build_id__sprintf(dso->build_id, sizeof(dso->build_id),
>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>> index e45c8f3..e084943 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -1109,7 +1109,9 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
>>  		struct dso *dso;
>>  
>>  		list_for_each_entry(dso, &machine->kernel_dsos.head, node) {
>> -			if (is_kernel_module(dso->long_name))
>> +			if (is_kernel_module(dso->long_name,
>> +					     event->header.misc &
>> +					     PERF_RECORD_MISC_CPUMODE_MASK))
>>  				continue;
>>  
>>  			kernel = dso;
>> -- 
>> 1.8.3.4
>>