Hey,
Just got a chance to take a look at the bugs, comments inline.
Nathan Scott <nathans@xxxxxxxxxx> writes:
[...]
> Coverity is reporting a couple of issues in pmdapapi that I
> could use some help with if you could? It's saying the sts
> setting here is dead code...
>
> /*
> * This is where we'd see if a requested counter was
> * "one too many". We must leave a note for the
> * function to return an error, but must continue (so
> * that reactivating other counters is still
> * attempted).
> */
> sts = PM_ERR_VALUE;
> continue;
>
> which at first glance does indeed seem to be the case (it'll
> be overwritten on subsequent loop iterations by PAPI_add_event).
So, it was indeed looping over and re-writing sts. The only case that
this variable would have returned PM_ERR_VALUE is if it occurred in the
last iteration of the loop.
This can be fixed by introducing a sts2 style variable, which the error
value is stored, and at the end of the function, if sts == 0, then we
return sts2, which would contain any error (if all is well it still
returns 0). However I'm running into an issue that was previously
hidden. When we loop over every available metric during the pmda
installation, I'm getting errors with the eventset unable to add any
more metrics to the eventset, even with mutliplexing enabled. AFAICT
this occurs due to the auto-enable functionality. Would you have any
suggestions on how to fix this? I'd prefer to not disable auto-enable
functionality by default.
> papi_store it then says has a loop control issue - apparently
> it can only execute the "for (i = 0; i < result->numpmid; i++)"
> loop once. I think this is because the switch always returns a
> result for the first PMID, when we should really be continuing
> on through all metrics being stored to.
I've updated/fixed this in my lberk/dev branch. It also takes an sts,
sts2 approach to the loop holding any errored states.
Cheers,
Lukas
|