On 18.07.21 06:36, Sandra Loosemore wrote:
This patch fixes bugs I observed in tests for the CFI_section function
-- it turns out both the function and test cases had bugs. :-(
The bugs in CFI_section itself had to do with incorrect computation of
the base address for the result descriptor, plus an ordering problem
that caused it not to work if the source and result descriptors are
the same. I fixed this by rewriting the loop to fill in the dimension
info for the result array and reordering it with respect to the base
address computation.
Note that the old version of CFI_section attempted to preserve the
lower bounds of the section passed in as an argument.
Namely: for CFI_section(result, source, /*lower_bound */ {5, 7}, ...),
the result was: 5 and 6 for i=1,2 in result->dim[i].lower_bound.
With the attached patch, it is == 0. The latter is closer to the
generic Fortran behavior, where in Fortran: lbound(A(5:,7:)) == [1,1].
And CFI arrays passed to nonallocatable, nonpointer dummies have [0,0] as
lower bound.
This is not actually required by the Fortran standard, which specifies
only the shape of the result array, not its bounds. My rewritten
version produces an array with zero lower bounds, similar to what
CFI_establish produces given the shape as input.
I think something has to set a lower bound. For
CFI_establish (dv, /* base_addr = */ NULL, ...)
it is not mandated to set dv->dim[i].lower_bound (as base_addr=NULL).
On the other hand, CFI_section(...) also does not explicitly require it
to be set.
Hence, lower_bound can be unset – but that does not work well, especially
not when the uninitialized lower_bound either exceeds 'int' or even
'ptrdiff_t'.
This issue shows up when not setting lower_bound in CFI_section, e.g.
at
ts29113/interoperability/cf-descriptor-6-c.c's ctest()
(submitted v2 testsuite, not yet committed),
which uses 'int' and overflows.
And for
gcc/testsuite/gfortran.dg/ISO_Fortran_binding_10.c
which assumed in the CFI_address call that lower_bound == 0.
* * *
Thus: I think we really should set it in CFI_section – and the choice
to set it to 0 is in my opinion better than to set it to lower_bound.
I wonder whether we need to add a comment before
result->dim[o].lower_bound = 0;
Maybe:
/* If result was established with base_addr = NULL, its lower bound it
unset; while Fortran 2018 does not specify that CFI_section updates
the lower_bound, setting it to zero is sensible and in line with
Fortran subsections having a lower bound of 1. */
The bug in the older ISO_Fortran_binding_1.c testcase was an incorrect
assertion about the lower bound behavior,
Concur.
while the bugs in the not-yet-committed TS29113 testsuite were due to
me having previously lost track of having fixed this already and just
failing to save the fix before I posted the testsuite patch. As with
the other patches I've been posting for TS29113 testsuite issues, I
can refactor the testsuite changes to lump them all in with the base
testsuite patch depending on the order that things get reviewed/committed.
I tried it with the 'dim[i].lower_bound = 0;' line commented out;
in any case, I see the following XPASS with the such-modified patch
applied:
gfortran.dg/ts29113/library/section-1p.f90
gfortran.dg/ts29113/library/section-2p.f90
gfortran.dg/ts29113/library/section-3p.f90
gfortran.dg/ts29113/interoperability/cf-descriptor-2.f90
gfortran.dg/ts29113/interoperability/fc-out-descriptor-7.f90
[PR libfortran/101310] Bind(c): Fix bugs in CFI_section
CFI_section was incorrectly adjusting the base pointer for the result
array twice in different ways. It was also overwriting the array
dimension info in the result descriptor before computing the base
address offset from the source descriptor, which caused problems if
the two descriptors are the same. This patch fixes both problems and
makes the code simpler, too.
A consequence of this patch is that the result array is now 0-based in
all dimensions instead of starting at the numbering to match the first
element of the source array. The Fortran standard only specifies the
shape of the result array, not its lower bounds, so this is permitted
and probably less confusing for users as well as implementors.
2021-07-17 Sandra Loosemore<san...@codesourcery.com>
PR libfortran/101310
libgfortran/
* runtime/ISO_Fortran_binding.c (CFI_section): Fix the base
address computation and simplify the code.
gcc/testsuite/
* gfortran.dg/ISO_Fortran_binding_1.c (section_c): Remove
incorrect assertions.
* gfortran.dg/ts29113/library/section-3.f90: Fix indexing bugs.
* gfortran.dg/ts29113/library/section-3p.f90: Likewise.
* gfortran.dg/ts29113/library/section-4-c.c: New file.
* gfortran.dg/ts29113/library/section-4.f90: New file.
LGTM – thanks for the patch!
Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
