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

[David Smith <dsmith at redhat dot com>]

On 06/26/2013 10:04 AM, David Smith wrote:
> This certainly isn't my area of expertise, but I'm beginning to think
> that both approaches are wrong. Rejecting everthing when nsrcs>1 seems
> to restrictive, but accepting everything when nsrcs>1 seems too broad.
> Perhaps there is a middle ground where if nsrcs>1 we go through each one
> and validate it. We'd only give an error if none were usable.
> To do this the existing code would probably need to be rearranged a bit.
> However I'm still unsure about what to do if nsrcs>1 and more than one
> validate correctly, assuming that is possible.

Here's a patch, but it isn't right either.

diff --git a/dwflpp.cxx b/dwflpp.cxx
index bb0d34a..320fcf9 100644
--- a/dwflpp.cxx
+++ b/dwflpp.cxx
@@ -1623,11 +1623,24 @@ dwflpp::iterate_over_srcfile_lines (char const *
srcfile,
 	  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)
-            break;
+
+	  size_t valid_srcs = 0;
+	  for (size_t s = 0; s < nsrcs; ++s)
+	    {
+	      dwarf_lineaddr (srcsp [s], &line_addr);
+	      if (!function_name_matches(func_pattern)
+		  && dwarf_haspc (function, line_addr) != 1)
+		break;
+	      ++valid_srcs;
+	    }
+	  nsrcs = valid_srcs;
+	  if (nsrcs == 0)
+	      break;
+
+	  // FIXME: if nsrcs > 1, and the a src didn't validate
+	  // above, we don't try the rest.
         }

       // NB: Formerly, we used to filter, because:
@@ -1646,7 +1659,10 @@ dwflpp::iterate_over_srcfile_lines (char const *
srcfile,

       size_t remaining_nsrcs = nsrcs;

-      if (need_single_match && remaining_nsrcs > 1)
+      // FIXME: this still isn't right. Statement probes on a single
+      // line that have >1 src get rejected.
+      if (need_single_match && remaining_nsrcs > 1
+	  && line_type != WILDCARD && line_type != RANGE)
         {
           // We wanted a single line record (a unique address for the
           // line) and we got a bunch of line records. We're going to

Here's what's wrong with this patch. A statement probe with a wildcard
looks reasonable now:

# stap -L 'kernel.statement("sys_nanosleep@kernel/hrtimer.c:*")'
kernel.statement("SyS_nanosleep@kernel/hrtimer.c:1644") $rqtp:long int
$rmtp:long int
kernel.statement("SyS_nanosleep@kernel/hrtimer.c:1649") $rqtp:long int
$rmtp:long int
kernel.statement("SyS_nanosleep@kernel/hrtimer.c:1650") $rqtp:long int
$rmtp:long int
kernel.statement("SyS_nanosleep@kernel/hrtimer.c:1653") $rqtp:long int
$rmtp:long int
kernel.statement("SyS_nanosleep@kernel/hrtimer.c:1655") $rqtp:long int
$rmtp:long int

On my kernel (3.10.0-0.rc6.git0.4.fc20.x86_64), lines 1644 and 1650 have
multiple line information. The problem is that probing individual lines
fails:

# stap -L 'kernel.statement("sys_nanosleep@kernel/hrtimer.c:1644")'
# stap -L 'kernel.statement("sys_nanosleep@kernel/hrtimer.c:1649")'
kernel.statement("SyS_nanosleep@kernel/hrtimer.c:1649") $rqtp:long int
$rmtp:long int
# stap -L 'kernel.statement("sys_nanosleep@kernel/hrtimer.c:1650")'
# stap -L 'kernel.statement("sys_nanosleep@kernel/hrtimer.c:1653")'
kernel.statement("SyS_nanosleep@kernel/hrtimer.c:1653") $rqtp:long int
$rmtp:long int

As I said before, this area of the code is really outside my knowledge,
but I'm wondering if we need to rethink the 'need_single_match' section.
'need_single_match' is really 'this_is_a_statement_probe'. Perhaps we
could do the same validation we do in the wildcard section when nsrcs>1
before the rejection.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)