Closed Roy-KC closed 5 months ago
What is this about? There is no explanation that I can see that specifies the purpose of the change.
Sorry about that. Here is a detailed changelog.
The purpose of the change is to correct inconsistencies in the library where Fortran Integer array is passed directly from a Fortran function to a C function that expects a C_INT array. If default Integer is not 4 bytes, the library will not compile. The correct pattern is to allocate a C_INT array and copy in or out the Fortran array. That pattern is followed sometimes, but not all the time.
I also added support for NF_INT_IS_C_LONG_LONG. This is necessary to support default Integer-8 on Windows Visual Studio.
I need the library to support Integer-8 or Integer-4.
Build Environment: Windows 11 Pro Visual Studio 2022 with Intel Fortran 2023.1
The C compiler identification is MSVC 19.36.32535.0 The Fortran compiler identification is Intel 2021.9.0.20230302
Compiling with default 8-byte integers uncovered several inconsistencies in the library. Most of the errors stem from assuming that Fortran Integer is equivalent to C_INT or by incorrectly passing a Fortran Integer array directly to a C Function that expects C_INT array.
The correct pattern for passing an array: ALLOCATE c_array COPY f_array to c_array Call c_function DEALLOCATE c_array
The correct pattern for receiving an array: ALLOCATE c_array Call c_function COPY c_array to f_array DEALLOCATE c_array
Here are the compiler-generated errors:
C:\Lib\tmp\netcdf-fortran\fortran\nf_fortv2.F90(841): error #6633: The type of the actual argument differs from the type of the dummy argument. [IMAP] C:\Lib\tmp\netcdf-fortran\fortran\nf_fortv2.F90(917): error #6633: The type of the actual argument differs from the type of the dummy argument. [IMAP] C:\Lib\tmp\netcdf-fortran\fortran\nf_fortv2.F90(1158): error #6633: The type of the actual argument differs from the type of the dummy argument. [IMAP] C:\Lib\tmp\netcdf-fortran\fortran\nf_fortv2.F90(1238): error #6633: The type of the actual argument differs from the type of the dummy argument. [IMAP] C:\Lib\tmp\netcdf-fortran\fortran\nf_fortv2.F90(1464): warning #6075: The data type of the actual argument does not match the definition. [LENSTR] compilation aborted for C:\Lib\tmp\netcdf-fortran\fortran\nf_fortv2.F90 (code 1)
C:\Lib\tmp\netcdf-fortran\fortran\nf_nc4.F90(319): error #6633: The type of the actual argument differs from the type of the dummy argument. [VARIDS] C:\Lib\tmp\netcdf-fortran\fortran\nf_nc4.F90(348): error #6633: The type of the actual argument differs from the type of the dummy argument. [DIMIDS] C:\Lib\tmp\netcdf-fortran\fortran\nf_nc4.F90(589): error #6633: The type of the actual argument differs from the type of the dummy argument. [DIM_SIZES] C:\Lib\tmp\netcdf-fortran\fortran\nf_nc4.F90(991): error #6633: The type of the actual argument differs from the type of the dummy argument. [DIM_SIZES] C:\Lib\tmp\netcdf-fortran\fortran\nf_nc4.F90(1863): error #6633: The type of the actual argument differs from the type of the dummy argument. [PARAMS] compilation aborted for C:\Lib\tmp\netcdf-fortran\fortran\nf_nc4.F90 (code 1)
The fortran/nf_fortv2.F90 [IMAP] errors are caused by passing Integer imap array to convert_v2_imap which expects C_INT array fmap. The change is trivial. ... fortran/nf_fortv2.F90(841): [IMAP] Subroutine ncvptg(ncid, varid, start, counts, strides, imap, values, & rcode) Integer, Intent(IN) :: imap(*) ... (841) Call convert_v2_imap(cncid, cvarid, imap, cimap, inullp)
fortran/nf_fortv2.F90(917): [IMAP] Subroutine ncvpgc(ncid, varid, start, counts, strides, imap, string, rcode) Integer, Intent(IN) :: imap(*) ... (917) Call convert_v2_imap(cncid, cvarid, imap, cimap, inullp)
fortran/nf_fortv2.F90(1158): [IMAP] Subroutine ncvgtg(ncid, varid, start, counts, strides, imap, values, & rcode) Integer, Intent(IN) :: imap(*) ... (1158) Call convert_v2_imap(cncid, cvarid, imap, cimap, inullp)
fortran/nf_fortv2.F90(1238): [IMAP] Subroutine ncvggc(ncid, varid, start, counts, strides, imap, string, rcode) Integer, Intent(IN) :: imap(*) ... (1238) Call convert_v2_imap(cncid, cvarid, imap, cimap, inullp)
fortran/module_netcdf_fortv2_c_interfaces.F90(597): (597) Subroutine convert_v2_imap(cncid, cvarid, fmap, cmap, inullp) Integer(C_INT), Intent(IN) :: fmap(*)
Change: Remove C_INT, since we are passing a Fortran Integer array
fortran/nf_fortv2.F90(1464): [LENSTR] Subroutine ncagtc(ncid, varid, attnam, string, lenstr, rcode) Integer, Intent(IN) :: lenstr ... (1464) Call c_ncagtc(cncid, cvarid, cattnam(1:ilen), cstring, lenstr, & crcode) ... fortran/module_netcdf_fortv2_c_interfaces.F90(508) Interface (508) Subroutine c_ncagtc(ncid, varid, attname, value, attlen, rcode) BIND(C) Integer(C_INT), VALUE :: ncid , varid, attlen
Note: The most significant changes occur in nf_ncf.F90 and should be reviewed carefully.
fortran/nf_nc4.F90(319): [VARIDS] Function nf_inq_varids( ncid, nvars, varids) RESULT (status) Integer, Intent(INOUT) :: varids(*) ... (319) cstatus = nc_inq_varids_f(cncid, cnvars, varids)
fortran/nf_lib.c(41): (41) int nc_inq_varids_f(int ncid, int nvars, int fvarids)
Change:
Add allocate cvarids Remove nc_inq_varids_f and call the netcdf-c:nc_inq_varids directly. Copy cvarids to varids, adding 1 to each deallocate cvarids
fortran/nf_nc4.F90(348): [DIMIDS] Function nf_inq_dimids( ncid, ndims, dimids, parent) RESULT (status) Integer, Intent(INOUT) :: dimids(*) ... (348) cstatus = nc_inq_dimids_f(cncid, cndims, dimids, cparent)
fortran/nf_lib.c(83): (83) int nc_inq_dimids_f(int ncid, int ndims, int fdimids, int parent)
Change:
Added nc_inq_numdimids to nf_lib.c to account for parent. No such function exists in netcdf-c. We have no way to duplicate passing NULL parameters to C functions without breaking f77. We need helper functions in the C layer.
allocate cdimids Remove nc_inq_dimids_f and call the netcdf-c:nc_inq_dimids directly. Copy cdimids to dimids, adding 1 to each deallocate cdimids
fortran/nf_nc4.F90(589): [DIM_SIZES] Function nf_insert_array_compound( ncid, xtype, name, offset, field_typeid, & ndims, dim_sizes) RESULT (status) Integer, Intent(INOUT) :: dim_sizes() ... (589) cstatus = nc_insert_array_compound_f(cncid, cxtype, cname(1:ie), & coffset, ctypeid, cndims, dim_sizes) fortran/nf_lib.c(125): (125) int nc_insert_array_compound_f(int ncid, int typeid, char name, size_t offset, nc_type field_typeid, int ndims, int *dim_sizesp) Change:
allocate cdim_sizes Copy dim_sizes to cdim_sizes, reverse order Remove nc_insert_array_compound_f and call the netcdf-c:nc_insert_array_compound directly. deallocate cdim_sizes
fortran/nf_nc4.F90(754): No compiler error (754) Function nf_inq_compound_field( ncid, xtype, fieldid, name, offset, & field_typeid, ndims, dim_sizes) RESULT (status) ... nc_inq_compound_field_f(cncid, cxtype, cfieldid, cname, coffset, & cfield_typeid, cndims, cdim_sizes) Change:
Remove nc_inq_compound_field_f and call the netcdf-c:nc_inq_compound_field directly. Copy cdim_sizes to dim_sizes, reverse order
fortran/nf_nc4.F90(991): [DIM_SIZES] Function nf_inq_compound_fielddim_sizes( ncid, xtype, fieldid, dim_sizes) & RESULT (status) Integer, Intent(INOUT) :: dim_sizes(*) ... (991) cstatus = nc_inq_compound_fielddim_sizes(cncid, cxtype, cfieldid, dim_sizes)
fortran/nf_lib.c(162): (162) int nc_inq_compound_field_ndims(int ncid, nc_type xtype, int fieldid, int *ndims)
Change:
Call nc_inq_compound_field_ndims to obtain ndims allocate cdim_sizes Call netcdf-c:nc_inq_compound_fielddim_sizes Copy cdim_sizes to dim_sizes, reverse order deallocate cdim_sizes
Note: This function looked incomplete. Not sure about copying reverse-order. Please check.
fortran/nf_nc4.F90(1863): [PARAMS] Function nf_def_var_filter( ncid, varid, filterid, nparams, params) RESULT(status) Integer, Intent(IN) :: params(*) ... (1863) cstatus = nc_def_var_filter(cncid, cvarid, cfilterid, cnparams, params)
fortran/module_netcdf4_nc_interfaces.F90(896): Function nc_def_var_filter(ncid, varid, id, nparams, params) BIND(C) Integer(C_INT) :: params(*)
Change:
allocate cparams Copy params to cparams Call netcdf-c:nc_def_var_filter deallocate cparams
fortran/module_netcdf4_nc_interfaces.F90 and fortran/nf_lib.c:
Remove nc_inq_varids_f => Use netcdf-c:nc_inq_varids Remove nc_inq_dimids_f => Use netcdf-c:nc_inq_dimids Add helper nc_inq_numdimids not in netcdf-c Remove nc_insert_array_compound_f => Use netcdf-c:nc_insert_array_compound Remove nc_inq_compound_field_f => Use netcdf-c:nc_inq_compound_field
On Windows Visual Studio, NF_INT_IS_C_LONG_LONG if Fortran Integer is IK8. These changes are completely safe, since they are active only when NF_INT_IS_C_LONG_LONG is defined.
Added NF_INT_IS_C_LONG_LONG ifdefs: fortran/module_netcdf_nc_data.F90 fortran/nf_attio.F90 fortran/nf_v2compat.c fortran/nf_var1io.F90 fortran/nf_varaio.F90 fortran/nf_vario.F90 fortran/nf_varmio.F90 fortran/nf_varsio.F90
Added support for long long: nf_test/util03.F nf_test/util.F nf_test/f03lib_f_interfaces.F90 nf_test/f03lib.c
Switched NF_INT variables to integer*4: nf_test4\f03tst_open_mem.F nf_test4\ftst_vars4.F nf_test4\ftst_vars5.F nf_test4\ftst_vars6.F
Note: All tests passed.
I reconsider my earlier comments after looking at this explanation.
I guess netcdf-fortran can work when compiled with -i8. I didn't think that was even possible.
Does not matter. This violates the netcdf spec.
How does it violate the netcdf spec? I did not make any interface changes. I added one c-helper function. Even if you do not approve of i8, the changes correctly handle the exchange of data between Fortran and C functions. It is incorrect to directly pass a Fortran array type to a C function. The ISO_C_BINDING exists to provide the exchange.
Please, at least review the changes. My motivation is i-8, but the actual changes correct more general inconsistencies.
The spec defines int as NC_INT as 32 bits. NC_INT64 is 64 bits. Do I misunderstand your proposed change?
@DennisHeimbigner I too had a negative reaction to this idea, but on thinking it over I see how it might work.
But it's not clear whether we should support such builds.
The PR cleans up some inconsistencies in the code and this is a good thing whether the -i8 feature is accepted or not. @Roy-KC may I suggest you start an issue in which you propose the capability of using -i8. Who would do this, and why?
Why not just compile netcdf the normal way and use NF_INT64 for 8-byte ints?
Also @WardF or @DennisHeimbigner can you approve the workflow running, so we can see what the CI system has to say?
Thank you for considering the changes. The need for i-8 support is not for the data passed to netcdf-c, although in for a penny in for a pound. I completely understand the use of INT verses INT64 data. In fact, I use the INT64 interface. The problem is the library uses default Integer interface for all arguments. I have code that needs very large indexing for sparse matrix arrays. I have to build the code i-8. That makes it very difficult to inteface with the i-4 version of the library. That means I have no type-checking. I would have to make blind calls to the library, hoping I specified i-4 as necessary.
I will be out of the office for the next five days. I will respond to any questions when I return. Again, thanks for considering the PR. If it's too big, I will try to break it up when I return. Other than the ifdef NF_INT_IS_C_LONG_LONG blocks, none of the changes include any special code to support i-8. Once the inconsistencies were addressed, the code is i-8 ready. Even if you do not want to officially support i-8 as a build option, please consider the changes. The code already supports ifdef NF_INT..., adding another define is a completely safe change and alters nothing unless the ifdef is active.
I am closing this PR because it contains too many changes at once.
Changes required for a clean Windows build
Build Environment: Windows 11 Pro Visual Studio 2022 with Intel Fortran 2023.1 CMake 3.26.4
The C compiler identification is MSVC 19.36.32535.0 The Fortran compiler identification is Intel 2021.9.0.20230302
Build a Windows shared lib
File ./CMakeLists.txt Symbols are not automatically exported. Added cmake command to export all symbols. This creates "exports.def" which could be edited to remove unnecessary symbols. I can provide this file on request. set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
Without this, netcdff.lib is not generated.
Config tests failures
File ./CMakeLists.txt In three places, config tests fail because the include folder is wrong. The tests fail because they cannot find "netcdf_meta.h" -SET(CMAKE_REQUIRED_INCLUDES ${NETCDF_INCLUDE_DIR}) SET(CMAKE_REQUIRED_INCLUDES ${NETCDF_C_INCLUDE_DIR})
RUN_TESTS failures
ftest: BUILD_DIR/nf_test/fills.nc is missing.
The "cp" command is unix only. Use cmake commands COPY/RENAME.
File ./nf_test/CMakeLists.txt -execute_process(COMMAND cp ${CMAKE_CURRENT_SOURCE_DIR}/ref_fills.nc file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/ref_fills.nc DESTINATION ${CMAKE_CURRENT_BINARY_DIR}) file(RENAME ${CMAKE_CURRENT_BINARY_DIR}/ref_fills.nc ${CMAKE_CURRENT_BINARY_DIR}/fills.nc)
f90tst_vars4: netcdf_expanded.F90 segfaults at line 1950: values(:, :) = reshape(defaultIntArray(:), shape(values)) nf90_get_var_2D_FourByteInt
defaultIntArray is a large array on the stack that causes a stack overflow. I used a compiler flag to switch stack arrays to the heap, but the best solution would be to ALLOCATE these arrays and move them off the stack.
Manually added /heap-arrays:1 to the netcdff project to get a successful test.
I did not commit any changes to address this issue because the temporary fix is compiler specific.
Here are the logs of the failures to support the changes:
File CMakeConfigureLog.yaml
Build RUN_TESTS failures
4/50 Testing: ftest 4/50 Test: ftest Command: "C:/Lib/tmp/build64/nf_test/Debug/ftest.exe" Directory: C:/Lib/tmp/build64/nf_test "ftest" start time: Jul 11 16:58 Central Daylight Time Output:
Testing netCDF-2 Fortran 77 API. testing nccre ... testing ncddef ... testing ncvdef ... testing ncapt, ncaptc ... testing ncclos ... testing ncvpt1 ... testing ncvgt1 ... testing ncvpt ... testing ncopn, ncinq, ncdinq, ncvinq, ncanam, ncainq ... testing ncvgt, ncvgtc ... testing ncagt, ncagtc ... testing ncredf, ncdren, ncvren, ncaren, ncendf ... testing ncacpy ... testing ncadel ... testing fill values ... ncopen: filename "fills.nc": No such file or directory NCOPN: : Unknown Error ncvarid: ncid -1: NetCDF: Not a valid ID ncvarid: ncid -1: NetCDF: Not a valid ID ncvarid: ncid -1: NetCDF: Not a valid ID ncvarid: ncid -1: NetCDF: Not a valid ID ncvarid: ncid -1: NetCDF: Not a valid ID NCVGT1: : NetCDF: Not a valid ID NCVGT1: : NetCDF: Not a valid ID NCVGT1: : NetCDF: Not a valid ID NCVGT1: : NetCDF: Not a valid ID NCVGT1: : NetCDF: Not a valid ID error in byte fill value error in double fill value error in float fill value error in long fill value error in short fill value Total number of failures: 11 2