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

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

On 2015/05/11 11:22, He Kuang wrote:
> 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?

OK, above should be changed to -ENOTSUP, since it does not seem to come
from an invalid range :)

Thank you,



-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com