PR 4936: probe pont conditions part 2; reorg in prep for full rewriting

2008-01-17  Frank Ch. Eigler  <fche@elastic.org>

	PR 4935.
	Reorganize probe condition implementation.
	* elaborate.cxx (add_condition): New function.
	(derived_probe): Remove condition member.
	(derived_probe ctors): Assert non-null incoming probe/location ptrs.
	(insert_condition_statement): Remove; turn into ...
	(semantic_pass_conditions): New pass-2 subpass.
	(semantic_pass_symbols, visit_symbol, visit_functioncall, find_var):
	Detect some condition-related error cases.
	(match_key): Change type to exp_type from tok_type.  Update callers.
	(alias_expansion_builder): Propagate probe conditions.
	* staptree.cxx (probe): Remove condition field and related functions.
	* tapsets.cxx (dwarf_derived_probe ctor): Compute replacement
	wildcard-expanded probe_point preserving more of the original
	location.
	(mark_derived_probe ctor): Make similar to others - take location
	rather than condition parameters.
	* translate.cxx (emit_common_header): Tweak ordering of tmpcounter
	traversal and hashkey expression generation.
	* elaborate.h: Corresponding changes.

2008-01-17  Frank Ch. Eigler  <fche@elastic.org>

	PR 4935.
	* semko/forty.stp, fortyone.stp, fortytwo.stp: New tests.
	* semok/twentynine.stp: Weaken test since condition expressions have
	become more tightly constrained.

12 files changed, 291 insertions, 173 deletions

diff --git a/ChangeLog b/ChangeLog
index 71cb2001..658ab44c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,26 @@
+2008-01-17  Frank Ch. Eigler  <fche@elastic.org>
+
+	PR 4935.
+	Reorganize probe condition implementation.
+	* elaborate.cxx (add_condition): New function.
+	(derived_probe): Remove condition member.
+	(derived_probe ctors): Assert non-null incoming probe/location ptrs.
+	(insert_condition_statement): Remove; turn into ...
+	(semantic_pass_conditions): New pass-2 subpass.
+	(semantic_pass_symbols, visit_symbol, visit_functioncall, find_var):
+	Detect some condition-related error cases.
+	(match_key): Change type to exp_type from tok_type.  Update callers.
+	(alias_expansion_builder): Propagate probe conditions.
+	* staptree.cxx (probe): Remove condition field and related functions.
+	* tapsets.cxx (dwarf_derived_probe ctor): Compute replacement
+	wildcard-expanded probe_point preserving more of the original
+	location.
+	(mark_derived_probe ctor): Make similar to others - take location
+	rather than condition parameters.
+	* translate.cxx (emit_common_header): Tweak ordering of tmpcounter
+	traversal and hashkey expression generation.
+	* elaborate.h: Corresponding changes.
+
 2008-01-17  David Smith  <dsmith@redhat.com>
 
 	* tapsets.cxx
diff --git a/elaborate.cxx b/elaborate.cxx
index 0ac0f4da..39dae294 100644
--- a/elaborate.cxx
+++ b/elaborate.cxx
@@ -33,60 +33,49 @@ using namespace std;
 
 // ------------------------------------------------------------------------
 
