[ChangeLog]

2005-11-28  Graydon Hoare  <graydon@redhat.com>

	* translate.cxx (var::assert_hist_compatible): New method.
	(var::hist): New method.
	(c_unparser::load_aggregate): New method.
	(hist_op_downcaster): Remove, it was a mistake.
	(expression_is_hist_op): Likewise.
	(c_tmpcounter::visit_print_format): Implement print(@hist(...)).
	(c_unparser::visit_print_format): Likewise.

	* staptree.h (struct print_format): Add optional hist_op* member.
	* staptree.cxx (traversing_visitor::visit_functioncall): Visit
	hist_op if present in print_format.
	(deep_copy_visitor::visit_print_format): Likewise.

	* parse.cxx (parse_symbol): Special case to consume print(@hist(...)).

	* elaborate.cxx (typeresolution_info::visit_arrayindex): Fix type inference bug.
	(typeresolution_info::visit_foreach_loop): Likewise.

	* testsuite/buildok/print_histograms.stp: New test.

[runtime/ChangeLog]

2005-11-28  Graydon Hoare  <graydon@redhat.com>

	* stat-common.c (_stp_stat_print_histogram): Various formatting
	corrections and aesthetic tweaks.
	(__stp_stat_add): Correction to linear bucket underflow cases.

9 files changed, 252 insertions, 123 deletions

diff --git a/ChangeLog b/ChangeLog
index a0fe764f..353c8e6b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,25 @@
+2005-11-28  Graydon Hoare  <graydon@redhat.com>
+
+	* translate.cxx (var::assert_hist_compatible): New method.
+	(var::hist): New method.
+	(c_unparser::load_aggregate): New method.
+	(hist_op_downcaster): Remove, it was a mistake.
+	(expression_is_hist_op): Likewise.
+	(c_tmpcounter::visit_print_format): Implement print(@hist(...)).
+	(c_unparser::visit_print_format): Likewise.
+
+	* staptree.h (struct print_format): Add optional hist_op* member.
+	* staptree.cxx (traversing_visitor::visit_functioncall): Visit
+	hist_op if present in print_format.
+	(deep_copy_visitor::visit_print_format): Likewise.
+
+	* parse.cxx (parse_symbol): Special case to consume print(@hist(...)).
+
+	* elaborate.cxx (typeresolution_info::visit_arrayindex): Fix type inference bug.
+	(typeresolution_info::visit_foreach_loop): Likewise.
+
+	* testsuite/buildok/print_histograms.stp: New test.
+
 2005-11-28  Frank Ch. Eigler  <fche@redhat.com>
 
 	* translate.cxx (c_tmpcounter_assignment::visit_symbol): Don't
diff --git a/elaborate.cxx b/elaborate.cxx
index de469a4f..ac4bd0f6 100644
--- a/elaborate.cxx
+++ b/elaborate.cxx
@@ -1593,7 +1593,7 @@ typeresolution_info::visit_arrayindex (arrayindex* e)
       e->indexes[0]->visit (this);
       if (e->indexes[0]->type != pe_long)
 	unresolved (e->tok);
-      hist->stat->visit (this);
+      hist->visit (this);
       return;
     }
 
@@ -1759,7 +1759,7 @@ typeresolution_info::visit_foreach_loop (foreach_loop* e)
       e->indexes[0]->visit (this);
       if (e->indexes[0]->type != pe_long)
 	unresolved (e->tok);
-      hist->stat->visit (this);
+      hist->visit (this);
     }
   else
     {
diff --git a/parse.cxx b/parse.cxx
index 628fe420..fe10c75c 100644
--- a/parse.cxx
+++ b/parse.cxx
@@ -1988,6 +1988,19 @@ parser::parse_symbol ()
 		  fmt->args.push_back(e);
 		}
 	    }
+	  else if (name == "print" && 
+		   (peek_kw("@hist_linear") || 
+		    peek_kw("@hist_log")))
+	    {
+	      // We have a special case where we recognize
+	      // print(@hist_foo(bar)) as a magic print-the-histogram
+	      // construct. This is sort of gross but it avoids
+	      // promoting histogram references to typeful
+	      // expressions.
+	      fmt->hist = NULL;
+	      t = parse_hist_op_or_bare_name(fmt->hist, name);
+	      assert(fmt->hist);
+	    }
 	  else
 	    {
 	      // If we are not printing with a format string, we permit
diff --git a/runtime/ChangeLog b/runtime/ChangeLog
index 5d2c0532..f05012ca 100644
--- a/runtime/ChangeLog
+++ b/runtime/ChangeLog
@@ -1,3 +1,9 @@
+2005-11-28  Graydon Hoare  <graydon@redhat.com>
+
+	* stat-common.c (_stp_stat_print_histogram): Various formatting
+	corrections and aesthetic tweaks.
+	(__stp_stat_add): Correction to linear bucket underflow cases.
+
 2005-11-28  Martin Hunt  <hunt@redhat.com>
 
 	* alloc.c (__stp_valloc_percpu): Use same code for up and smp.
diff --git a/runtime/stat-common.c b/runtime/stat-common.c
index aa30641e..94f1bdfa 100644
--- a/runtime/stat-common.c
+++ b/runtime/stat-common.c
@@ -98,17 +98,34 @@ static int msb64(int64_t val)
 
 static void _stp_stat_print_histogram (Hist st, stat *sd)
 {
-	int scale, i, j, val_space, cnt_space;
+	int scale, i, j, val_space, cnt_space, 
+		low_bucket = -1, high_bucket = 0;
 	int64_t val, v, max = 0;
 
 	if (st->type != HIST_LOG && st->type != HIST_LINEAR)
 		return;
-	
-	/* get the maximum value, for scaling */
-	for (i = 0; i < st->buckets; i++)
+
+	/* Get the maximum value, for scaling. Also calculate the low
+	   and high values to bound the reporting range. */
+	for (i = 0; i < st->buckets; i++) {
+		if (sd->histogram[i] > 0 && low_bucket == -1)
+			low_bucket = i;
+		if (sd->histogram[i] > 0)
+			high_bucket = i;
 		if (sd->histogram[i] > max)
 			max = sd->histogram[i];
-	
+	}
+
+	/* Touch up the bucket margin to show up to two zero-slots on
+	   either side of the data range, seems aesthetically pleasant. */
+	for (i = 0; i < 2; i++) {
+		if (low_bucket > 0)
+			low_bucket--;
+		
+		if (high_bucket < (st->buckets-1))
+			high_bucket++;
+	}
+
 	if (max <= HIST_WIDTH)
 		scale = 1;
 	else {
@@ -120,9 +137,9 @@ static void _stp_stat_print_histogram (Hist st, stat *sd)
 
 	cnt_space = needed_space (max);
 	if (st->type == HIST_LINEAR)
-		val_space = needed_space (st->start +  st->interval * (st->buckets - 1));
+		val_space = needed_space (st->start +  st->interval * high_bucket);
 	else
-		val_space = needed_space (1 << (st->buckets - 1));
+		val_space = needed_space (((int64_t)1) << high_bucket);
 	//dbug ("max=%lld scale=%d val_space=%d\n", max, scale, val_space);
 
 	/* print header */
@@ -142,17 +159,19 @@ static void _stp_stat_print_histogram (Hist st, stat *sd)
 	else
 		val = 0;
 	for (i = 0; i < st->buckets; i++) {
-		reprint (val_space - needed_space(val), " ");
-		_stp_printf("%d", val);
-		_stp_print_cstr (" |");
-
-		/* v = s->histogram[i] / scale; */
-		v = sd->histogram[i];
-		do_div (v, scale);
+		if (i >= low_bucket && i <= high_bucket) {
+			reprint (val_space - needed_space(val), " ");
+			_stp_printf("%lld", val);
+			_stp_print_cstr (" |");
+			
+			/* v = s->histogram[i] / scale; */
+			v = sd->histogram[i];
+			do_div (v, scale);
 		
-		reprint (v, "@");
-		reprint (HIST_WIDTH - v + 1 + cnt_space - needed_space(sd->histogram[i]), " ");
-		_stp_printf ("%lld\n", sd->histogram[i]);
+			reprint (v, "@");
+			reprint (HIST_WIDTH - v + 1 + cnt_space - needed_space(sd->histogram[i]), " ");
+			_stp_printf ("%lld\n", sd->histogram[i]);
+		}
 		if (st->type == HIST_LINEAR) 
 			val += st->interval;
 		else if (val == 0)
@@ -217,14 +236,14 @@ static void __stp_stat_add (Hist st, stat *sd, int64_t val)
 		sd->histogram[n]++;
 		break;
 	case HIST_LINEAR:
-		val -= st->start;
+		if (val < st->start)
+			val = st->start;
+		else
+			val -= st->start;
 		do_div (val, st->interval);
-		n = val;
-		if (n < 0)
-			n = 0;
-		if (n >= st->buckets)
-			n = st->buckets - 1;
-		sd->histogram[n]++;
+		if (val >= st->buckets)
+			val = st->buckets - 1;
+		sd->histogram[val]++;
 	default:
 		break;
 	}
diff --git a/staptree.cxx b/staptree.cxx
index 6f75e09b..3015c796 100644
--- a/staptree.cxx
+++ b/staptree.cxx
@@ -1391,6 +1391,8 @@ traversing_visitor::visit_print_format (print_format* e)
 {
   for (unsigned i=0; i<e->args.size(); i++)
     e->args[i]->visit (this);
+  if (e->hist)
+    e->hist->visit(this);
 }
 
 void
@@ -1942,6 +1944,8 @@ deep_copy_visitor::visit_print_format (print_format* e)
       require <expression*> (this, &na, e->args[i]);
       n->args.push_back(na);
     }
+  if (e->hist)
+    require <hist_op*> (this, &n->hist, e->hist);
   provide <print_format*> (this, n);
 }
 
diff --git a/staptree.h b/staptree.h
index 5c98507a..b5420693 100644
--- a/staptree.h
+++ b/staptree.h
@@ -306,8 +306,13 @@ struct print_format: public expression
     }
   };
 
