[Bug translator/16615] New: don't require access to dwarf_query in has_single_line_record()

[Bug translator/16615] New: don't require access to dwarf_query in has_single_line_record()

[jlebon at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16615

            Bug ID: 16615
           Summary: don't require access to dwarf_query in
                    has_single_line_record()
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: translator
          Assignee: systemtap at sourceware dot org
          Reporter: jlebon at redhat dot com

The function iterate_over_srcfile_lines() iterates over target lines specified
by the user. When a line corresponds to multiple addresses,
iterate_over_srcfile_lines() looks around the area to see if there is another
line that we could suggest to users that has a single line record, which is why
has_single_line_record() is used.

The issue is that has_single_line_record() requires access to
q->filtered_functions and q->filtered_inlines to ensure that the suggested
lines fall within functions the user targeted.

There are various FIXMEs and XXXs in these parts of the code related to
eventually getting rid of this coupling.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/16615] don't require access to dwarf_query in has_single_line_record()

[jlebon at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16615

--- Comment #1 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 7430
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7430&action=edit
patch for removing dwarf_query coupling

Here is a possible patch to fix this. We go back to tapsets.cxx-land when
iterate_over_srcfile_lines() meets a line with multiple records and then call
suggest_alternative_lines(), explicitly passing it the acceptable functions and
inlines in which the suggestions have to fall.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/16615] don't require access to dwarf_query in has_single_line_record()

[jlebon at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16615

--- Comment #2 from Jonathan Lebon <jlebon at redhat dot com> ---
Looking at this again, I took the opportunity to try and do this uncoupling
properly and at the same time improve the way func@file:N statements are
implemented. The branch jlebon/pr16615 contains the relevant patch series.

I will just go through the main ways in which behaviour has changed:

1. Handling of linenos with multiple line records

We previously completely skipped those lines and threw a semantic error with
suggestions on nearby lines with single line records. This behaviour is kept
for ABSOLUTE and RELATIVE line types (i.e. the user specified a specific
lineno, so better not be ambiguous). However, for WILDCARD and RANGE line
types, we simply pick the first line record (rather than skip over them as we
did before).

$ nl -b a statement.c
     1  #include <stdio.h>
     2
     3  int foo(int a)
     4  {
     5     int b = 2;
     6     return b + a * 3;
     7  }
     8
     9  void bar(int b) { b += 2; printf("single line! %d\n", b); }
    10
    11  int main(int argc, char** argv)
    12  {
    13     if (argc != 1)
    14        return 42;
    15     foo(argv[0][0]); bar(argv[0][1]); return 0; }
$ gcc statement.c -g -o statement
$ stap -l 'process("./statement").statement("main@statement.c:*")' # OLD STAP
process("/tmp/statement").statement("main@/tmp/statement.c:12")
process("/tmp/statement").statement("main@/tmp/statement.c:13")
process("/tmp/statement").statement("main@/tmp/statement.c:14")
$ stap -l 'process("./statement").statement("main@statement.c:*")' # NEW STAP
process("/tmp/statement").statement("main@/tmp/statement.c:12")
process("/tmp/statement").statement("main@/tmp/statement.c:13")
process("/tmp/statement").statement("main@/tmp/statement.c:14")
process("/tmp/statement").statement("main@/tmp/statement.c:15")
$ stap -p2 -e 'probe process("./statement").statement("main@statement.c:15")
{}' # OLD AND NEW STAP
semantic error: multiple addresses for /tmp/statement.c:15 [man error::dwarf]
(try /tmp/statement.c:14)
semantic error: while resolving probe point: identifier 'process' at
<input>:1:7
        source: probe process("./statement").statement("main@statement.c:15")
{}
                      ^

semantic error: no match
Pass 2: analysis failed.  [man error::pass2]
$

2. Handling of RELATIVE and WILDCARD line types

We previously applied RELATIVE and WILDCARD line types to only one of the
filtered functions. Now, these are applied *per* function, so that e.g. the
following works as expected:

$ stap -l 'process("./statement").statement("[fm]*@statement.c+2")' # OLD STAP
process("/tmp/statement").statement("foo@/tmp/statement.c:6")
$ stap -l 'process("./statement").statement("[fm]*@statement.c+2")' # NEW STAP
process("/tmp/statement").statement("foo@/tmp/statement.c:6")
process("/tmp/statement").statement("main@/tmp/statement.c:14")
$

Other issues such as the one described in PR14774 then also become non-issues
and do not require special treatment.

3. Bug fixes

E.g. properly freeing memory, guarding from duplicate linenos, better handling
of libdw failures, and better error messages.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/16615] don't require access to dwarf_query in has_single_line_record()