+// Used in probe_point condition construction.  Either argument may be
+// NULL; if both, return NULL too.  Resulting expression is a deep
+// copy for symbol resolution purposes.
+expression* add_condition (expression* a, expression* b)
+{
+  if (!a && !b) return 0;
+  if (! a) return deep_copy_visitor::deep_copy(b);
+  if (! b) return deep_copy_visitor::deep_copy(a);
+  logical_and_expr la;
+  la.op = "&&";
+  la.left = a;
+  la.right = b;
+  la.tok = a->tok; // or could be b->tok
+  return deep_copy_visitor::deep_copy(& la);
+}
+
+// ------------------------------------------------------------------------
+
+
 
 derived_probe::derived_probe (probe *p):
   base (p)
 {
-  if (p)
-    {
-      this->locations = p->locations;
-      this->condition = deep_copy_visitor::deep_copy(p->condition);
-      this->tok = p->tok;
-      this->privileged = p->privileged;
-      this->body = deep_copy_visitor::deep_copy(p->body);
-    }
+  assert (p);
+  this->locations = p->locations;
+  this->tok = p->tok;
+  this->privileged = p->privileged;
+  this->body = deep_copy_visitor::deep_copy(p->body);
 }
 
 
 derived_probe::derived_probe (probe *p, probe_point *l):
   base (p)
 {
-  if (p)
-    {
-      if (p->condition) 
-        this->condition = deep_copy_visitor::deep_copy(p->condition);
-      this->tok = p->tok;
-      this->privileged = p->privileged;
-      this->body = deep_copy_visitor::deep_copy(p->body);
-    }
+  assert (p);
+  this->tok = p->tok;
+  this->privileged = p->privileged;
+  this->body = deep_copy_visitor::deep_copy(p->body);
 
-  if (l)
-    {
-      probe_point *pp = new probe_point (l->components, l->tok);
-      this->locations.push_back (pp);
-      this->add_condition (l->condition);
-      this->insert_condition_statement ();
-    }
+  assert (l);
+  this->locations.push_back (l);
 }
 
-void
-derived_probe::insert_condition_statement (void)
-{
-  if (this->condition)
-    {
-      if_statement *ifs = new if_statement ();
-      ifs->tok = this->tok;
-      ifs->thenblock = new next_statement ();
-      ifs->thenblock->tok = this->tok;
-      ifs->elseblock = NULL;
-      unary_expression *notex = new unary_expression ();
-      notex->op = "!";
-      notex->tok = this->tok;
-      notex->operand = this->condition;
-      ifs->condition = notex;
-      body->statements.insert (body->statements.begin(), ifs);
-    }
-}
 
 void
 derived_probe::printsig (ostream& o) const
