PR5787, PR2608: .statement(NUM) probing fixes

2008-02-22  Frank Ch. Eigler  <fche@elastic.org>

	PR5787, PR2608, .statement()
	* tapsets.cxx (query_dwarf_func): Process .statement(NUM) probes too.
	(query_cu): Ditto.
	(query_func_info): Bypass prologue searching for .statement() probes.
	(query_cu): Ditto.

	(build_blacklist): Remove unsightly empty first alternative in
	"^(|foo)$" regexps.  Show them for -vvv.

	* buildrun.cxx (compile_pass): Don't turn on "gcc -Q" on until -vvvv.

2008-02-22  Frank Ch. Eigler  <fche@elastic.org>

	* systemtap.base/stmtvars.exp: New test for PR 5787.

5 files changed, 114 insertions, 47 deletions

diff --git a/ChangeLog b/ChangeLog
index 342cb6d5..fad79819 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2008-02-22  Frank Ch. Eigler  <fche@elastic.org>
+
+	PR5787, PR2608, .statement()
+	* tapsets.cxx (query_dwarf_func): Process .statement(NUM) probes too.
+	(query_cu): Ditto.
+	(query_func_info): Bypass prologue searching for .statement() probes.
+	(query_cu): Ditto.
+
+	(build_blacklist): Remove unsightly empty first alternative in
+	"^(|foo)$" regexps.  Show them for -vvv.
+
+	* buildrun.cxx (compile_pass): Don't turn on "gcc -Q" on until -vvvv.
+
 2008-02-20  David Smith  <dsmith@redhat.com>
 
 	* tapsets.cxx (struct mark_derived_probe): Renamed 'probe_sig' to
diff --git a/buildrun.cxx b/buildrun.cxx
index 66cc4986..67836108 100644
--- a/buildrun.cxx
+++ b/buildrun.cxx
@@ -97,7 +97,7 @@ compile_pass (systemtap_session& s)
   for (unsigned i=0; i<s.macros.size(); i++)
     o << "EXTRA_CFLAGS += -D " << lex_cast_qstring(s.macros[i]) << endl;
 
-  if (s.verbose > 2)
+  if (s.verbose > 3)
     o << "EXTRA_CFLAGS += -ftime-report -Q" << endl;
 
   // XXX: unfortunately, -save-temps can't work since linux kbuild cwd
diff --git a/tapsets.cxx b/tapsets.cxx
index 5265a27b..c5ed6ea1 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -2379,16 +2379,11 @@ dwarf_query::build_blacklist()
 
   // Add ^ anchors at the front; $ will be added just before regcomp.
 
-  // NB: all the regexp strings will start with "^(|foo|bar)$", note the
-  // empty first alternative.  It's a bit unsightly, but it lets all the
-  // regexp concatenation statements look uniform, and we should have no
-  // empty actual strings to match against anyway.
-
   string blfn = "^(";
   string blfn_ret = "^(";
   string blfile = "^(";
 
-  blfile += "|kernel/kprobes.c";
+  blfile += "kernel/kprobes.c"; // first alternative, no "|"
   blfile += "|arch/.*/kernel/kprobes.c";
 
   // XXX: it would be nice if these blacklisted functions were pulled
@@ -2402,7 +2397,7 @@ dwarf_query::build_blacklist()
   // also allows detection of problems at translate- rather than
   // run-time.
 
-  blfn += "|atomic_notifier_call_chain";
+  blfn += "atomic_notifier_call_chain"; // first blfn; no "|"
   blfn += "|default_do_nmi";
   blfn += "|__die";
   blfn += "|die_nmi";
@@ -2457,6 +2452,11 @@ dwarf_query::build_blacklist()
   blfn += "|.*preempt_count.*";
   blfn += "|preempt_schedule";
 
+  // These functions don't return, so return probes would never be recovered
+  blfn_ret += "do_exit"; // no "|"
+  blfn_ret += "|sys_exit";
+  blfn_ret += "|sys_exit_group";
+
   // __switch_to changes "current" on x86_64 and i686, so return probes
   // would cause kernel panic, and it is marked as "__kprobes" on x86_64
   if (sess.architecture == "x86_64")
@@ -2464,15 +2464,18 @@ dwarf_query::build_blacklist()
   if (sess.architecture == "i686")
     blfn_ret += "|__switch_to";
 
-  // These functions don't return, so return probes would never be recovered
-  blfn_ret += "|do_exit";
-  blfn_ret += "|sys_exit";
-  blfn_ret += "|sys_exit_group";
-
   blfn += ")$";
   blfn_ret += ")$";
   blfile += ")$";
 
