Don't rely on the bindings list in gotcha_wrap() being static!
Closed this issue · 7 comments
Turns out Gotcha internally keeps pointers to the user-provided gotcha_binding_t structs in gotcha_wrap() and happily accesses them later on, e.g. when someone calls dl_open(). That was new to me! Not good when the original buffer is long since gone, as that leads to issues like LLNL/Caliper#288. Would be great if Gotcha would copy the user-provided list instead.
Does the problem still exist if you reset the priority to point to the original function? I think this is done by resetting the priority to -1 or something like that
While we could make this change, it seems a bit awkward. Part of the defined gotcha_wrap() behavior is modifying the gotcha_binding_t[] and handing it back to the user. It feels a little non-intuitive if the gotcha_binding_t we hand back isn't the definitive copy the user should use in the future (such as if I get around to implementing gotcha_unwrap).
Would it be much of a challenge to make Caliper keep the gotcha_binding_t[] around in the heap?
If so, I could be convinced to make this gotcha change. But my first gut feeling is this could change in Caliper.
(And either way, this needs to be better documented in Gotcha).
It's possible to work around this in Caliper, but it's pretty ugly. For example, in the MPI wrapper, we create the bindings list dynamically based on what the user wants to wrap, and I don't really want to keep that list around just so gotcha has something to modify behind my back. It shouldn't be my job to manage gotcha's memory :-) And without unwrap(), how am I supposed to know when it's safe to release the data?
I wasn't even aware that gotcha is supposed to return the modified list - gotcha_get_wrappee() works just fine on the input handle. And what would the user even want with it? I get the gotcha_unwrap() case, but can't you also do that just with the wrappee handle?
Even so, having gotcha_wrap() modify the gotcha_binding_t[] would be one thing, but I for one find it extremely unintuitive that gotcha still accesses behind my back after gotcha_wrap() is done. To me that list looks like input data for gotcha and nothing else. If an API wants to keep modifying user-managed memory in the background, then this should be made very clear.
@hariharan-devarajan Sounds good, thanks! Certainly helpful to have this in the documentation now.
@daboehme is this still a issue for Caliper? I know this was created 3 years back. I am trying to figure out if you guys need this resolved or the current design is fine.
Hi @hariharan-devarajan , we can work around it in Caliper. I'll close this issue.