@@ -196,14 +185,14 @@ derived_probe_builder::has_null_param (std::map<std::string, literal*> const & p
 match_key::match_key(string const & n) 
   : name(n), 
     have_parameter(false), 
-    parameter_type(tok_junk)
+    parameter_type(pe_unknown)
 {
 }
 
 match_key::match_key(probe_point::component const & c)
   : name(c.functor),
     have_parameter(c.arg != NULL),
-    parameter_type(c.arg ? c.arg->tok->type : tok_junk)
+    parameter_type(c.arg ? c.arg->type : pe_unknown)
 {
 }
 
@@ -211,7 +200,7 @@ match_key &
 match_key::with_number() 
 {
   have_parameter = true;
-  parameter_type = tok_number;
+  parameter_type = pe_long;
   return *this;
 }
 
@@ -219,7 +208,7 @@ match_key &
 match_key::with_string() 
 {
   have_parameter = true;
-  parameter_type = tok_string;
+  parameter_type = pe_string;
   return *this;
 }
 
@@ -229,8 +218,8 @@ match_key::str() const
   if (have_parameter)
     switch (parameter_type)
       {
-      case tok_string: return name + "(string)";
-      case tok_number: return name + "(number)";
+      case pe_string: return name + "(string)";
+      case pe_long: return name + "(number)";
       default: return name + "(...)";
       }
   return name;
@@ -371,6 +360,10 @@ match_node::find_and_build (systemtap_session& s,
 	      non_wildcard_component->functor = subkey.name;
 	      non_wildcard_pp->components[pos] = non_wildcard_component;
 
+              // NB: probe conditions are not attached at the wildcard
+              // (component/functor) level, but at the overall
+              // probe_point level.
+
 	      // recurse (with the non-wildcard probe point)
 	      try
 	        {
@@ -439,7 +432,7 @@ match_node::build_no_more (systemtap_session& s)
 
 struct alias_derived_probe: public derived_probe
 {
-  alias_derived_probe (probe* base): derived_probe (base) {}
+  alias_derived_probe (probe* base, probe_point *l): derived_probe (base, l) {}
 
   void upchuck () { throw semantic_error ("inappropriate", this->tok); }
 
@@ -471,17 +464,18 @@ alias_expansion_builder
     // alias_expansion_probe so that the expansion loop recognizes it as
     // such and re-expands its expansion.
     
-    probe * n = new alias_derived_probe (use);
+    alias_derived_probe * n = new alias_derived_probe (use, location /* soon overwritten */);
     n->body = new block();
 
-    // The new probe gets the location list of the alias,
+    // The new probe gets the location list of the alias (with incoming condition joined)
     n->locations = alias->locations;
-  
+    for (unsigned i=0; i<n->locations.size(); i++)
+      n->locations[i]->condition = add_condition (n->locations[i]->condition,
+                                                  location->condition);
+
     // the token location of the alias,
     n->tok = location->tok;
     n->body->tok = location->tok;
-    // The new probe takes over condition.
-    n->add_condition (location->condition);
 
     // and statements representing the concatenation of the alias'
     // body with the use's. 
@@ -813,40 +807,6 @@ struct no_var_mutation_during_iteration_check
 };
 
 
-static int
-semantic_pass_vars (systemtap_session & sess)
-{
-  
-  map<functiondecl *, set<vardecl *> *> fmv;
-  no_var_mutation_during_iteration_check chk(sess, fmv);
-  
-  for (unsigned i = 0; i < sess.functions.size(); ++i)
-    {
-      functiondecl * fn = sess.functions[i];
-      if (fn->body)
-	{
-	  set<vardecl *> * m = new set<vardecl *>();
-	  mutated_var_collector mc (m);
-	  fn->body->visit (&mc);
-	  fmv[fn] = m;
-	}
-    }
-
-  for (unsigned i = 0; i < sess.functions.size(); ++i)
-    {
-      if (sess.functions[i]->body)
-	sess.functions[i]->body->visit (&chk);
-    }
-
-  for (unsigned i = 0; i < sess.probes.size(); ++i)
-    {
-      if (sess.probes[i]->body)
-	sess.probes[i]->body->visit (&chk);
-    }  
-
-  return sess.num_errors();
-}
-
 // ------------------------------------------------------------------------
 
 struct stat_decl_collector
@@ -950,6 +910,106 @@ semantic_pass_stats (systemtap_session & sess)
 
 // ------------------------------------------------------------------------
 
+// Enforce variable-related invariants: no modification of
+// a foreach()-iterated array.
+static int
+semantic_pass_vars (systemtap_session & sess)
+{
+  
+  map<functiondecl *, set<vardecl *> *> fmv;
+  no_var_mutation_during_iteration_check chk(sess, fmv);
+  
+  for (unsigned i = 0; i < sess.functions.size(); ++i)
+    {
+      functiondecl * fn = sess.functions[i];
+      if (fn->body)
+	{
+	  set<vardecl *> * m = new set<vardecl *>();
+	  mutated_var_collector mc (m);
+	  fn->body->visit (&mc);
+	  fmv[fn] = m;
+	}
+    }
+
+  for (unsigned i = 0; i < sess.functions.size(); ++i)
+    {
+      if (sess.functions[i]->body)
+	sess.functions[i]->body->visit (&chk);
+    }
+
+  for (unsigned i = 0; i < sess.probes.size(); ++i)
+    {
+      if (sess.probes[i]->body)
+	sess.probes[i]->body->visit (&chk);
+    }  
+
+  return sess.num_errors();
+}
+
+
+// ------------------------------------------------------------------------
+
+// Rewrite probe condition expressions into probe bodies.  Tricky and
+// exciting business, this.  This:
+//
+// probe foo if (g1 || g2) { ... }
+// probe bar { ... g1 ++ ... }
+//
+// becomes:
+//
+// probe begin(MAX) { if (! (g1 || g2)) %{ disable_probe_foo %} }
+// probe foo { if (! (g1 || g2)) next; ... }
+// probe bar { ... g1 ++ ...; 
+//             if (g1 || g2) %{ enable_probe_foo %} else %{ disable_probe_foo %}
+//           }
+//
+// XXX: As a first cut, do only the "inline probe condition" part of the
+// transform.
+
+static int
+semantic_pass_conditions (systemtap_session & sess)
+{
+  for (unsigned i = 0; i < sess.probes.size(); ++i)
+    {
+      derived_probe* p = sess.probes[i];
+      expression* e = p->sole_location()->condition;
+      if (e)
+        {
+          varuse_collecting_visitor vut;
+          e->visit (& vut);
+
+          if (! vut.written.empty())
+            {
+              string err = ("probe condition must not modify any variables");
+              sess.print_error (semantic_error (err, e->tok));
+            }
+          else if (vut.embedded_seen)
+            {
+              sess.print_error (semantic_error ("probe condition must not include impure embedded-C", e->tok));
+            }
+
+          // Add the condition expression to the front of the
+          // derived_probe body.
+          if_statement *ifs = new if_statement ();
+          ifs->tok = e->tok;
+          ifs->thenblock = new next_statement ();
+          ifs->thenblock->tok = e->tok;
+          ifs->elseblock = NULL;
+          unary_expression *notex = new unary_expression ();
+          notex->op = "!";
+          notex->tok = e->tok;
+          notex->operand = e;
+          ifs->condition = notex;
+          p->body->statements.insert (p->body->statements.begin(), ifs);
+        }
+    }  
+
+  return sess.num_errors();
+}
+
+
+// ------------------------------------------------------------------------
+
 
 static int semantic_pass_symbols (systemtap_session&);
 static int semantic_pass_optimize1 (systemtap_session&);
@@ -957,7 +1017,7 @@ static int semantic_pass_optimize2 (systemtap_session&);
 static int semantic_pass_types (systemtap_session&);
 static int semantic_pass_vars (systemtap_session&);
 static int semantic_pass_stats (systemtap_session&);
-
+static int semantic_pass_conditions (systemtap_session&);
 
 
 // Link up symbols to their declarations.  Set the session's
@@ -1030,6 +1090,12 @@ semantic_pass_symbols (systemtap_session& s)
                   sym.current_function = 0;
                   sym.current_probe = dp;
                   dp->body->visit (& sym);
+
+                  // Process the probe-point condition expression.
+                  sym.current_function = 0;
+                  sym.current_probe = 0;
+                  if (dp->sole_location()->condition)
+                    dp->sole_location()->condition->visit (& sym);
                 }
               catch (const semantic_error& e)
                 {
@@ -1058,7 +1124,8 @@ semantic_pass (systemtap_session& s)
       s.register_library_aliases();
       register_standard_tapsets(s);
       
-      rc = semantic_pass_symbols (s);
+      if (rc == 0) rc = semantic_pass_symbols (s);
+      if (rc == 0) rc = semantic_pass_conditions (s);
       if (rc == 0 && ! s.unoptimized) rc = semantic_pass_optimize1 (s);
       if (rc == 0) rc = semantic_pass_types (s);
       if (rc == 0 && ! s.unoptimized) rc = semantic_pass_optimize2 (s);
@@ -1248,8 +1315,8 @@ symresolution_info::visit_symbol (symbol* e)
       else if (current_probe)
         current_probe->locals.push_back (v);
       else
-        // must not happen
-        throw semantic_error ("no current probe/function", e->tok);
+        // must be probe-condition expression
+        throw semantic_error ("probe condition must not reference undeclared global", e->tok);
       e->referent = v;
     }
 }
@@ -1301,6 +1368,14 @@ symresolution_info::visit_arrayindex (arrayindex* e)
 void
 symresolution_info::visit_functioncall (functioncall* e)
 {
+  // XXX: we could relax this, if we're going to examine the
+  // vartracking data recursively.  See testsuite/semko/fortytwo.stp.
+  if (! (current_function || current_probe))
+    {
+      // must be probe-condition expression
+      throw semantic_error ("probe condition must not reference function", e->tok);
+    }
+
   for (unsigned i=0; i<e->args.size(); i++)
     e->args[i]->visit (this);
 
@@ -1323,20 +1398,22 @@ symresolution_info::visit_functioncall (functioncall* e)
 vardecl* 
 symresolution_info::find_var (const string& name, int arity)
 {
-
-  // search locals
-  vector<vardecl*>& locals = (current_function ? 
-                              current_function->locals :
-			      current_probe->locals);
-
-
-  for (unsigned i=0; i<locals.size(); i++)
-    if (locals[i]->name == name 
-	&& locals[i]->compatible_arity(arity))
-      {
-	locals[i]->set_arity (arity);
-	return locals[i];
-      }
+  if (current_function || current_probe)
+    {
+      // search locals
+      vector<vardecl*>& locals = (current_function ? 
+                                  current_function->locals :
+                                  current_probe->locals);
+      
+      
+      for (unsigned i=0; i<locals.size(); i++)
+        if (locals[i]->name == name 
+            && locals[i]->compatible_arity(arity))
+          {
+            locals[i]->set_arity (arity);
+            return locals[i];
+          }
+    }
 
   // search function formal parameters (for scalars)
   if (arity == 0 && current_function)
@@ -1430,7 +1507,11 @@ void semantic_pass_opt1 (systemtap_session& s, bool& relaxed_p)
 {
   functioncall_traversing_visitor ftv;
   for (unsigned i=0; i<s.probes.size(); i++)
-    s.probes[i]->body->visit (& ftv);
+    {
+      s.probes[i]->body->visit (& ftv);
+      if (s.probes[i]->sole_location()->condition)
+        s.probes[i]->sole_location()->condition->visit (& ftv);
+    }
   for (unsigned i=0; i<s.functions.size(); /* see below */)
     {
       if (ftv.traversed.find(s.functions[i]) == ftv.traversed.end())
@@ -1463,7 +1544,13 @@ void semantic_pass_opt2 (systemtap_session& s, bool& relaxed_p)
   varuse_collecting_visitor vut;
   
   for (unsigned i=0; i<s.probes.size(); i++)
-    s.probes[i]->body->visit (& vut);
+    {
+      s.probes[i]->body->visit (& vut);
+
+      if (s.probes[i]->sole_location()->condition)
+        s.probes[i]->sole_location()->condition->visit (& vut);
+    }
+
   // NB: Since varuse_collecting_visitor also traverses down
   // actually called functions, we don't need to explicitly
   // iterate over them.  Uncalled ones should have been pruned
@@ -1985,6 +2072,15 @@ semantic_pass_types (systemtap_session& s)
           ti.current_probe = pn;
           ti.t = pe_unknown;
           pn->body->visit (& ti);
+
+          probe_point* pp = pn->sole_location();
+          if (pp->condition)
+            {
+              ti.current_function = 0;
+              ti.current_probe = 0;
+              ti.t = pe_long; // NB: expected type
+              pp->condition->visit (& ti);
+            }
         }
 
       for (unsigned j=0; j<s.globals.size(); j++)
@@ -2853,7 +2949,7 @@ typeresolution_info::unresolved (const token* tok)
       stringstream msg;
       string nm = (current_function ? current_function->name :
                    current_probe ? current_probe->name :
-                   "?");
+                   "probe condition");
       msg << nm + " with unresolved type";
       session.print_error (semantic_error (msg.str(), tok));
     }
@@ -2870,7 +2966,7 @@ typeresolution_info::invalid (const token* tok, exp_type pe)
       stringstream msg;
       string nm = (current_function ? current_function->name :
                    current_probe ? current_probe->name :
-                   "?");
+                   "probe condition");
       if (tok && tok->type == tok_operator)
         msg << nm + " uses invalid operator";
       else
@@ -2890,7 +2986,7 @@ typeresolution_info::mismatch (const token* tok, exp_type t1, exp_type t2)
       stringstream msg;
       string nm = (current_function ? current_function->name :
                    current_probe ? current_probe->name :
-                   "?");
+                   "probe condition");
       msg << nm + " with type mismatch (" << t1 << " vs. " << t2 << ")";
       session.print_error (semantic_error (msg.str(), tok));
     }
