2007-08-24 Frank Ch. Eigler <fche@redhat.com>

	PR 4899
	* tapsets.cxx (dwflpp::has_single_line_record): Extended,
	abstraction violated.
	(iterate_over_srcfile_lines): Remove exactly line number match
	logic.  Improve error message to offered better-checked alternative
	line numbers.
	(query_srcfile_line): Whoops, pass scope_die down for statement("...")
	probes, to enable $target var processing.

2007-08-24  Frank Ch. Eigler  <fche@redhat.com>

	PR 4899
	* buildok/fortytwo.stp: New test.

5 files changed, 149 insertions, 44 deletions

diff --git a/ChangeLog b/ChangeLog
index cde5dc75..71025d81 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2007-08-24  Frank Ch. Eigler  <fche@redhat.com>
+
+	PR 4899
+	* tapsets.cxx (dwflpp::has_single_line_record): Extended,
+	abstraction violated.
+	(iterate_over_srcfile_lines): Remove exactly line number match
+	logic.  Improve error message to offered better-checked alternative
+	line numbers.
+	(query_srcfile_line): Whoops, pass scope_die down for statement("...")
+	probes, to enable $target var processing.
+
 2007-08-21  David Smith  <dsmith@redhat.com>
 
 	PR 2305
@@ -6,9 +17,9 @@
 	@count.
 
 2007-08-20  Martin Hunt  <hunt@redhat.com>
-	PR2424
-	From Lai Jiangshan <laijs@cn.fujitsu.com>
-	
+
+	PR 2424
+	From Lai Jiangshan <laijs@cn.fujitsu.com:
 	* util.cxx (cmdstr_quoted): New. Properly quote 
 	command string.
 	* buildrun.cxx (run_pass): Call cmdstr_quoted().
diff --git a/NEWS b/NEWS
index 6cd91482..9bc76658 100644
--- a/NEWS
+++ b/NEWS
@@ -1,10 +1,17 @@
 * What's new since version 0.5.15?
 
 - New security model.  To install a systemtap kernel module, a user
-must be one of the following: the root user; a member of the 'stapdev'
-group; or a member of the 'stapusr' group.  Members of the stapusr group can
-only use modules located in the /lib/modules/VERSION/systemtap
-directory (where VERSION is the output of "uname -r").
+  must be one of the following: the root user; a member of the
+  'stapdev' group; or a member of the 'stapusr' group.  Members of the
+  stapusr group can only use modules located in the
+  /lib/modules/VERSION/systemtap directory (where VERSION is the
+  output of "uname -r").
+
+- .statement("...@file:line") probes now apply heuristics to allow an
+  approximate match for the line number.  This works similarly to gdb,
+  where a breakpoint placed on an empty source line is automatically
+  moved to the next statement.  A silly bug that made many $target
+  variables inaccessible to .statement() probes was also fixed.
 
 * What's new since version 0.5.14?
 
diff --git a/tapsets.cxx b/tapsets.cxx
index 3b08a348..281fcd0a 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -429,6 +429,9 @@ dwarf_diename_integrate (Dwarf_Die *die)
   return dwarf_formstring (dwarf_attr_integrate (die, DW_AT_name, &attr_mem));
 }
 
+
+struct dwarf_query; // forward decl
+
 struct dwflpp
 {
   systemtap_session & sess;
@@ -901,21 +904,7 @@ struct dwflpp
   }
 
 
