[Bug translator/2149] New: check return value for _stp_map_set_xx()

[Bug translator/2149] New: check return value for _stp_map_set_xx()

[hunt at redhat dot com]

The translator does not check the return values for _stp_map_set calls. The most
common problem with this is that sets silently fail when the maps are filled.

-- 
           Summary: check return value for _stp_map_set_xx()
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: translator
        AssignedTo: systemtap at sources dot redhat dot com
        ReportedBy: hunt at redhat dot com


http://sourceware.org/bugzilla/show_bug.cgi?id=2149

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/2149] check return value for _stp_map_set_xx()

[joshua dot i dot stone at intel dot com]

------- Additional Comments From joshua dot i dot stone at intel dot com  2006-01-12 20:49 -------
I don't want to belittle the importance of checking return values, but filled
maps could be avoided by turning on map->wrap.  Perhaps we need pragmas or
something to let the user turn this on...

And BTW - the current replacement policy when wrapping is LRU.  But, when
aggregating pmaps (_stp_pmap_agg), it favors data from the later cpus in the
for_each_cpu.  I'm not sure of a "fair" way to correct this, except to let the
pmap->agg have more entries (MAXMAPENTRIES*NUM_CPUS).

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=2149

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/2149] check return value for _stp_map_set_xx()

[joshua dot i dot stone at intel dot com]

------- Additional Comments From joshua dot i dot stone at intel dot com  2006-01-12 20:56 -------
(In reply to comment #1)
> And BTW - the current replacement policy when wrapping is LRU.

Sorry, this is wrong - it's actually a FIFO.  To be LRU we need to move nodes to
the tail when they're accessed as well.  I think this would be preferable...

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=2149

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/2149] check return value for _stp_map_set_xx()

[hunt at redhat dot com]

------- Additional Comments From hunt at redhat dot com  2006-01-12 21:07 -------

> Sorry, this is wrong - it's actually a FIFO.  To be LRU we need to move nodes to
> the tail when they're accessed as well.  I think this would be preferable...

It's a little more complicated than that. When wrapping, it removes the first
node in the node list. That would typically be the first node created, kind of
FIFO. Except you can sort the list and change the ordering. If we enable wrap
mode, it should be per-map and should be used for things like FIFOs or lists.


-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=2149

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/2149] check return value for _stp_map_set_xx()

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2006-01-16 15:57 -------
(In reply to comment #1)
> I don't want to belittle the importance of checking return values, but filled
> maps could be avoided by turning on map->wrap.

The current wrapping scheme seems too complicated to use.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=2149

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/2149] check return value for _stp_map_set_xx()

[hunt at redhat dot com]

------- Additional Comments From hunt at redhat dot com  2006-01-16 18:23 -------
(In reply to comment #4)
> (In reply to comment #1)
> > I don't want to belittle the importance of checking return values, but filled
> > maps could be avoided by turning on map->wrap.
> 
> The current wrapping scheme seems too complicated to use.

It depends what you want to use it for. Setting one flag enables it. Current
behaviour is FIFO. Adding a flag to change the behaviour to LRU would be simple.
Only gotcha is that there is only one node list and sorting will reorder the list.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=2149

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/2149] check return value for _stp_map_set_xx()

[eteo at redhat dot com]

------- Additional Comments From eteo at redhat dot com  2006-04-23 04:32 -------
Before fix, _stp_map_set_xx do not have their return values checked:

[eteo@eteo src]$ stap -DMAXMAPENTRIES=1 -p3 -u -e 'probe begin { a[2]="hello" ;
a[3]="a"} global a' | grep _stp_map_set
     _stp_map_set_is (global_a, l->__tmp0, (l->__tmp2[0] ? l->__tmp2 : NULL));
     _stp_map_set_is (global_a, l->__tmp4, (l->__tmp6[0] ? l->__tmp6 : NULL));

After applying fix:

[eteo@eteo src]$ stap -DMAXMAPENTRIES=1 -p3 -u -e 'probe begin { a[2]="hello" ;
a[3]="a"} global a' | grep _stp_map_set
      { int rc = _stp_map_set_is (global_a, l->__tmp0, (l->__tmp2[0] ? l->__tmp2
: NULL)); if (unlikely(rc)) c->last_error = "Array overflow, check
MAXMAPENTRIES"; };
      { int rc = _stp_map_set_is (global_a, l->__tmp4, (l->__tmp6[0] ? l->__tmp6
: NULL)); if (unlikely(rc)) c->last_error = "Array overflow, check
MAXMAPENTRIES"; };

[eteo@eteo src]$ stap -DMAXMAPENTRIES=1 -u -e 'probe begin { a[2]="hello" ;
a[3]="a"} global a'
ERROR: Array overflow, check MAXMAPENTRIES near identifier 'a' at <input>:1:30
WARNING: Number of errors: 1, skipped probes: 0

Here's the patch. Please feedback.

Index: ChangeLog
===================================================================
RCS file: /cvs/systemtap/src/ChangeLog,v
retrieving revision 1.360
diff -u -3 -r1.360 ChangeLog
--- ChangeLog   23 Apr 2006 03:28:45 -0000      1.360
+++ ChangeLog   23 Apr 2006 04:24:42 -0000
@@ -1,5 +1,11 @@
 2006-04-23  Eugene Teo  <eteo@redhat.com>

+       PR 2149
+       * translate.cxx (mapvar::set): Test _stp_map_set_xx() for
+       array overflows.
+
+2006-04-23  Eugene Teo  <eteo@redhat.com>
+
        * small_demos/ansi_colors.stp: Add an example of using octal
        escape sequences to display all possible ansi colors.

Index: translate.cxx
===================================================================
RCS file: /cvs/systemtap/src/translate.cxx,v
retrieving revision 1.114
diff -u -3 -r1.114 translate.cxx
--- translate.cxx       22 Apr 2006 06:46:00 -0000      1.114
+++ translate.cxx       23 Apr 2006 04:24:46 -0000
@@ -581,14 +581,20 @@

   string set (vector<tmpvar> const & indices, tmpvar const & val) const
   {
+    string res = "{ int rc = ";
+
     // impedance matching: empty strings -> NULL
     if (type() == pe_string)
-      return (call_prefix("set", indices)
+      res += (call_prefix("set", indices)
              + ", (" + val.qname() + "[0] ? " + val.qname() + " : NULL))");
     else if (type() == pe_long)
-      return (call_prefix("set", indices) + ", " + val.qname() + ")");
+      res += (call_prefix("set", indices) + ", " + val.qname() + ")");
     else
       throw semantic_error("setting a value of an unsupported map type");
+
+    res += "; if (unlikely(rc)) c->last_error = \"Array overflow, check
MAXMAPENTRIES\"; }";
+
+    return res;
   }

   string hist() const

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING


http://sourceware.org/bugzilla/show_bug.cgi?id=2149

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/2149] check return value for _stp_map_set_xx()

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2006-04-27 16:46 -------
patch committed

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|                            |FIXED


http://sourceware.org/bugzilla/show_bug.cgi?id=2149

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.