diff --git a/elaborate.h b/elaborate.h
index 13246b86..5f396526 100644
--- a/elaborate.h
+++ b/elaborate.h
@@ -118,7 +118,6 @@ struct derived_probe: public probe
   virtual probe_point* sole_location () const;
   virtual void printsig (std::ostream &o) const;
   void printsig_nested (std::ostream &o) const;
-  void insert_condition_statement (void);
   virtual void collect_derivation_chain (std::vector<derived_probe*> &probes_list);
 
   virtual void emit_probe_context_vars (translator_output*) {}
@@ -197,7 +196,7 @@ match_key
 {
   std::string name;
   bool have_parameter;
-  token_type parameter_type;
+  exp_type parameter_type;
 
   match_key(std::string const & n);
   match_key(probe_point::component const & c);
diff --git a/staptree.cxx b/staptree.cxx
index 8cd9ca83..ed2bc00e 100644
--- a/staptree.cxx
+++ b/staptree.cxx
@@ -89,7 +89,7 @@ probe_point::probe_point ():
 
 unsigned probe::last_probeidx = 0;
 probe::probe ():
-  body (0), tok (0), condition (0)
+  body (0), tok (0)
 {
   this->name = string ("probe_") + lex_cast<string>(last_probeidx ++);
 }
@@ -899,26 +899,6 @@ probe::collect_derivation_chain (std::vector<derived_probe*> &probes_list)
   probes_list.push_back((derived_probe*)this);
 }
 
