Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix/fortran-bindings field_set_stride_xd pass by value #204

Conversation

diegorsjv
Copy link
Contributor

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.

… are actually passed by value and not by reference as they were. This resulted in segmentation faults when using strided compression with the Fortran bindings
@diegorsjv diegorsjv changed the title Fortran bindings set_stride_xd bugfix/fortran-bindings field_set_stride_xd pass by value Apr 11, 2023
@lindstro
Copy link
Member

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
Copy link

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
Copy link
Member

@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.

@GarrettDMorrison GarrettDMorrison merged commit b345842 into LLNL:develop Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants