Re: [Patch RFC] Tolerate if nsrcs>1 in iterate_over_srcfile_lines

[Jia He <hejianet at gmail dot com>]

On Thu, Jun 13, 2013 at 11:08 PM, Frank Ch. Eigler <fche@redhat.com> wrote:
> Jia He <hejianet@gmail.com> writes:
>
>> Now if dwarf_getsrc_file returns by nsrcs>1 in
>> dwflpp::iterate_over_srcfile_lines,
>> The loop for (int l = lineno; ; l = l + 1) will not be continued.
>> But actually it is not correct in some cases.
>
> Could you elaborate why you think it is incorrect?  Some of the
> filtering is done deliberately, for example if the compiler debuginfo
> cannot give an unambiguous starting PC-address for a source-level
> statement.
Maybe I don't understand the codes correctly, please correct me if possible.
the related readelf info for hrtimer.o(ppc64) is as follows:
#readelf --debug-dump=decodedline hrtimer.o
...
hrtimer.c                                   1601              0x2040
hrtimer.c                                   1605              0x2050
hrtimer.c                                   1601              0x2054
hrtimer.c                                   1605              0x205c
hrtimer.c                                   1601              0x207c
hrtimer.c                                   1608              0x20a0
hrtimer.c                                   1611              0x20ac
hrtimer.c                                   1608              0x20b4
hrtimer.c                                   1611              0x20c8
...
the objdump info(ppc64):
0000000000002040 <.SyS_nanosleep>:
    2040:       7c 08 02 a6     mflr    r0
    2044:       fb a1 ff e8     std     r29,-24(r1)
    2048:       fb e1 ff f8     std     r31,-8(r1)
    204c:       7c 9d 23 78     mr      r29,r4
    2050:       38 a0 00 10     li      r5,16
    2054:       f8 01 00 10     std     r0,16(r1)
    2058:       f8 21 ff 61     stdu    r1,-160(r1)
    205c:       7c 64 1b 78     mr      r4,r3
    2060:       3b e1 00 70     addi    r31,r1,112
    2064:       7f e3 fb 78     mr      r3,r31
    2068:       48 00 00 01     bl      2068 <.SyS_nanosleep+0x28>
    206c:       60 00 00 00     nop
    2070:       38 00 ff f2     li      r0,-14
    2074:       2f a3 00 00     cmpdi   cr7,r3,0
    2078:       41 9e 00 28     beq-    cr7,20a0 <.SyS_nanosleep+0x60>
    207c:       38 21 00 a0     addi    r1,r1,160
    2080:       7c 03 03 78     mr      r3,r0
    2084:       e8 01 00 10     ld      r0,16(r1)
    2088:       eb a1 ff e8     ld      r29,-24(r1)
    208c:       eb e1 ff f8     ld      r31,-8(r1)
    2090:       7c 08 03 a6     mtlr    r0
    2094:       4e 80 00 20     blr
    2098:       60 00 00 00     nop
    209c:       60 00 00 00     nop
    20a0:       e8 01 00 70     ld      r0,112(r1)
    20a4:       2f a0 00 00     cmpdi   cr7,r0,0
    20a8:       40 9c 00 0c     bge-    cr7,20b4 <.SyS_nanosleep+0x74>
    20ac:       38 00 ff ea     li      r0,-22
    20b0:       4b ff ff cc     b       207c <.SyS_nanosleep+0x3c>
    20b4:       3c 00 3b 9a     lis     r0,15258
    20b8:       e9 21 00 78     ld      r9,120(r1)
    20bc:       60 00 c9 ff     ori     r0,r0,51711
    20c0:       7f a9 00 40     cmpld   cr7,r9,r0
    20c4:       41 9d ff e8     bgt+    cr7,20ac <.SyS_nanosleep+0x6c>
    20c8:       7f e3 fb 78     mr      r3,r31
    20cc:       7f a4 eb 78     mr      r4,r29
    20d0:       38 a0 00 01     li      r5,1
    20d4:       38 c0 00 01     li      r6,1
    20d8:       48 00 00 01     bl      20d8 <.SyS_nanosleep+0x98>
    20dc:       7c 60 1b 78     mr      r0,r3
    20e0:       4b ff ff 9c     b       207c <.SyS_nanosleep+0x3c>

Seems if l=1601 or 1605 or 1608 in the loop  for (int l = lineno; ; l = l + 1)
the nsrcs will return >1 after dwarf_getsrc_file().
So the old logic will regard it as sth abnormal and will not consider
it is valid
and will not insert it to lines_probed. Do u think the old logic is incorrect?
>
>> --- systemtap-2.2.1.orig/dwflpp.cxx     2013-05-16 10:30:37.000000000 -0400
>> +++ systemtap-2.2.1/dwflpp.cxx  2013-06-13 01:59:52.000000000 -0400
>> @@ -1619,7 +1619,7 @@ dwflpp::iterate_over_srcfile_lines (char
>>           if (line_type == RANGE && lineno > lines[1])
>>              break;
>>            line_probed = lines_probed.insert(lineno);
>> -          if (lineno != l || line_probed.second == false || nsrcs > 1)
>> +          if (lineno != l || line_probed.second == false)
>>              continue;
>>            dwarf_lineaddr (srcsp [0], &line_addr);
>>            if (!function_name_matches(func_pattern) && dwarf_haspc
>> (function, line_addr) != 1)
>
> For example, this change would ignore srcsp[n] for n>0, which would
> need an explanation about how that could come about and why we can
> ignore them.
>
>
>>                  advice << srcfile << ":" << hi_try;
>>                advice << ")";
>>              }
>> -          throw semantic_error (advice.str());
>> +          if (sess.verbose > 0)
>> +            clog<<advice.str();
>> +//          throw semantic_error (advice.str());
>>          }
>
> What would be the purpose of this change?
If it throws the error message, the loop will not be continued and the
following "l"
will not be checked.
>
>
>> test result
>> command:stap -L 'kernel.statement("sys_nanosleep@kernel/hrtimer.c:*")'
>> in X86_64)
>> Before this patch:
>> kernel.statement("sys_nanosleep@kernel/hrtimer.c:1612") $rmtp:struct
>> timespec* $tu:struct timespec
>>
>> After this patch:
>> kernel.statement("sys_nanosleep@kernel/hrtimer.c:1602") $rqtp:struct
>> timespec* $rmtp:struct timespec* $tu:struct timespec
>> kernel.statement("sys_nanosleep@kernel/hrtimer.c:1605") $rqtp:struct
>> timespec* $rmtp:struct timespec* $tu:struct timespec
>> kernel.statement("sys_nanosleep@kernel/hrtimer.c:1608") $rmtp:struct
>> timespec* $tu:struct timespec
>> kernel.statement("sys_nanosleep@kernel/hrtimer.c:1611") $rmtp:struct
>> timespec* $tu:struct timespec
>> kernel.statement("sys_nanosleep@kernel/hrtimer.c:1612") $rmtp:struct
>> timespec* $tu:struct timespec
>
> That looks good, as long as those listed probe points map to proper
> addresses and give back proper context variables.
>
>
> - FChE