PR 3887: sufficient+optional probe points

2007-11-19  Frank Ch. Eigler  <fche@elastic.org>

	PR 3887.
	* staptree.h (probe_point): Add "sufficient" field.
	* staptree.cxx: Initialize it, print it.
	* parse.cxx (parse_probe_point): Parse it.
	* elaborate.cxx (derive_probes): Implement it.
	* stapprobes.5.in: Document it.
	* NEWS: Gloat about it.

	* parseok/five.stp, semok/twentyseven.stp: Test "!" probe point flag.

	* iostat-scsi.stp: Adopt "!" probe point flag.

12 files changed, 114 insertions, 34 deletions

diff --git a/ChangeLog b/ChangeLog
index 01b6525d..eae8dda8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2007-11-19  Frank Ch. Eigler  <fche@elastic.org>
+
+	PR 3887.
+	* staptree.h (probe_point): Add "sufficient" field.
+	* staptree.cxx: Initialize it, print it.
+	* parse.cxx (parse_probe_point): Parse it.
+	* elaborate.cxx (derive_probes): Implement it.
+	* stapprobes.5.in: Document it.
+	* NEWS: Gloat about it.
+
 2007-11-15  David Smith  <dsmith@redhat.com>
 
 	* tapsets.cxx (mark_derived_probe::initialize_probe_context_vars):
diff --git a/NEWS b/NEWS
index b093d78f..c5f3937c 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,21 @@
     probe error { println ("oops, errors encountered; here's a report anyway")
                   foreach (coin in mint) { println (coin) } }
 
+- In a related twist, one may list probe points in order of preference,
+  and mark any of them as "sufficient" beyond just "optional".  Probe
+  point sequence expansion stops if a sufficient-marked probe point has a hit.
+  This is useful for probes on functions that may be in a module (CONFIG_FOO=m)
+  or may have been compiled into the kernel (CONFIG_FOO=y), but we don't know
+  which.  Instead of
+
+    probe module("sd").function("sd_init_command") ? ,
+          kernel.function("sd_init_command") ? { ... }
+
+  which might match neither, now one can write this:
+
+    probe module("sd").function("sd_init_command") ! , /* <-- note excl. mark */
+          kernel.function("sd_init_command")  { ... }
+
 - 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