-void
-probe::add_condition (expression* e)
-{
-  if (e)
-    {
-      if (this->condition)
-        {
-          logical_and_expr *la = new logical_and_expr ();
-          la->op = "&&";
-          la->left = this->condition;
-          la->right = e;
-          la->tok = e->tok;
-          this->condition = la;
-        }
-      else
-        {
-          this->condition = e;
-        }
-    }
-}
 
 void probe_point::print (ostream& o) const
 {
diff --git a/staptree.h b/staptree.h
index a1a5ebd2..4584d32d 100644
--- a/staptree.h
+++ b/staptree.h
@@ -591,10 +591,8 @@ struct probe
   const token* tok;
   std::vector<vardecl*> locals;
   std::vector<vardecl*> unused_locals;
-  expression* condition;
   probe ();
   void print (std::ostream& o) const;
-  void add_condition (expression* e);
   virtual void printsig (std::ostream &o) const;
   virtual void collect_derivation_chain (std::vector<derived_probe*> &probes_list);
   virtual probe* basest () { return this; }
diff --git a/tapsets.cxx b/tapsets.cxx
index 84187960..937f34b3 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -3632,7 +3632,7 @@ dwarf_derived_probe::dwarf_derived_probe(const string& funcname,
                                          Dwarf_Addr addr,
                                          dwarf_query& q,
                                          Dwarf_Die* scope_die /* may be null */)
-  : derived_probe (q.base_probe, 0 /* location-less */),
+  : derived_probe (q.base_probe, q.base_loc /* NB: base_loc.components will be overwritten */ ),
     module (module), section (section), addr (addr),
     has_return (q.has_return),
     has_maxactive (q.has_maxactive),
@@ -3644,14 +3644,8 @@ dwarf_derived_probe::dwarf_derived_probe(const string& funcname,
   if (section != "" && dwfl_addr == addr) // addr should be an offset
     throw semantic_error ("inconsistent relocation address", q.base_loc->tok);
 
-
   this->tok = q.base_probe->tok;
 
-  // add condition from base location
-  if (q.base_loc->condition)
-  	add_condition (q.base_loc->condition);
-  insert_condition_statement ();
-
   // Make a target-variable-expanded copy of the probe body
   if (scope_die)
     {
@@ -3674,7 +3668,7 @@ dwarf_derived_probe::dwarf_derived_probe(const string& funcname,
     }
   // else - null scope_die - $target variables will produce an error during translate phase
 
-  // Set the sole element of the "locations" vector as a
+  // Reset the sole element of the "locations" vector as a
   // "reverse-engineered" form of the incoming (q.base_loc) probe
   // point.  This allows a user to see what function / file / line
   // number any particular match of the wildcards.
@@ -3727,7 +3721,8 @@ dwarf_derived_probe::dwarf_derived_probe(const string& funcname,
     comps.push_back (new probe_point::component
                      (TOK_MAXACTIVE, new literal_number(maxactive_val)));
 
-  locations.push_back(new probe_point(comps, q.base_loc->tok));
+  // Overwrite it.
+  this->sole_location()->components = comps;
 }
 
 
@@ -4578,6 +4573,8 @@ procfs_derived_probe_group::enroll (procfs_derived_probe* p)
 	throw semantic_error("only one write procfs probe can exist for procfs path \"" + p->path + "\"");
       else if (! p->write && pset->read_probe != NULL)
 	throw semantic_error("only one read procfs probe can exist for procfs path \"" + p->path + "\"");
+
+      // XXX: multiple writes should be acceptable
     }
 
   if (p->write)
@@ -4970,7 +4967,7 @@ struct mark_derived_probe: public derived_probe
 {
   mark_derived_probe (systemtap_session &s,
                       const string& probe_name, const string& probe_sig,
-                      probe* base_probe, expression* cond);
+                      probe* base_probe, probe_point* location);
 
   systemtap_session& sess;
   string probe_name, probe_sig;
@@ -5121,24 +5118,15 @@ mark_var_expanding_copy_visitor::visit_target_symbol (target_symbol* e)
 mark_derived_probe::mark_derived_probe (systemtap_session &s,
                                         const string& p_n,
                                         const string& p_s,
-                                        probe* base, expression* cond):
-  derived_probe (base, 0), sess (s), probe_name (p_n), probe_sig (p_s),
+                                        probe* base, probe_point* loc):
+  derived_probe (base, loc), sess (s), probe_name (p_n), probe_sig (p_s),
   target_symbol_seen (false)
 {
-  // create synthetic probe point
-  probe_point* pp = new probe_point;
-
-  probe_point::component* c;
-  c = new probe_point::component ("kernel");
-  pp->components.push_back (c);
-  c = new probe_point::component ("mark",
-                                  new literal_string (probe_name));
-  pp->components.push_back (c);
-  this->locations.push_back (pp);
-
-  if (cond)
-    add_condition (cond);
-  insert_condition_statement ();
+  // create synthetic probe point name; preserve condition
+  vector<probe_point::component*> comps;
+  comps.push_back (new probe_point::component ("kernel"));
+  comps.push_back (new probe_point::component ("mark", new literal_string (probe_name)));
+  this->sole_location()->components = comps;
 
   // expand the signature string
   parse_probe_sig();
@@ -5543,7 +5531,7 @@ mark_builder::build(systemtap_session & sess,
 	  derived_probe *dp
 	      = new mark_derived_probe (sess,
 					it->first, it->second,
-					base, loc->condition);
+					base, loc);
 	  finished_results.push_back (dp);
 	}
     }
