Conversation
nf_def_var_fill and nf_inq_var_fill declare their fill_value dummy argument as Character(KIND=C_CHAR) fill_value(*) and use it as a void* pass-through. Passing an INTEGER actual argument through the implicit interface is a type mismatch and therefore not conforming; it only happens to work with compilers whose character dummy ABI puts the raw data address in the leading argument. Use a character*4 buffer and the transfer() intrinsic to move the bit pattern of the integer fill value in and out of it, so the actual argument matches the dummy argument's type.
nf_insert_enum, nf_inq_enum_member, nf_put_att, nf_get_att, nf_put_vlen_element and nf_get_vlen_element all declare their data dummy argument as Character(KIND=C_CHAR) and use it as a void* pass-through. Passing INTEGER actual arguments through the implicit interface is a type mismatch and therefore not conforming. Use character buffers sized to the data being exchanged and the transfer() intrinsic to move the bit pattern in and out of them. The vlen handles are opaque to Fortran, so declare them as character objects directly instead of integer*8 arrays.
ncvpt declares its values dummy argument as Character(KIND=C_CHAR) values(*) and uses it as a void* pass-through, so handing it a REAL array through the implicit interface is a type mismatch and not conforming. Copy the array's bit pattern into a character buffer with transfer() and pass that instead.
ncapt, ncagt, ncvpt, ncvgt, ncvpt1 and ncvgt1 declare their data dummy argument as Character(KIND=C_CHAR) and use it as a void* pass-through. Handing them BYTE, INTEGER*2, INTEGER, REAL or DOUBLE PRECISION actual arguments through the implicit interface is a type mismatch and therefore not conforming. Add small pack/unpack helpers that move the bit pattern of the numeric data in and out of a character buffer with transfer(), and pass that buffer to the netCDF-2 routines instead.
nf_def_var_fill, nf_inq_var_fill, nf_insert_enum and nf_inq_enum_member all declare their data dummy argument as Character(KIND=C_CHAR) and use it as a void* pass-through. The nf90_ wrappers in netcdf4_func.F90 hand them INTEGER and REAL actual arguments through the implicit interface, which is a type mismatch and therefore not conforming; it only happens to work with compilers whose character dummy ABI puts the raw data address in the leading argument. Use character buffers sized to the data being exchanged and the transfer() intrinsic to move the bit pattern in and out of them, so the actual arguments match the dummy arguments' type. This is the same treatment the test programs already received; it leaves the shipped library as the last place the mismatch remained.
nf90_def_var_fill, nf90_inq_var_fill, nf90_insert_enum and nf90_inq_enum_member had no test coverage at all, so the character buffer conversion in netcdf4_func.F90 was unverified. f90tst_var_fill exercises the def/inq fill value round trip for every kind the generic interfaces are overloaded for, and f90tst_enum exercises the enum member round trip. Both check the values that come back, so a mis-sized or misplaced transfer() in the wrappers is caught.
|
|
|
Please confirm a human in the loop, otherwise this will be closed as AI copy-and-paste spam. |
|
Yes, I personally discovered the non-conforming code, figured out how to make it conforming using a transfer and then directed AI to do the changes everywhere. Then I noticed some of the changes were not tested, so I directed AI to add a test (nf03_test4/f90tst_enum.F90). I am happy to rework it based on feedback. The main thing I want to hear whether you are willing to make the code conforming using the transfer mechanism, or whether we need to figure out another solution. |
|
Can you approve the CLA? |
|
@WardF sure! The app doesn't work however: 
|
|
@certik I suspect that this is an issue on the back-end; I am also seeing strange behavior and 'you have not signed the CLA' messages on other PRs I am working with, on other projects. I would give it a little time. In the meantime, I will proceed as though you have signed. Thanks! @dopplershift I'm not certain where to check the status of the CLA service we're using; a quick glance at GitHub.com's status dashboard doesn't show any outages, so I'm unsure if it's a back-end issue or if something has changed and we need to reconfigure things on our end. Any insight/thoughts? (For reference, I'm getting a 'you have not signed the CLA' message over at Unidata/netcdf-c#3453, even after I re-sign the CLA) |
|
@WardF I'll keep trying. In the meantime, review the PR and let me know:
|
|
CLA site seems to be working now? @WardF for Unidata/netcdf-c#3454 it looks like the commit itself is triggered to account "agent", so I'm not surprised it's not cleared by the CLA checker. |
4 participants
Problem
netCDF-Fortran implements the C
void *data arguments of about 28 entrypoints as
Character(KIND=C_CHAR) :: x(*)— among themnf_put_att,nf_get_att,nf_put_var1,nf_get_var1,nf_put_vara,nf_get_vara,nf_put_vars,nf_get_vars,nf_def_var_fill,nf_inq_var_fill,nf_insert_enum,nf_inq_enum_member,nf_put_vlen_element,nf_get_vlen_element, and the netCDF-2 routinesncapt,ncagt,ncvpt,ncvgt,ncvpt1,ncvgt1.These are called through an implicit interface — they are declared only as
Integer, External(seefortran/netcdf4_externals.F90) — so passing anINTEGERorREALactual argument to aCHARACTERdummy is a type mismatchthat no compiler is obliged to diagnose. It works today only because the
character-dummy ABI happens to put the raw data address in the leading
argument slot and the C side ignores the hidden length. Nothing in the
standard guarantees that, and a compiler that checks (or that passes
character actual arguments differently) is free to reject or miscompile it.
This is not confined to the test suite: the shipped F90 API has the same
mismatch.
fortran/netcdf4_func.F90handsinteger(OneByteInt)throughreal(EightByteReal)straight tonf_def_var_fill/nf_inq_var_fill, anddefault
INTEGERtonf_insert_enum/nf_inq_enum_member— 14 call sites inthe library itself.
Fix
Copy the bit pattern of the data into (and out of) a character buffer with
transfer()at every affected call site, so the actual argument matches thedummy argument's type:
This is a call-site-only change. No public interface changes, no ABI change,
and no change to the bytes that reach the C library — the generated calls are
the same, they are now merely standard-conforming.
Call sites fixed in the library (
fortran/netcdf4_func.F90): the sixnf90_def_var_fill_*and sixnf90_inq_var_fill_*specifics, plusnf90_insert_enumandnf90_inq_enum_member.nf90_free_vlen,nf90_put_att_any,nf90_get_att_any,nf90_put_var_anyandnf90_get_var_anyalready passedcharacter(len=*)and are untouched.Call sites fixed in the tests:
nf_test/ftest.F,nf_test/tst_f77_v2.F,nf_test4/ftst_vars3.F,ftst_vars4.F,ftst_vars5.F,ftst_vars6.F,ftst_var_compact.F,ftst_var_szip.F,f03tst_open_mem.F.New tests
nf90_def_var_fill,nf90_inq_var_fill,nf90_insert_enumandnf90_inq_enum_memberhad no test coverage at all, which would have left thelibrary change unverified. Two tests are added under
nf03_test4:f90tst_var_fill— def/inq fill value round trip for every kind the genericinterfaces are overloaded for (1/2/4/8-byte integers, 4/8-byte reals).
f90tst_enum— enum member round trip throughnf90_insert_enum/nf90_inq_enum_member.Both check the values that come back, so a mis-sized or misplaced
transfer()is caught; verified by deliberately shrinking a buffer and watching the test
fail. Wired into both the CMake and autotools builds.
Testing
Full suite, 55/55 passing with each of:
Why not assumed-type?
The arguably cleaner fix is to declare these dummies
TYPE(*), DIMENSION(..),which is what
void *actually means and would make every existing numericcall site legal without touching any of them. That is a much larger change: it
requires Fortran 2018, and these procedures apply
C_LOCto the dummy (whichassumed-type disallows), so each would need restructuring. The
transfer()approach here is contained, works with the F2003 baseline the library already
targets, and needs no compiler-specific attributes.