+  print_format() 
+    : hist(NULL) 
+  {}
+
   std::vector<format_component> components;
   std::vector<expression*> args;
+  hist_op *hist;
 
   static std::string components_to_string(std::vector<format_component> const & components);
   static std::vector<format_component> string_to_components(std::string const & str);
diff --git a/testsuite/buildok/print_histograms.stp b/testsuite/buildok/print_histograms.stp
new file mode 100755
index 00000000..70c6e68d
--- /dev/null
+++ b/testsuite/buildok/print_histograms.stp
@@ -0,0 +1,28 @@
+#! stap -p4
+
+global foo, bar
+global i
+
+probe begin
+{
+	print("starting up\n")
+	i = 0
+}
+
+probe timer.jiffies(100)
+{
+	printf("ping %d\n", i)
+	foo <<< ((100 * i) + 50)
+	bar <<< (50 * i * i)
+	if (i++ > 15)
+		exit()
+}
+
+probe end
+{
+	print("shutting down\n")
+	printf("count %d, avg %d\n", @count(foo), @avg(foo))
+	print(@hist_log(foo))
+	print(@hist_linear(bar, 1000, 10000, 1000))
+}
+
diff --git a/translate.cxx b/translate.cxx
index 2035477e..67c79c4b 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -116,6 +116,8 @@ struct c_unparser: public unparser, public visitor
   void load_map_indices(arrayindex* e,
 			vector<tmpvar> & idx);
 
+  void load_aggregate (expression *e, aggvar & agg);
+
   void collect_map_index_types(vector<vardecl* > const & vars,
 			       set< pair<vector<exp_type>, exp_type> > & types);
 
@@ -299,6 +301,29 @@ public:
     return sd;
   }
 
+  void assert_hist_compatible(hist_op const & hop)
+  {
+    // Semantic checks in elaborate should have caught this if it was
+    // false. This is just a double-check.
+    switch (sd.type)
+      {
+      case statistic_decl::linear:
+	assert(hop.htype == hist_linear);
+	assert(hop.params.size() == 3);
+	assert(hop.params[0] == sd.linear_low);
+	assert(hop.params[1] == sd.linear_high);
+	assert(hop.params[2] == sd.linear_step);
+	break;
+      case statistic_decl::logarithmic:
+	assert(hop.htype == hist_log);
+	assert(hop.params.size() == 1);
+	assert(hop.params[0] == sd.logarithmic_buckets);
+	break;
+      case statistic_decl::none:
+	assert(false);
+      }
+  }
+
   exp_type type() const
   {
     return ty;
@@ -312,6 +337,12 @@ public:
       return "global_" + name;
   }
 