diff --git a/testsuite/ChangeLog b/testsuite/ChangeLog
index 6dd0d8de..ac0eeebb 100644
--- a/testsuite/ChangeLog
+++ b/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2008-01-17  Frank Ch. Eigler  <fche@elastic.org>
+
+	PR 4935.
+	* semko/forty.stp, fortyone.stp, fortytwo.stp: New tests.
+	* semok/twentynine.stp: Weaken test since condition expressions have
+	become more tightly constrained.
+
 2008-01-17  David Smith  <dsmith@redhat.com>
 
 	* semko/procfs11.stp: Added test for invalid use of procfs probe
diff --git a/testsuite/semko/forty.stp b/testsuite/semko/forty.stp
new file mode 100755
index 00000000..f7721a47
--- /dev/null
+++ b/testsuite/semko/forty.stp
@@ -0,0 +1,4 @@
+#! stap -p2
+
+global x
+probe kernel.function("sys_open") if (x = 1) { }  # bad side-effect
diff --git a/testsuite/semko/fortyone.stp b/testsuite/semko/fortyone.stp
new file mode 100755
index 00000000..e9b986df
--- /dev/null
+++ b/testsuite/semko/fortyone.stp
@@ -0,0 +1,3 @@
+#! stap -p2
+
+probe kernel.function("sys_open") if (x > 1) { }  # not a global
diff --git a/testsuite/semko/fortytwo.stp b/testsuite/semko/fortytwo.stp
new file mode 100755
index 00000000..17dacb1c
--- /dev/null
+++ b/testsuite/semko/fortytwo.stp
@@ -0,0 +1,10 @@
+#! stap -p2
+
+probe kernel.function("sys_open") if (foo(2)) { }  # must not call functions
+
+function foo(x) { return x }
+
+# NB: If this condition is relaxed, then this will have to be blocked:
+#    global y function foo () { return y++ }   # since global y is written-to
+# but this one would be fine:
+#    function foo () { return y++ }   # since y is written-to
diff --git a/testsuite/semok/twentynine.stp b/testsuite/semok/twentynine.stp
index 6fe308f2..05e591ce 100644
--- a/testsuite/semok/twentynine.stp
+++ b/testsuite/semok/twentynine.stp
@@ -4,10 +4,10 @@ function dummy:long () {return p;}
 
 # alias with a condition
 probe alias0 = begin if (3) {p=1}