-  bool has_single_line_record (char const * srcfile, int lineno)
-  {
-    if (lineno < 0)
-      return false;
-
-    Dwarf_Line **srcsp = NULL;
-    size_t nsrcs = 0;
-
-    dwarf_assert ("dwarf_getsrc_file",
-		  dwarf_getsrc_file (module_dwarf,
-				     srcfile, lineno, 0,
-				     &srcsp, &nsrcs));
-
-    return nsrcs == 1;
-  }
+  bool has_single_line_record (dwarf_query * q, char const * srcfile, int lineno);
 
   void iterate_over_srcfile_lines (char const * srcfile,
 				   int lineno,
@@ -925,6 +914,7 @@ struct dwflpp
   {
     Dwarf_Line **srcsp = NULL;
     size_t nsrcs = 0;
+    dwarf_query * q = static_cast<dwarf_query *>(data);
 
     get_module_dwarf();
 
@@ -932,7 +922,8 @@ struct dwflpp
 		  dwarf_getsrc_file (module_dwarf,
 				     srcfile, lineno, 0,
 				     &srcsp, &nsrcs));
-    // BUT:
+
+    // NB: Formerly, we used to filter, because:
 
     // dwarf_getsrc_file gets one *near hits* for line numbers, not
     // exact matches.  For example, an existing file but a nonexistent
@@ -940,11 +931,14 @@ struct dwflpp
     // file.  This may be similar to the GDB breakpoint algorithm, but
     // we don't want to be so fuzzy in systemtap land.  So we filter.
 
+    // But we now see the error of our ways, and skip this filtering.
+
     // XXX: the code also fails to match e.g.  inline function
     // definitions when the srcfile is a header file rather than the
     // CU name.
 
     size_t remaining_nsrcs = nsrcs;
+#if 0
     for (size_t i = 0; i < nsrcs; ++i)
       {
         int l_no;
@@ -961,7 +955,7 @@ struct dwflpp
             remaining_nsrcs --;
           }
       }
-
+#endif
 
     if (need_single_match && remaining_nsrcs > 1)
       {
@@ -976,30 +970,27 @@ struct dwflpp
 	int hi_try = -1;
 	for (size_t i = 1; i < 6; ++i)
 	  {
-	    if (lo_try == -1 && has_single_line_record(srcfile, lineno - i))
+	    if (lo_try == -1 && has_single_line_record(q, srcfile, lineno - i))
 	      lo_try = lineno - i;
 
-	    if (hi_try == -1 && has_single_line_record(srcfile, lineno + i))
+	    if (hi_try == -1 && has_single_line_record(q, srcfile, lineno + i))
 	      hi_try = lineno + i;
 	  }
 
-	string advice = "";
+        stringstream advice;
+        advice << "multiple addresses for " << srcfile << ":" << lineno;
 	if (lo_try > 0 || hi_try > 0)
-	  advice = " (try "
-	    + (lo_try > 0
-	       ? (string(srcfile) + ":" + lex_cast<string>(lo_try))
-	       : string(""))
-	    + (lo_try > 0 && hi_try > 0 ? " or " : "")
-	    + (hi_try > 0
-	       ? (string(srcfile) + ":"+ lex_cast<string>(hi_try))
-	       : string(""))
-	    + ")";
-
-	throw semantic_error("multiple addresses for "
-			     + string(srcfile)
-			     + ":"
-			     + lex_cast<string>(lineno)
-			     + advice);
+          {
+            advice << " (try ";
+            if (lo_try > 0)
+              advice << srcfile << ":" << lo_try;
+            if (lo_try > 0 && hi_try > 0)
+              advice << " or ";
+            if (hi_try > 0)
+              advice << srcfile << ":" << hi_try;
+            advice << ")";
+          }
+        throw semantic_error (advice.str());
       }
 
     try
@@ -2148,6 +2139,73 @@ struct dwarf_query : public base_query
 };
 
 
