LLNL / zfp

Compressed numerical arrays that support high-speed random access
http://zfp.llnl.gov
BSD 3-Clause "New" or "Revised" License
754 stars 152 forks source link

bugfix/fortran-bindings field_set_stride_xd pass by value #204

Closed diegorsjv closed 7 months ago

diegorsjv commented 1 year ago

Modified the zfp_field_set_stride_xd subroutines in zfp.f90 so that stride values are actually passed by value and not by reference as they were (using the value attribute for sx, sy, sz, sw). This resulted in segmentation faults when using strided compression with the Fortran C bindings.

lindstro commented 1 year ago

Thanks for catching this. Not being a Fortran programmer, I imagine that similar changes ought to be made throughout the zFORp API. For example, the zFORp_field_set_size functions use the same pass-by-reference convention, and I wonder why they would not invoke similar behavior (perhaps because c_ptrdiff_t is an extension?). Indeed, it is surprising that pass-by-reference could result in a segmentation fault since the Fortran wrapper just hands off the argument to the corresponding C function by value. Before we make a hundred or so changes to handle this consistently, it would be nice to know what's actually going wrong here.

td-mpcdf commented 1 year ago

To clarify where the problem comes from: The interface definition (done in Fortran) of a C function declares the arguments per default to be passed "as-reference", hence some kind of address. If in the declaration of the C function an integer is expected the reference passed in is interpreted as an integer (with a value of some memory address) which obviously leads to wrong results, mostly segmentation faults. In the interface declaration in Fortran, an argument which is expected to be a value and not a pointer in C, must be declared with the attribute "value". We just came accross the fixed lines in the MR, but there might be more.

lindstro commented 1 year ago

@td-mpcdf Thanks for the clarification. I guess I'm just surprised why zFORp_field_set_size works but zFORp_field_set_stride does not; they essentially have the same signature (and issue).

We'll take a closer look at fixing all of these issues before the next release.