-# alias with a kernel-variable condition
-probe alias1 = kernel.function("sys_read").return if ($return) {p=0}
+# alias with a kernel-variable condition -- not valid
+probe alias1 = kernel.function("sys_read").return if (0) { if ($return) {p=0} }
 # alias with a function-call condition
-probe blias0 = timer.s(1) if (dummy()) {p=10}
+probe blias0 = timer.s(1) if (1 /* dummy() */) {p=10}
 
 # multiple probe point with conditions
 probe alias2 = alias0 if (1), alias1 if (-1) {p=2}
diff --git a/translate.cxx b/translate.cxx
index 7f42ceec..21a438b6 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -887,8 +887,12 @@ c_unparser::emit_common_header ()
       // NB: see c_unparser::emit_probe() for original copy of duplicate-hashing logic.
       ostringstream oss;
       oss << "c->statp = & time_" << dp->basest()->name << ";" << endl;  // -t anti-dupe
-      dp->body->print(oss);
       oss << "# needs_global_locks: " << dp->needs_global_locks () << endl;
+      dp->body->print(oss);
+      // NB: dependent probe conditions *could* be listed here, but don't need to be.
+      // That's because they're only dependent on the probe body, which is already
+      // "hashed" in above.
+
 
       if (tmp_probe_contents.count(oss.str()) == 0) // unique
         {
@@ -911,9 +915,13 @@ c_unparser::emit_common_header ()
                 throw e2;
               }
             }