[jlebon at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16615

--- Comment #3 from Jonathan Lebon <jlebon at redhat dot com> ---
I've just updated the jlebon/pr16615 branch containing many new goodies for
statement probes.

1. SPEED

We no longer rely on dwarf_getsrc_file(), which is slow, imprecise, and has
funny behaviours. Instead, we get our Dwarf_Lines straight from
dwarf_getsrclines(), which elfutils already caches, and then we cache our own
processed version of it. The results are much faster speeds:

$ # OLD STAP
$ time sh -c 'stap -l '\''process.statement("*@*:*")'\'' -c stap | wc -l'
13071

real    25m49.115s
user    25m47.171s
sys     0m0.268s

$ # NEW STAP
$ time sh -c 'stap -l '\''process.statement("*@*:*")'\'' -c stap | wc -l'
54221

real    1m49.447s
user    1m44.032s
sys     0m5.304s

More realistically, even simple kernel statement probes are faster to resolve:

$ # OLD STAP
$ time sh -c 'stap -l '\''kernel.statement("bio_init@bio.c:*")'\'' | wc -l'
1

real    0m8.338s
user    0m8.259s
sys     0m0.039s
$ 
$ # NEW STAP
$ time sh -c 'stap -l '\''kernel.statement("bio_init@bio.c:*")'\'' | wc -l'
4

real    0m0.395s
user    0m0.360s
sys     0m0.032s
$

2. INLINED FUNCTIONS

We previously just skipped linenos with multiple addresses. We're now smarter
about choosing them by tracking which ones are in which DIEs. This greatly
helps inlined functions:

$ nl -b a inlines.c
     1
     2  __attribute__((always_inline))
     3  static void foo(int i)
     4  {
     5     printf("%d\n", i);
     6  }
     7
     8  int main(int argc, char** argv)
     9  {
    10     // This is a comment
    11     foo(argc);
    12     foo(argc*2);
    13     return 0;
    14  }
$ 
$ gcc -g -w inlines.c -o inlines
$ 
$ # OLD STAP
$ stap -e 'probe process.statement("foo@inlines.c:5") { printf("in foo with
i=%d\n", $i) }' -c ./inlines

semantic error: multiple addresses for
/home/yyz/jlebon/codebase/systemtap/systemtap/inlines.c:5 [man error::dwarf]
(try /home/yyz/jlebon/codebase/systemtap/systemtap/inlines.c:4)
semantic error: while resolving probe point: identifier 'process' at
<input>:1:7
        source: probe process.statement("foo@inlines.c:5") { printf("in foo
with i=%d\n", $i) }
                      ^

semantic error: no match
Pass 2: analysis failed.  [man error::pass2]
$ 
$ # NEW STAP
$ stap -e 'probe process.statement("foo@inlines.c:5") { printf("in foo with
i=%d\n", $i) }' -c ./inlines
1
2
in foo with i=1
in foo with i=2
$

3. ACCURACY

Cross-checking against DIEs also allows us to confidently pick the right addr,
even when there are multiple Dwarf_Lines at the same lineno. This gives us
greater coverage in many optimized functions:

$ # OLD STAP
$ stap -l 'kernel.statement("bio_init@bio.c:*")'
kernel.statement("bio_init@fs/bio.c:277")
$ stap -l 'kernel.statement("bio_reset@bio.c:*")'
kernel.statement("bio_reset@fs/bio.c:297")
kernel.statement("bio_reset@fs/bio.c:298")

$ # NEW STAP
$ stap -l 'kernel.statement("bio_init@bio.c:*")'
kernel.statement("bio_init@fs/bio.c:273")
kernel.statement("bio_init@fs/bio.c:274")
kernel.statement("bio_init@fs/bio.c:275")
kernel.statement("bio_init@fs/bio.c:277")
$ stap -l 'kernel.statement("bio_reset@bio.c:*")'
kernel.statement("bio_reset@fs/bio.c:291")
kernel.statement("bio_reset@fs/bio.c:292")
kernel.statement("bio_reset@fs/bio.c:296")
kernel.statement("bio_reset@fs/bio.c:297")
kernel.statement("bio_reset@fs/bio.c:298")

Hopefully, the new code is now more consistent with users' expectations and
easier to understand.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/16615] don't require access to dwarf_query in has_single_line_record()

[jlebon at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16615

Jonathan Lebon <jlebon at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #4 from Jonathan Lebon <jlebon at redhat dot com> ---
Merged into master.

(In reply to Jonathan Lebon from comment #3)
> We no longer rely on dwarf_getsrc_file(), which is slow, imprecise, and has
> funny behaviours.

Hmmm, that sounded a bit harsh in retrospect. Just to clarify,
dwarf_getsrc_file() does the job, but isn't what we need here. It scours all
CUs for any Dwarf_Lines in the target srcfile, when all we're interested in is
the current CU. This is one of the main reasons it is slower. If no line
records are found for the wanted lineno, it picks the next lineno with line
records. This complicates things for us, since we want to be precise.

These are the reasons why it's not a good fit for what we're trying to achieve
here (but may work well in other situations).

-- 
You are receiving this mail because:
You are the assignee for the bug.