When _stp_pmap_agg() returns NULL, something has gone wrong. Typically this is because the map size was too small.
patch committed translate.cxx 1.96
(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.
(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?
> 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.
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.
(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.
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.