+
+          // NB: This part is finicky.  The logic here must
+          // match up with 
           c_tmpcounter ct (this);
-          dp->body->visit (& ct);
           dp->emit_probe_context_vars (o);
+          dp->body->visit (& ct);
+
           o->newline(-1) << "} " << dp->name << ";";
         }
     }
@@ -1404,8 +1412,11 @@ c_unparser::emit_probe (derived_probe* v)
   //
   // which would make comparisons impossible.
   //
-  // NB: see also c_unparser:emit_common_header(), which duplicates
-  // this calculation.
+  // --------------------------------------------------------------------------
+  // NB: see also c_unparser:emit_common_header(), which deliberately but sadly
+  // duplicates this calculation.
+  // --------------------------------------------------------------------------
+  //
   ostringstream oss;
 
   // NB: statp is just for avoiding designation as duplicate.  It need not be C.
@@ -1503,13 +1514,12 @@ c_unparser::emit_probe (derived_probe* v)
         }
 
       v->initialize_probe_context_vars (o);
-  
+
       v->body->visit (this);
 
       o->newline(-1) << "out:";
-      // NB: no need to uninitialize locals, except if arrays can
-      // somedays be local 
-
+      // NB: no need to uninitialize locals, except if arrays/stats can
+      // someday be local 
 
       // XXX: do this flush only if the body included a
       // print/printf/etc. routine!