+  if (sess.verbose > 2) 
+    {
+      clog << "blacklist regexps:" << endl;
+      clog << "blfn: " << blfn << endl;
+      clog << "blfn_ret: " << blfn_ret << endl;
+      clog << "blfile: " << blfile << endl;
+    }
+
   int rc = regcomp (& blacklist_func, blfn.c_str(), REG_NOSUB|REG_EXTENDED);
   if (rc) throw semantic_error ("blacklist_func regcomp failed");
   rc = regcomp (& blacklist_func_ret, blfn_ret.c_str(), REG_NOSUB|REG_EXTENDED);
@@ -2812,7 +2815,8 @@ query_func_info (Dwarf_Addr entrypc,
 	}
       else
 	{
-          if (q->sess.prologue_searching)
+          if (q->sess.prologue_searching
+              && !q->has_statement_str && !q->has_statement_num) // PR 2608
             {
               if (fi.prologue_end == 0)
                 throw semantic_error("could not find prologue-end "
@@ -2921,7 +2925,6 @@ static int
 query_dwarf_func (Dwarf_Die * func, void * arg)
 {
   dwarf_query * q = static_cast<dwarf_query *>(arg);
-  assert (!q->has_statement_num);
 
   try
     {
@@ -2949,11 +2952,12 @@ query_dwarf_func (Dwarf_Die * func, void * arg)
 	    {
 	      record_this_function = true;
 	    }
-	  else if (q->has_function_num)
+	  else if (q->has_function_num || q->has_statement_num)
 	    {
-	      Dwarf_Addr query_addr = q->function_num_val;
-              query_addr = q->dw.module_address_to_global(query_addr);
-
+              Dwarf_Addr query_addr =
+                q->dw.module_address_to_global(q->has_function_num ? q->function_num_val :
+                                               q->has_statement_num ? q->statement_num_val :
+                                               (assert(0) , 0));
 	      Dwarf_Die d;
 	      q->dw.function_die (&d);
 
@@ -2966,22 +2970,33 @@ query_dwarf_func (Dwarf_Die * func, void * arg)
 	      if (q->sess.verbose>2)
 		clog << "selected function " << q->dw.function_name << "\n";
 
-	      Dwarf_Addr entrypc;
-	      if (q->dw.function_entrypc (&entrypc))
-		{
-		  func_info func;
-		  q->dw.function_die (&func.die);
-		  func.name = q->dw.function_name;
-		  q->dw.function_file (&func.decl_file);
-		  q->dw.function_line (&func.decl_line);
-		  q->filtered_functions[entrypc] = func;
-
+              func_info func;
+              q->dw.function_die (&func.die);
+              func.name = q->dw.function_name;
+              q->dw.function_file (&func.decl_file);
+              q->dw.function_line (&func.decl_line);
+
+              if (q->has_function_num || q->has_function_str || q->has_statement_str)
+                {
+                  Dwarf_Addr entrypc;
+                  if (q->dw.function_entrypc (&entrypc))
+                    q->filtered_functions[entrypc] = func;
+                  else
+                    throw semantic_error("no entrypc found for function '"
+                                         + q->dw.function_name + "'");
+                }
+              else if (q->has_statement_num)
+                {
+                  Dwarf_Addr probepc = q->statement_num_val;
+		  q->filtered_functions[probepc] = func;
                   if (q->dw.function_name_final_match (q->function))
                     return DWARF_CB_ABORT;
-		}
-	      else
-		throw semantic_error("no entrypc found for function '"
-				     + q->dw.function_name + "'");
+                }
+              else
+                assert(0);
+
+              if (q->dw.function_name_final_match (q->function))
+                return DWARF_CB_ABORT;
 	    }
 	}
       return DWARF_CB_OK;
@@ -3006,7 +3021,7 @@ query_cu (Dwarf_Die * cudie, void * arg)
         clog << "focused on CU '" << q->dw.cu_name
              << "', in module '" << q->dw.module_name << "'\n";
 
-      if (q->has_statement_str
+      if (q->has_statement_str || q->has_statement_num
 	  || q->has_function_str || q->has_function_num)
 	{
 	  q->filtered_srcfiles.clear();
@@ -3037,7 +3052,8 @@ query_cu (Dwarf_Die * cudie, void * arg)
 	  // all in a single pass.
 	  q->dw.iterate_over_functions (query_dwarf_func, q);
 
-          if (q->sess.prologue_searching)
+          if (q->sess.prologue_searching 
+              && !q->has_statement_str && !q->has_statement_num) // PR 2608
             if (! q->filtered_functions.empty())
               q->dw.resolve_prologue_endings (q->filtered_functions);
 
@@ -3053,32 +3069,35 @@ query_cu (Dwarf_Die * cudie, void * arg)
 	    }
 	  else
 	    {
-	      // Otherwise, simply probe all resolved functions (if
-	      // we're scanning functions)
-	      if (q->has_statement_str || q->has_function_str || q->has_function_num)
-		for (map<Dwarf_Addr, func_info>::iterator i = q->filtered_functions.begin();
-		     i != q->filtered_functions.end(); ++i)
-		  query_func_info (i->first, i->second, q);
-
-	      // Or all inline instances (if we're scanning inlines)
-	      if (q->has_statement_str
-                  || ((q->has_function_str || q->has_function_num) && !q->has_call))
+	      // Otherwise, simply probe all resolved functions.
+              for (map<Dwarf_Addr, func_info>::iterator i = q->filtered_functions.begin();
+                   i != q->filtered_functions.end(); ++i)
+                query_func_info (i->first, i->second, q);
+
+	      // And all inline instances (if we're not excluding inlines with ".call")
+	      if (! q->has_call)
 		for (map<Dwarf_Addr, inline_instance_info>::iterator i
 		       = q->filtered_inlines.begin(); i != q->filtered_inlines.end(); ++i)
 		  query_inline_instance_info (i->first, i->second, q);
-
 	    }
 	}
       else
         {
+          // Before PR 5787, we used to have this:
+#if 0
 	  // Otherwise we have a statement number, and we can just
 	  // query it directly within this module.
-
 	  assert (q->has_statement_num);
 	  Dwarf_Addr query_addr = q->statement_num_val;
           query_addr = q->dw.module_address_to_global(query_addr);
 
 	  query_statement ("", "", -1, NULL, query_addr, q);
+#endif
+          // But now, we traverse CUs/functions even for
+          // statement_num's, for blacklist sensitivity and $var
+          // resolution purposes.
+
+          assert (0); // NOTREACHED
         }
       return DWARF_CB_OK;
     }
diff --git a/testsuite/ChangeLog b/testsuite/ChangeLog
index 1760d0aa..38d893db 100644
--- a/testsuite/ChangeLog
+++ b/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2008-02-22  Frank Ch. Eigler  <fche@elastic.org>
+
+	* systemtap.base/stmtvars.exp: New test for PR 5787.
+
 2008-02-21  Will Cohen  <wcohen@redhat.com>
 
 	* systemtap.base/beginenderror.exp: Increase timeout for slow machines.
diff --git a/testsuite/systemtap.base/stmtvars.exp b/testsuite/systemtap.base/stmtvars.exp
new file mode 100644
index 00000000..6e950ea0
--- /dev/null
+++ b/testsuite/systemtap.base/stmtvars.exp
@@ -0,0 +1,31 @@
+set test "stmtvars"
+# PR 5787
+
+set pc 0
+set vars ""
+spawn stap -e "probe kernel.function(\"sys_open\") {\$foo}" -p4 -vv -u
+expect {
+    -re {probe sys_open.*pc=(0x.*)\r\n} { set pc $expect_out(1,string); exp_continue }
+    -re {alternatives: ([^\r\n]*) identifier [^\r\n]*\r\n} { set vars $expect_out(1,string)
+        exp_continue }
+    timeout { fail "$test (timeout)" }
+    eof
+    }
+wait
+verbose -log "pc=$pc vars=$vars"
+if {$pc != 0 && $vars != ""} { pass "$test - .function" } { fail "$test - .function" }
+
+set pc2 0
+set vars2 ""
+spawn stap -e "probe kernel.statement($pc) {\$foo}" -p4 -vv -u
+expect {
+    -re {probe sys_open.*pc=(0x.*)\r\n} { set pc2 $expect_out(1,string); exp_continue }
+    -re {alternatives: ([^\r\n]*) identifier [^\r\n]*\r\n} { set vars2 $expect_out(1,string)
+        exp_continue }
+    timeout { fail "$test (timeout)" }
+    eof
+    }
+wait
+
+verbose -log "pc2=$pc2 vars2=$vars2"
+if {$pc == $pc2 && $vars == $vars2} { pass $test } { fail $test }