PR6538: warn about read-only variables

3 files changed, 68 insertions, 6 deletions

diff --git a/ChangeLog b/ChangeLog
index 34e4af1a..e3e67b33 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2008-05-20  Frank Ch. Eigler  <fche@elastic.org>
+
+	PR 6538
+	* elaborate.cxx (semantic_pass_opt2): Warn about read-only locals and
+	globals.
+	* staptree.cxx (varuse_collecting_visitor::visit_symbol): Recognize
+	initialized global as written-to.
+	(visit_foreach_loop): Belatedly recognize index symbols as lvalues.
+
 2008-05-20  Tim Moore  <timoore@redhat.com>
 
 	* configure.ac: Check for tr1/unordered_map header.
diff --git a/elaborate.cxx b/elaborate.cxx
index 4bc43832..f8bfabdb 100644
--- a/elaborate.cxx
+++ b/elaborate.cxx
@@ -1624,7 +1624,13 @@ void semantic_pass_opt2 (systemtap_session& s, bool& relaxed_p)
             // don't increment j
           }
         else
-          j++;
+          {
+            if (vut.written.find (l) == vut.written.end())
+              if (! s.suppress_warnings)
+                clog << "WARNING: read-only local variable " << *l->tok << endl;
+
+            j++;
+          }
       }
   for (unsigned i=0; i<s.functions.size(); i++)
     for (unsigned j=0; j<s.functions[i]->locals.size(); /* see below */)
@@ -1649,7 +1655,12 @@ void semantic_pass_opt2 (systemtap_session& s, bool& relaxed_p)
             // don't increment j
           }
         else
-          j++;
+          {
+            if (vut.written.find (l) == vut.written.end())
+              if (! s.suppress_warnings)
+                clog << "WARNING: read-only local variable " << *l->tok << endl;
+            j++;
+          }
       }
   for (unsigned i=0; i<s.globals.size(); /* see below */)
     {
@@ -1671,7 +1682,12 @@ void semantic_pass_opt2 (systemtap_session& s, bool& relaxed_p)
           // don't increment i
         }
       else
-        i++;
+        {
+          if (vut.written.find (l) == vut.written.end())
+            if (! s.suppress_warnings)
+              clog << "WARNING: read-only global variable " << *l->tok << endl;
+          i++;
+        }
     }
 }
 
@@ -1834,6 +1850,8 @@ void semantic_pass_opt3 (systemtap_session& s, bool& relaxed_p)
   for (unsigned i=0; i<s.functions.size(); i++)
     s.functions[i]->body->visit (& dar);
   // The rewrite operation is performed within the visitor.
+
+  // XXX: we could also zap write-only globals here
 }
 
 
@@ -2057,6 +2075,8 @@ void semantic_pass_opt4 (systemtap_session& s, bool& relaxed_p)
 
           p->body = new null_statement();
           p->body->tok = p->tok;
+
+          // XXX: possible duplicate warnings; see below
         }
     }
   for (unsigned i=0; i<s.functions.size(); i++)
@@ -2080,6 +2100,12 @@ void semantic_pass_opt4 (systemtap_session& s, bool& relaxed_p)
 
           fn->body = new null_statement();
           fn->body->tok = fn->tok;
+
+          // XXX: the next iteration of the outer optimization loop may
+          // take this new null_statement away again, and thus give us a
+          // fresh warning.  It would be better if this fixup was performed
+          // only after the relaxation iterations.
+          // XXX: or else see bug #6469.
         }
     }
 }
diff --git a/staptree.cxx b/staptree.cxx
index 02a6c8dc..b5cbd5c9 100644
--- a/staptree.cxx
+++ b/staptree.cxx
@@ -1713,6 +1713,10 @@ varuse_collecting_visitor::visit_symbol (symbol *e)
   if (e->referent == 0)
     throw semantic_error ("symbol without referent", e->tok);
 
+  // handle initialized globals
+  if (e->referent->init)
+    written.insert (e->referent);
+
   if (current_lvalue == e || current_lrvalue == e)
     {
       written.insert (e->referent);
@@ -1789,10 +1793,19 @@ varuse_collecting_visitor::visit_post_crement (post_crement *e)
 void
 varuse_collecting_visitor::visit_foreach_loop (foreach_loop* s)
 {
-  functioncall_traversing_visitor::visit_foreach_loop (s);
+  // NB: we duplicate so don't bother call
+  // functioncall_traversing_visitor::visit_foreach_loop (s);
+
+  symbol *array = NULL;  
+  hist_op *hist = NULL;
+  classify_indexable (s->base, array, hist);
+  if (array)
+    array->visit(this);
+  else
+    hist->visit(this);
+
   // If the collection is sorted, imply a "write" access to the
-  // array in addition to the "read" one already noted in the
-  // base class call above.
+  // array in addition to the "read" one already noted above.
   if (s->sort_direction)
     {
       symbol *array = NULL;  
@@ -1801,6 +1814,20 @@ varuse_collecting_visitor::visit_foreach_loop (foreach_loop* s)
       if (array) this->written.insert (array->referent);
       // XXX: Can hist_op iterations be sorted?
     }
+
+  // NB: don't forget to visit the index expressions, which are lvalues.
+  for (unsigned i=0; i<s->indexes.size(); i++)
+    {
+      expression* last_lvalue = current_lvalue;
+      current_lvalue = s->indexes[i]; // leave a mark for ::visit_symbol
+      s->indexes[i]->visit (this);
+      current_lvalue = last_lvalue;
+    }
+
+  if (s->limit)
+    s->limit->visit (this);
+
+  s->block->visit (this);
 }