+  string hist() const
+  {
+    assert (ty == pe_stats);
+    return "&(" + qname() + "->hist)";
+  }
+
   string init() const
   {
     switch (type())
@@ -2510,6 +2541,67 @@ c_unparser::load_map_indices(arrayindex *e,
 }
 
 
+struct arrayindex_downcaster
+  : public traversing_visitor
+{
+  arrayindex *& arr;
+  
+  arrayindex_downcaster (arrayindex *& arr)
+    : arr(arr) 
+  {}
+
+  void visit_arrayindex (arrayindex* e)
+  {
+    arr = e;
+  }
+};
+
+
+static bool
+expression_is_arrayindex (expression *e, 
+			  arrayindex *& hist)
+{
+  arrayindex *h = NULL;
+  arrayindex_downcaster d(h);
+  e->visit (&d);
+  if (static_cast<void*>(h) == static_cast<void*>(e))
+    {
+      hist = h;
+      return true;
+    }
+  return false;
+}
+
+
+void 
+c_unparser::load_aggregate (expression *e, aggvar & agg)
+{
+  symbol *sym = get_symbol_within_expression (e);
+  
+  if (sym->referent->type != pe_stats)
+    throw semantic_error ("unexpected aggregate of non-statistic", sym->tok);
+  
+  var v = getvar(sym->referent, e->tok);
+
+  if (sym->referent->arity == 0)
+    {
+      o->newline() << "c->last_stmt = " << lex_cast_qstring(*sym->tok) << ";";
+      o->newline() << agg << " = _stp_stat_get (" << v << ", 0);";	  
+    }
+  else
+    {
+      arrayindex *arr = NULL;
+      if (!expression_is_arrayindex (e, arr))
+	throw semantic_error("unexpected aggregate of non-arrayindex", e->tok);
+      
+      vector<tmpvar> idx;
+      load_map_indices (arr, idx);
+      mapvar mvar = getmap (sym->referent, sym->tok);
+      o->newline() << "c->last_stmt = " << lex_cast_qstring(*sym->tok) << ";";
+      o->newline() << agg << " = " << mvar.get(idx) << ";";
+    }
+}
+
 void
 c_unparser::visit_arrayindex (arrayindex* e)
 {
@@ -2788,45 +2880,31 @@ c_unparser::visit_functioncall (functioncall* e)
                  << ".__retvalue;";
 }
 
-struct hist_op_downcaster
-  : public traversing_visitor
-{
-  hist_op *& hist;
-
-  hist_op_downcaster (hist_op *& hist)
-    : hist(hist) 
-  {}
-
-  void visit_hist_op (hist_op* e)
-  {
-    hist = e;
-  }
-};
-
-static bool
-expression_is_hist_op (expression *e, 
-		       hist_op *& hist)
-{
-  hist_op *h = NULL;
-  hist_op_downcaster d(h);
-  e->visit (&d);
-  if (static_cast<void*>(h) == static_cast<void*>(e))
-    {
-      hist = h;
-      return true;
-    }
-  return false;
-}
-
-
 void
 c_tmpcounter::visit_print_format (print_format* e)
 {
-  hist_op *hist;
-  if ((!e->print_with_format) &&
-      (e->args.size() == 1) &&
-      expression_is_hist_op (e->args[0], hist))
+  if (e->hist)
     {
+      symbol *sym = get_symbol_within_expression (e->hist->stat);
+      var v = parent->getvar(sym->referent, sym->tok);
+      aggvar agg = parent->gensym_aggregate ();
+
+      agg.declare(*(this->parent));
+
+      if (sym->referent->arity != 0)
+	{
+	  // One temporary per index dimension.
+	  for (unsigned i=0; i<sym->referent->index_types.size(); i++)
+	    {
+	      arrayindex *arr = NULL;
+	      if (!expression_is_arrayindex (e->hist->stat, arr))
+		throw semantic_error("expected arrayindex expression in printed hist_op", e->tok);
+	      
+	      tmpvar ix = parent->gensym (sym->referent->index_types[i]);
+	      ix.declare (*parent);
+	      arr->indexes[i]->visit(this);
+	    }
+	}
     }
   else
     {
@@ -2859,13 +2937,18 @@ c_unparser::visit_print_format (print_format* e)
   // type of argument which gets its own processing: a single,
   // non-format-string'ed, histogram-type stat_op expression.
 
-  hist_op *hist;
-  if ((!e->print_with_format) &&
-      (e->args.size() == 1) &&
-      expression_is_hist_op (e->args[0], hist))
+  if (e->hist)
     {
-      // FIXME: fill in some logic here!
-      assert(false);
+      stmt_expr block(*this);  
+      symbol *sym = get_symbol_within_expression (e->hist->stat);
+      aggvar agg = gensym_aggregate ();
+      var v = getvar(sym->referent, e->tok);
+      v.assert_hist_compatible(*e->hist);
+
+      varlock_w guard(*this, v);
+      load_aggregate(e->hist->stat, agg);
+      o->newline() << "c->last_stmt = " << lex_cast_qstring(*e->tok) << ";";
+      o->newline() << "_stp_stat_print_histogram (" << v.hist() << ", " << agg.qname() << ");";
     }
   else
     {
@@ -2941,35 +3024,6 @@ c_unparser::visit_print_format (print_format* e)
     }
 }
 
-struct arrayindex_downcaster
-  : public traversing_visitor
-{
-  arrayindex *& arr;
-  
-  arrayindex_downcaster (arrayindex *& arr)
-    : arr(arr) 
-  {}
-
-  void visit_arrayindex (arrayindex* e)
-  {
-    arr = e;
-  }
-};
-
-static bool
-expression_is_arrayindex (expression *e, 
-			  arrayindex *& hist)
-{
-  arrayindex *h = NULL;
-  arrayindex_downcaster d(h);
-  e->visit (&d);
-  if (static_cast<void*>(h) == static_cast<void*>(e))
-    {
-      hist = h;
-      return true;
-    }
-  return false;
-}
 
 void 
 c_tmpcounter::visit_stat_op (stat_op* e)
@@ -3016,38 +3070,16 @@ c_unparser::visit_stat_op (stat_op* e)
   // FIXME: also note that summarizing anything is expensive, and we
   // really ought to pass a timeout handler into the summary routine,
   // check its response, possibly exit if it ran out of cycles.
-
-  symbol *sym = get_symbol_within_expression (e->stat);
-
-  if (sym->referent->type != pe_stats)
-    throw semantic_error ("non-statistic value in statistic operator context", sym->tok);
   
   {
-    stmt_expr block(*this);  
+    stmt_expr block(*this);
+    symbol *sym = get_symbol_within_expression (e->stat);
     aggvar agg = gensym_aggregate ();
-    tmpvar res = gensym (pe_long);      
-    
+    tmpvar res = gensym (pe_long);    
+    var v = getvar(sym->referent, e->tok);
     {
-      var v = getvar(sym->referent, e->tok);
       varlock_w guard(*this, v);
-      
-      if (sym->referent->arity == 0)
-	{
-	  o->newline() << "c->last_stmt = " << lex_cast_qstring(*sym->tok) << ";";
-	  o->newline() << agg << " = _stp_stat_get (" << v << ", 0);";	  
-	}
-      else
-	{
-	  arrayindex *arr = NULL;
-	  if (!expression_is_arrayindex (e->stat, arr))
-	    throw semantic_error("expected arrayindex expression in stat_op of array", e->tok);
-	  
-	  vector<tmpvar> idx;
-	  load_map_indices (arr, idx);
-	  mapvar mvar = getmap (sym->referent, sym->tok);
-	  o->newline() << "c->last_stmt = " << lex_cast_qstring(*sym->tok) << ";";
-	  o->newline() << agg << " = " << mvar.get(idx) << ";";
-	}
+      load_aggregate(e->stat, agg);
     
       switch (e->ctype)
 	{