+// This little test routine represents an unfortunate breakdown in
+// abstraction between dwflpp (putatively, a layer right on top of
+// elfutils), and dwarf_query (interpreting a systemtap probe point).
+// It arises because we sometimes try to fix up slightly-off
+// .statement() probes (something we find out in fairly low-level).
+//
+// An alternative would be to put some more intellgence into query_cu(),
+// and have it print additional suggestions after finding that 
+// q->dw.iterate_over_srcfile_lines resulted in no new finished_results.
+
+bool
+dwflpp::has_single_line_record (dwarf_query * q, char const * srcfile, int lineno)
+{
+  if (lineno < 0)
+    return false;
+
+    Dwarf_Line **srcsp = NULL;
+    size_t nsrcs = 0;
+
+    dwarf_assert ("dwarf_getsrc_file",
+		  dwarf_getsrc_file (module_dwarf,
+				     srcfile, lineno, 0,
+				     &srcsp, &nsrcs));
+
+    if (nsrcs != 1) 
+      {
+        if (sess.verbose>4)
+          clog << "alternative line " << lineno << " rejected: nsrcs=" << nsrcs << endl;
+        return false;
+      }
+
+    // We also try to filter out lines that leave the selected
+    // functions (if any).
+
+    Dwarf_Line *line = srcsp[0];
+    Dwarf_Addr addr;
+    dwarf_lineaddr (line, &addr);
+
+    for (map<Dwarf_Addr, func_info>::iterator i = q->filtered_functions.begin();
+         i != q->filtered_functions.end(); ++i)
+      {
+        if (q->dw.die_has_pc (&(i->second.die), addr))
+          {
+            if (q->sess.verbose>4)
+              clog << "alternative line " << lineno << " accepted: fn=" << i->second.name << endl;
+            return true;
+          }
+      }
+
+    for (map<Dwarf_Addr, inline_instance_info>::iterator i = q->filtered_inlines.begin();
+         i != q->filtered_inlines.end(); ++i)
+      {
+        if (q->dw.die_has_pc (&(i->second.die), addr))
+          {
+            if (sess.verbose>4)
+              clog << "alternative line " << lineno << " accepted: ifn=" << i->second.name << endl;
+            return true;
+          }
+      }
+
+    if (sess.verbose>4)
+      clog << "alternative line " << lineno << " rejected: leaves selected fns" << endl;
+    return false;
+  }
+
+
+
 struct dwarf_builder: public derived_probe_builder
 {
   dwflpp *kern_dw;
@@ -2672,7 +2730,7 @@ query_srcfile_line (Dwarf_Line * line, void * arg)
 	  if (q->has_statement_str)
 	    query_statement (i->second.name, i->second.decl_file,
 			     lineno, // NB: not q->line !
-                             NULL, addr, q);
+                             &(i->second.die), addr, q);
 	  else
 	    query_func_info (i->first, i->second, q);
 	}
@@ -2688,7 +2746,7 @@ query_srcfile_line (Dwarf_Line * line, void * arg)
 	    clog << "inline instance DIE lands on srcfile\n";
 	  if (q->has_statement_str)
 	    query_statement (i->second.name, i->second.decl_file,
-			     q->line, NULL, addr, q);
+			     q->line, &(i->second.die), addr, q);
 	  else
 	    query_inline_instance_info (i->first, i->second, q);
 	}
diff --git a/testsuite/ChangeLog b/testsuite/ChangeLog
index fbe1143e..bb55955c 100644
--- a/testsuite/ChangeLog
+++ b/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2007-08-24  Frank Ch. Eigler  <fche@redhat.com>
+
+	PR 4899
+	* buildok/fortytwo.stp: New test.
+
 2007-08-21  David Smith  <dsmith@redhat.com>
 
 	* foreach_limit.stp: Added test for sorting numeric arrays by
diff --git a/testsuite/buildok/fortytwo.stp b/testsuite/buildok/fortytwo.stp
new file mode 100755
index 00000000..c37d19af
--- /dev/null
+++ b/testsuite/buildok/fortytwo.stp
@@ -0,0 +1,24 @@
+#! /bin/sh
+
+# This is a roundabout test of kernel.statement().
+
+set -e
+
+fn="do_readv_writev@fs/read_write.c"
+var="file"
+
+fullfn=`stap -p2 -e 'probe kernel.statement("'$fn'") {}' | grep kernel | cut -f2 -d'"'`
+lineno=`echo $fullfn | cut -f2 -d:`
+
+echo "$0: $fn found, starting line $lineno"
+
+for i in 0 1 2 4 6 10 15 20 25 # some random numbers, not larger than the number of lines in fn
+do
+    ilineno=`expr $lineno + $i`
+    errors=`stap -u -p4 -e 'probe kernel.statement("'$fn':'$ilineno'") {$'$var'}' 2>&1 >/dev/null ||true `
+    if echo "$errors" | grep -q unable.to.find.local
+    then
+	echo "Unexpected error $errors"; exit 1
+    fi
+    echo 'probe kernel.statement("'$fn':'$ilineno'") {$'$var'}' '#' OK
+done