Bugfix: PAPI_list_events

Open discussion of PAPI.

Bugfix: PAPI_list_events

Postby nsmeds » Thu Mar 21, 2013 5:35 am

I just now found out that PAPI_list_events() does not follow its own specs ;)

The current code (5.1.0.2) will never return the number of events in the eventset unless the supplied array (and thus the size argument) is larger than the event set. The man page clearly states that the returned number may be greater than the array size if the event set does not fit in the supplied array. In particular the call PAPI_list_events(EventSet, NULL, &n) should be allowed if n=0 at the time of the call.

Now, this may break some existing code out there :P but this is the only way in the spec to get the event set size so I argue it should be fixed. The case where things may get broken in existing code is when the caller supplies arguments with an array that does not fit the whole event set and trusts that the returned number of events is the numer of written entries written into the event array and not the number of available events in the real event set.

Since I have no code that relies on the above behaviour I think we should fix the library to follow its spec and that way find out who has programmed their apps according to library behaviour and not library spec :-)

Cheers,
/Nils

Code: Select all
--- orig/papi.c   2013-01-15 21:44:44.000000000 +0100
+++ patched/papi.c   2013-03-21 10:12:38.999322342 +0100
@@ -5882,26 +5882,36 @@ PAPI_remove_events( int EventSet, int *E
 int
 PAPI_list_events( int EventSet, int *Events, int *number )
 {
+   int maxEvents = *number;
    EventSetInfo_t *ESI;
    int i, j;
 
-   if ( ( Events == NULL ) || ( *number <= 0 ) )
+   if ( ( number == NULL ) || ( *number < 0 ) )
       papi_return( PAPI_EINVAL );
 
    ESI = _papi_hwi_lookup_EventSet( EventSet );
    if ( !ESI )
       papi_return( PAPI_ENOEVST );
 
+   /* The spec states that we always return the number of events in the event set */
+   *number = ESI->NumberOfEvents;
+
+   if ( maxEvents == 0 )
+      papi_return( PAPI_OK );
+
+   /* We better have a valid pointer now that we are going to write stuff */
+   if ( Events == NULL )
+      papi_return( PAPI_EINVAL );
+
    for ( i = 0, j = 0; j < ESI->NumberOfEvents; i++ ) {
       if ( ( int ) ESI->EventInfoArray[i].event_code != PAPI_NULL ) {
          Events[j] = ( int ) ESI->EventInfoArray[i].event_code;
          j++;
-         if ( j == *number )
+         if ( j == maxEvents )
             break;
       }
    }
 
-   *number = j;
 
    return ( PAPI_OK );
 }
nsmeds
 
Posts: 4
Joined: Tue Mar 05, 2013 11:15 am

Re: Bugfix: PAPI_list_events

Postby danterpstra » Tue Jun 25, 2013 5:33 pm

You're correct in your analysis and in your recommendation.
The code has been modified to match the documentation.
You can now pass Events = NULL and/or *number = 0 and get the expected behavior.
This has been committed to the git repository if you want to try it out; it will be officially available in the 5.2.0 release.
Now let's see if anybody complains about broken code :)
danterpstra
 
Posts: 63
Joined: Wed Jun 23, 2010 2:21 pm


Return to General discussion

Who is online

Users browsing this forum: No registered users and 0 guests

cron