[Bug translator/2156] New: check return value for _stp_pmap_agg()

[Bug translator/2156] New: check return value for _stp_pmap_agg()

[hunt at redhat dot com]

When _stp_pmap_agg() returns NULL, something has gone wrong. Typically this is
because the map size was too small.

-- 
           Summary: check return value for _stp_pmap_agg()
           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=2156

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

[Bug translator/2156] check return value for _stp_pmap_agg()

[joshua dot i dot stone at intel dot com]

------- Additional Comments From joshua dot i dot stone at intel dot com  2006-01-18 02:10 -------
patch committed

translate.cxx   1.96

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


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

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

[Bug translator/2156] check return value for _stp_pmap_agg()

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2006-01-18 03:05 -------
(In reply to comment #1)
> patch committed
> translate.cxx   1.96

Is there a test case for this scenario?
The new code looks suspicious for the unsorted case.


-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
  GCC build triplet|                            |joshua.i.stone@intel.com


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

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

[Bug translator/2156] check return value for _stp_pmap_agg()

[joshua dot i dot stone at intel dot com]

------- Additional Comments From joshua dot i dot stone at intel dot com  2006-01-18 03:35 -------
(In reply to comment #2)
> Is there a test case for this scenario?
> The new code looks suspicious for the unsorted case.

buildok/pmap_foreach.stp tests the unsorted case - I just modified it to test
the sorted case as well.

What looks suspicious to you?

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |joshua dot i dot stone at
                   |                            |intel dot com
  GCC build triplet|joshua.i.stone@intel.com    |


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

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

[Bug translator/2156] check return value for _stp_pmap_agg()

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2006-01-18 03:45 -------
> buildok/pmap_foreach.stp tests the unsorted case - I just modified it to test
> the sorted case as well.

OK, but that tests only buildability.  Is there a run-time test?

> What looks suspicious to you?

The conditional emission of an "else" (only in the sorted case) is a tip.
Looking at the generated code in both cases, stp_pmap_get_agg() can still
be called and the result used, in the stp_map_start call.


-- 


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

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

[Bug translator/2156] check return value for _stp_pmap_agg()

[hunt at redhat dot com]

------- Additional Comments From hunt at redhat dot com  2006-01-18 18:06 -------
Looking at the latest generated code, it does this
if (unlikely(NULL == _stp_pmap_agg (global_foo)))
      c->last_error = "unknown error while aggregating global_foo";
    write_unlock (& global_foo_lock);
  post_unlock_3: ;
    read_lock (& global_foo_lock);
    l->__tmp19 = _stp_map_start (_stp_pmap_get_agg(global_foo));
[...]

_stp_pmap_agg() will return NULL if the map pointer is invalid, the map type in
unknown, or it overflows.  The first two should be impossible with code from the
translator, so I suggest changing the error message to indicate an overflow
happened and suggest the map size needs increased.

_stp_pmap_get_agg() always returns a valid pointer but in the case above the
aggragated map will not be complete. So printing it anyway is safe, but
potentially misleading.


-- 


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

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

[Bug translator/2156] check return value for _stp_pmap_agg()

[joshua dot i dot stone at intel dot com]

------- Additional Comments From joshua dot i dot stone at intel dot com  2006-01-18 18:54 -------
(In reply to comment #4)
> > buildok/pmap_foreach.stp tests the unsorted case - I just modified it to test
> > the sorted case as well.
> 
> OK, but that tests only buildability.  Is there a run-time test?

I will make sure that there's a runtime test.  If possible, I will also try to
create tests of the failure case as well.


(In reply to comment #5)
> _stp_pmap_agg() will return NULL if the map pointer is invalid, the map type in
> unknown, or it overflows.  The first two should be impossible with code from the
> translator, so I suggest changing the error message to indicate an overflow
> happened and suggest the map size needs increased.

Ok, I will change the message as suggested.

> _stp_pmap_get_agg() always returns a valid pointer but in the case above the
> aggragated map will not be complete. So printing it anyway is safe, but
> potentially misleading.

There won't be anything misleading, because the first statement within the
foreach (maybe a print, or anything) will check c->last_error before doing
anything.  Thus the error is delayed a little bit, but as long as the operations
between the error and its detection are safe, there's no problem.  I see no
reason to optimize for the error case and try to catch it sooner.


-- 


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

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

[Bug translator/2156] check return value for _stp_pmap_agg()

[joshua dot i dot stone at intel dot com]

------- Additional Comments From joshua dot i dot stone at intel dot com  2006-01-18 21:00 -------
Runtime tests are now in CVS

tests/testsuite/systemtap.maps/ix.*
Modified to test successful pmap aggregation for sorted and unsorted cases.

tests/testsuite/systemtap.maps/pmap_agg_overflow.*
New test to verify the failing case is handled properly.

-- 


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

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