Re: [PATCH v2 2/3] perf probe: Add --range option to show variable location range

[He Kuang <hekuang@huawei.com>]

Hi, Masami

On 2015/5/10 11:21, Masami Hiramatsu wrote:
> On 2015/05/09 18:55, He Kuang wrote:
>> It is not easy for users to get the accurate byte offset or the line
>> number where a local variable can be probed. With '--range' option,
>> local variables in scope of the probe point are showed with byte offset
>> range, and can be added according to this range information.
>>
>> For example, there are some variables in function
>> generic_perform_write():
>>
>>    <generic_perform_write@mm/filemap.c:0>
>>    0  ssize_t generic_perform_write(struct file *file,
>>    1                                 struct iov_iter *i, loff_t pos)
>>    2  {
>>    3          struct address_space *mapping = file->f_mapping;
>>    4          const struct address_space_operations *a_ops = mapping->a_ops;
>>    ...
>>    42                 status = a_ops->write_begin(file, mapping, pos, bytes, flags,
>>                                                 &page, &fsdata);
>>    44                 if (unlikely(status < 0))
>>
>> But we got failed when we try to probe the variable 'a_ops' at line 42
>> or 44.
>>
>>    $ perf probe --add 'generic_perform_write:42 a_ops'
>>    Failed to find the location of a_ops at this address.
>>      Perhaps, it has been optimized out.
>>
>> This is because source code do not match assembly, so a variable may not
>> be available in the sourcecode line where it presents. After this patch,
>> we can lookup the accurate byte offset range of a variable, 'INV'
>> indicates that this variable is not valid at the given point, but
>> available in scope:
>>
>>    $ perf probe --vars 'generic_perform_write:42' --range
>>    Available variables at generic_perform_write:42
>>          @<generic_perform_write+141>
>>                  [INV]   ssize_t written @<generic_perform_write+[324-331]>
>>                  [INV]   struct address_space_operations*        a_ops   @<generic_perform_write+[55-61,170-176,223-246]>
>>                  [VAL]   (unknown_type)  fsdata  @<generic_perform_write+[70-307,346-411]>
>>                  [VAL]   loff_t  pos     @<generic_perform_write+[0-286,286-336,346-411]>
>>                  [VAL]   long int        status  @<generic_perform_write+[83-342,346-411]>
>>                  [VAL]   long unsigned int       bytes   @<generic_perform_write+[122-311,320-338,346-403,403-411]>
>>                  [VAL]   struct address_space*   mapping @<generic_perform_write+[35-344,346-411]>
>>                  [VAL]   struct iov_iter*        i       @<generic_perform_write+[0-340,346-411]>
>>                  [VAL]   struct page*    page    @<generic_perform_write+[70-307,346-411]>
>>
> Thanks, this looks easier to understand :)
>
> [...]
>> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
>> index dcca551..30a1a1b 100644
>> --- a/tools/perf/util/probe-finder.c
>> +++ b/tools/perf/util/probe-finder.c
>> @@ -43,6 +43,9 @@
>>   /* Kprobe tracer basic type is up to u64 */
>>   #define MAX_BASIC_TYPE_BITS	64
>>   
>> +/* Variable location invalid at addr but valid in scope */
>> +#define VARIABLE_LOCATION_INVALID_AT_ADDR	-10000
> Hmm, could you use -ERANGE instead of this?
> Other part is OK for me.
>
> Thank you!

I've checked libdw, it never returns -ERANGE, but there is an
errno conflict in the function convert_variable_location itself:

   268        regs = get_arch_regstr(regn);
   269        if (!regs) {
   270                /* This should be a bug in DWARF or this tool */
   271                pr_warning("Mapping for the register number %u "
   272                           "missing on this architecture.\n", regn);
   273                return -ERANGE;
   274 }

So shell we change the above errno to -ENOENT or choose another
errno for current 'VARIABLE_LOCATION_INVALID_AT_ADDR', what's
your opinion?

Thanks!

>
>
>> +
>>   /* Dwarf FL wrappers */
>>   static char *debuginfo_path;	/* Currently dummy */
>>   
>> @@ -177,7 +180,7 @@ static int convert_variable_location(Dwarf_Die *vr_die, Dwarf_Addr addr,
>>   	Dwarf_Word offs = 0;
>>   	bool ref = false;
>>   	const char *regs;
>> -	int ret;
>> +	int ret, ret2 = 0;
>>   
>>   	if (dwarf_attr(vr_die, DW_AT_external, &attr) != NULL)
>>   		goto static_var;
>> @@ -187,9 +190,19 @@ static int convert_variable_location(Dwarf_Die *vr_die, Dwarf_Addr addr,
>>   		return -EINVAL;	/* Broken DIE ? */
>>   	if (dwarf_getlocation_addr(&attr, addr, &op, &nops, 1) <= 0) {
>>   		ret = dwarf_entrypc(sp_die, &tmp);
>> -		if (ret || addr != tmp ||
>> -		    dwarf_tag(vr_die) != DW_TAG_formal_parameter ||
>> -		    dwarf_highpc(sp_die, &tmp))
>> +		if (ret)
>> +			return -ENOENT;
>> +
>> +		if (probe_conf.show_location_range &&
>> +			(dwarf_tag(vr_die) == DW_TAG_variable)) {
>> +			ret2 = VARIABLE_LOCATION_INVALID_AT_ADDR;
>> +		} else if (addr != tmp ||
>> +			dwarf_tag(vr_die) != DW_TAG_formal_parameter) {
>> +			return -ENOENT;
>> +		}
>> +
>> +		ret = dwarf_highpc(sp_die, &tmp);
>> +		if (ret)
>>   			return -ENOENT;
>>   		/*
>>   		 * This is fuzzed by fentry mcount. We try to find the
>> @@ -210,7 +223,7 @@ found:
>>   	if (op->atom == DW_OP_addr) {
>>   static_var:
>>   		if (!tvar)
>> -			return 0;
>> +			return ret2;
>>   		/* Static variables on memory (not stack), make @varname */
>>   		ret = strlen(dwarf_diename(vr_die));
>>   		tvar->value = zalloc(ret + 2);
>> @@ -220,7 +233,7 @@ static_var:
>>   		tvar->ref = alloc_trace_arg_ref((long)offs);
>>   		if (tvar->ref == NULL)
>>   			return -ENOMEM;
>> -		return 0;
>> +		return ret2;
>>   	}
>>   
>>   	/* If this is based on frame buffer, set the offset */
>> @@ -250,7 +263,7 @@ static_var:
>>   	}
>>   
>>   	if (!tvar)
>> -		return 0;
>> +		return ret2;
>>   
>>   	regs = get_arch_regstr(regn);
>>   	if (!regs) {
>> @@ -269,7 +282,7 @@ static_var:
>>   		if (tvar->ref == NULL)
>>   			return -ENOMEM;
>>   	}
>> -	return 0;
>> +	return ret2;
>>   }
>>   
>>   #define BYTES_TO_BITS(nb)	((nb) * BITS_PER_LONG / sizeof(long))
>> @@ -1268,13 +1281,34 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
>>   		ret = convert_variable_location(die_mem, af->pf.addr,
>>   						af->pf.fb_ops, &af->pf.sp_die,
>>   						NULL);
>> -		if (ret == 0) {
>> +		if (ret == 0 || ret == VARIABLE_LOCATION_INVALID_AT_ADDR) {
>> +			int ret2;
>> +			bool externs = !af->child;
>>   			struct strbuf buf;
>>   
>>   			strbuf_init(&buf, 64);
>> -			ret = die_get_varname(die_mem, &buf);
>> -			pr_debug2("Add new var: %s\n", buf.buf);
>> -			if (ret == 0) {
>> +
>> +			if (probe_conf.show_location_range) {
>> +				if (!externs) {
>> +					if (ret)
>> +						strbuf_addf(&buf, "[INV]\t");
>> +					else
>> +						strbuf_addf(&buf, "[VAL]\t");
>> +				} else
>> +					strbuf_addf(&buf, "[EXT]\t");
>> +			}
>> +
>> +			ret2 = die_get_varname(die_mem, &buf);
>> +
>> +			if (!ret2 && probe_conf.show_location_range &&
>> +				!externs) {
>> +				strbuf_addf(&buf, "\t");
>> +				ret2 = die_get_var_range(&af->pf.sp_die,
>> +							die_mem, &buf);
>> +			}
>> +
>> +			pr_debug("Add new var: %s\n", buf.buf);
>> +			if (ret2 == 0) {
>>   				strlist__add(vl->vars,
>>   					strbuf_detach(&buf, NULL));
>>   			}
>>
>