diff --git a/elaborate.cxx b/elaborate.cxx
index d86cc37d..aa6529fa 100644
--- a/elaborate.cxx
+++ b/elaborate.cxx
@@ -553,23 +553,40 @@ derive_probes (systemtap_session& s,
     {
       probe_point *loc = p->locations[i];
 
-      // Pass down optional flag from e.g. alias reference to each
-      // probe_point instance.  We do this by temporarily overriding
-      // the probe_point optional flag.  We could instead deep-copy
-      // and set a flag on the copy permanently.
-
-      bool old_loc_opt = loc->optional;
-      loc->optional = loc->optional || optional;
-
       try
         {
           unsigned num_atbegin = dps.size();
-          s.pattern_root->find_and_build (s, p, loc, 0, dps);
+
+          // Pass down optional flag from e.g. alias reference to each
+          // probe_point instance.  We do this by temporarily overriding
+          // the probe_point optional flag.  We could instead deep-copy
+          // and set a flag on the copy permanently.
+          bool old_loc_opt = loc->optional;
+          loc->optional = loc->optional || optional;
+          s.pattern_root->find_and_build (s, p, loc, 0, dps); // <-- actual derivation!
+          loc->optional = old_loc_opt;
           unsigned num_atend = dps.size();
-          
-          if (! loc->optional && // something required, but
+
+          if (! (loc->optional||optional) && // something required, but
               num_atbegin == num_atend) // nothing new derived!
-            throw semantic_error ("no match for probe point");
+            throw semantic_error ("no match");
+
+          if (loc->sufficient && (num_atend > num_atbegin))
+            {
+              if (s.verbose > 1)
+                {
+                  clog << "Probe point ";
+                  p->locations[i]->print(clog);
+                  clog << " sufficient, skipped";
+                  for (unsigned j = i+1; j < p->locations.size(); ++j)
+                    {
+                      clog << " ";
+                      p->locations[j]->print(clog);
+                    }
+                  clog << endl;
+                }
+              break; // we need not try to derive for any other locations
+            }
         }
       catch (const semantic_error& e)
         {
@@ -584,7 +601,6 @@ derive_probes (systemtap_session& s,
           delete er;
         }
 
-      loc->optional = old_loc_opt;
     }
 }
 
diff --git a/examples/ChangeLog b/examples/ChangeLog
index ded655b4..c1bf2238 100644
--- a/examples/ChangeLog
+++ b/examples/ChangeLog
@@ -1,3 +1,7 @@
+2007-11-19  Frank Ch. Eigler  <fche@elastic.org>
+
+	* iostat-scsi.stp: Adopt "!" probe point flag.
+
 2007-11-09  Martin Hunt  <hunt@redhat.com>
 
 	* README: New.
diff --git a/examples/iostat-scsi.stp b/examples/iostat-scsi.stp
index ec1aeeb8..a65ec3f7 100755
--- a/examples/iostat-scsi.stp
+++ b/examples/iostat-scsi.stp
@@ -3,7 +3,7 @@
 global devices, reads, writes
 
 /* data collection: SCSI disk */
-probe module("sd_mod").function("sd_init_command") ?, kernel.function("sd_init_command") ? {
+probe module("sd_mod").function("sd_init_command") !, kernel.function("sd_init_command") {
   device=kernel_string($SCpnt->request->rq_disk->disk_name)
   sector_size=$SCpnt->device->sector_size
   nr_sectors=$SCpnt->request->nr_sectors
@@ -18,7 +18,7 @@ probe module("sd_mod").function("sd_init_command") ?, kernel.function("sd_init_c
     reads[device] <<< nr_sectors * sector_size
 }
 /* data collection: SCSI tape */
-probe module("st").function("st_do_scsi") ?, kernel.function("st_do_scsi") ? {
+probe module("st").function("st_do_scsi") !, kernel.function("st_do_scsi") {
   device=kernel_string($STp->disk->disk_name)
   devices[device] = 1
   if ($direction)
diff --git a/parse.cxx b/parse.cxx
index 173639b3..228f9a2c 100644
--- a/parse.cxx
+++ b/parse.cxx
@@ -996,7 +996,6 @@ parser::parse_probe (std::vector<probe *> & probe_ret,
           next ();
           continue;
         }
-
       else if (t && t->type == tok_operator && t->content == ",")
         {
           locations.push_back(pp);
@@ -1348,9 +1347,12 @@ parser::parse_probe_point ()
       // We only fall through here at the end of a probe point (past
       // all the dotted/parametrized components).
 
-      if (t && t->type == tok_operator && t->content == "?")
+      if (t && t->type == tok_operator &&
+          (t->content == "?" || t->content == "!"))
         {
           pl->optional = true;
+          if (t->content == "!") pl->sufficient = true;
+          // NB: sufficient implies optional
           next ();
           t = peek ();
           // fall through
@@ -1361,7 +1363,7 @@ parser::parse_probe_point ()
               t->content == "=" || t->content == "+=" ))
         break;
       
-      throw parse_error ("expected '.' or ',' or '(' or '?' or '{' or '=' or '+='");
+      throw parse_error ("expected one of '. , ( ? ! { = +='");
     }
 
   return pl;
diff --git a/stapprobes.5.in b/stapprobes.5.in
index 6e271304..6d1df5a4 100644
--- a/stapprobes.5.in
+++ b/stapprobes.5.in
@@ -25,11 +25,22 @@ The general probe point syntax is a dotted-symbol sequence.  This
 allows a breakdown of the event namespace into parts, somewhat like
 the Domain Name System does on the Internet.  Each component
 identifier may be parametrized by a string or number literal, with a
-syntax like a function call.  A component may include a "*"
-character, to expand to other matching probe points.  A probe point
-may be followed by a "?" character, to indicate that it is optional,
-and that no error should result if it fails to expand.  Optionalness
-passes down through all levels of alias/wildcard expansion.
+syntax like a function call.  A component may include a "*" character,
+to expand to a set of matching probe points.  Probe aliases likewise
+expand to other probe points.  Each and every resulting probe point is
+normally resolved to some low-level system instrumentation facility
+(e.g., a kprobe address, marker, or a timer configuration), otherwise
+the elaboration phase will fail.
+.PP
+However, a probe point may be followed by a "?" character, to indicate
+that it is optional, and that no error should result if it fails to
+resolve.  Optionalness passes down through all levels of
+alias/wildcard expansion.  Alternately, a probe point may be followed
+by a "!" character, to indicate that it is both optional and
+sufficient.  (Think vaguely of the prolog cut operator.) If it does
+resolve, then no further probe points in the same comma-separated list
+will be resolved.  Therefore, the "!"  sufficiency mark only makes
+sense in a list of probe point alternatives.
 
 These are all syntactically valid probe points:
 
@@ -40,6 +51,7 @@ user.inode("/bin/vi").statement(0x2222)
 end
 syscall.*
 kernel.function("no_such_function") ?
+module("awol").function("no_such_function") !
 .ESAMPLE
 
 Probes may be broadly classified into "synchronous" and
@@ -87,11 +99,11 @@ The
 .IR error
 probe point is similar to the 
 .IR end
-probe, except that each such probe handler run when the session and
-after errors having occurred.  In such cases, "end" probes are
-skipped, but each "error" prober is still attempted.  This kind of
-probe can be used to clean up or emit a final gasp message.  It may
-also be numerically parametrized to set a sequence.
+probe, except that each such probe handler run when the session ends
+after errors have occurred.  In such cases, "end" probes are skipped,
+but each "error" prober is still attempted.  This kind of probe can be
+used to clean up or emit a "final gasp".  It may also be numerically
+parametrized to set a sequence.
 
 .SS NEVER
 The probe point
diff --git a/staptree.cxx b/staptree.cxx
index d71472a6..8b702306 100644
--- a/staptree.cxx
+++ b/staptree.cxx
@@ -75,12 +75,12 @@ symboldecl::~symboldecl ()
 
 probe_point::probe_point (std::vector<component*> const & comps,
 			  const token * t):
-  components(comps), tok(t), optional (false)
+  components(comps), tok(t), optional (false), sufficient (false)
 {
 }
 
 probe_point::probe_point ():
-  tok (0), optional (false)
+  tok (0), optional (false), sufficient (false)
 {
 }
 
@@ -530,7 +530,7 @@ print_format::string_to_components(string const & str)
 	break;
 
       // Now we are definitely parsing a conversion. 
-      // Begin by parsing flags (whicih are optional).
+      // Begin by parsing flags (which are optional).
 
       switch (*i)
 	{
@@ -908,7 +908,9 @@ void probe_point::print (ostream& o) const
       if (c->arg)
         o << "(" << *c->arg << ")";
     }
-  if (optional)
+  if (sufficient)
+    o << "!";
+  else if (optional) // sufficient implies optional
     o << "?";
 }
 
@@ -923,7 +925,9 @@ string probe_point::str ()
       if (c->arg)
         o << "(" << *c->arg << ")";
     }
-  if (optional)
+  if (sufficient)
+    o << "!";
+  else if (optional) // sufficient implies optional
     o << "?";
   return o.str();
 }
@@ -944,7 +948,7 @@ void probe_alias::printsig (ostream& o) const
   o << " = ";
   for (unsigned i=0; i<locations.size(); i++)
     {
-      o << (i>0 ? ", " : "");
+      if (i > 0) o << ", ";
       locations[i]->print (o);
     }
 }
diff --git a/staptree.h b/staptree.h
index 23f43566..d8c97633 100644
--- a/staptree.h
+++ b/staptree.h
@@ -573,6 +573,7 @@ struct probe_point
   std::vector<component*> components;
   const token* tok; // points to first component's functor
   bool optional;
+  bool sufficient;
   void print (std::ostream& o) const;
   probe_point ();
   probe_point(std::vector<component*> const & comps,const token * t);
diff --git a/testsuite/ChangeLog b/testsuite/ChangeLog
index bc19c015..075245aa 100644
--- a/testsuite/ChangeLog
+++ b/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2007-11-19  Frank Ch. Eigler  <fche@elastic.org>
+
+	* parseok/five.stp, semok/twentyseven.stp: Test "!" probe point flag.
+
 2007-11-15  David Smith  <dsmith@redhat.com>
 
 	* systemtap.base/marker.exp: Removed 'module("foo").mark("bar")'
diff --git a/testsuite/parseok/five.stp b/testsuite/parseok/five.stp
index 29ff5990..e1b5d94a 100755
--- a/testsuite/parseok/five.stp
+++ b/testsuite/parseok/five.stp
@@ -18,3 +18,4 @@ probe perfcounter("tlbmiss").count(4000) {}
 probe resource.freemembelow(50) {} # pages?
 probe begin {}
 probe something?, or?, nothing? {}
+probe something!, or, nothing!, and?, zoo {}
diff --git a/testsuite/semok/twentyseven.stp b/testsuite/semok/twentyseven.stp
new file mode 100755
index 00000000..def633a5
--- /dev/null
+++ b/testsuite/semok/twentyseven.stp
@@ -0,0 +1,11 @@
+#! stap -p2
+
+probe foo.a = kernel.function("sys_read") /* some */ ! /* sufficient */ ,
+              kernel.function("no_such_function")
+      { "foo.a" }
+
+probe foo.b = module("*scsi*").function("no_such_thing") /* none */ ? ,
+      	      module("*scsi*").function("*queue*") /* some */ ! , /* suff'nt */
+	      module("no_such_module").function("no_such_function") 
+      { "foo.b" }
+probe foo.* { }