On 7/21/21 11:26 AM, Tobias Burnus wrote:
On 17.07.21 02:49, Sandra Loosemore wrote:
This patch is for PR101317, one of the bugs uncovered by the TS29113
testsuite. Here I'd observed that CFI_establish, etc was not
diagnosing some invalid-argument situations documented in the
standard, although it was properly catching others. After fixing
those I discovered a couple small mistakes in the test cases and fixed
those too.
Some first comments – I think I have to read though the file
ISO_Fortran_binding.c itself and not only your patch.
--- a/libgfortran/runtime/ISO_Fortran_binding.c
+++ b/libgfortran/runtime/ISO_Fortran_binding.c
@@ -232,7 +232,16 @@ CFI_allocate (CFI_cdesc_t *dv, const CFI_index_t
lower_bounds[],
/* If the type is a Fortran character type, the descriptor's element
length is replaced by the elem_len argument. */
if (dv->type == CFI_type_char || dv->type == CFI_type_ucs4_char)
- dv->elem_len = elem_len;
+ {
+ if (unlikely (compile_options.bounds_check) && elem_len == 0)
+ {
+ fprintf ("CFI_allocate: The supplied elem_len must be "
+ "greater than zero (elem_len = %d).\n",
+ (int) elem_len);
I think there is no need to use '(elem_len = %d)' given that it is
always zero as stated in the error message itself.
Yeah, I could fix this. I'd initially forgotten that elem_len was an
unsigned type and was trying to test it by passing a negative value. :-P
(Appears twice)
However, the check itself is also wrong – cf. below.
Hmmm. CFI_establish explicitly says that the elem_len has to be greater
than zero. It seems somewhat confusing that it's inconsistent with the
other functions that take an elem_len argument.
Talking about CFI_allocatable, there is also another bug in that function,
untouched by your patch:
/* If the type is a character, the descriptor's element length is
replaced
by the elem_len argument. */
if (dv->type == CFI_type_char || dv->type == CFI_type_ucs4_char ||
dv->type == CFI_type_signed_char)
dv->elem_len = elem_len;
The bug is that CFI_type_signed_char is not a character type.
Ha! I noticed the same thing and already posted a separate patch for
that. :-P
https://gcc.gnu.org/pipermail/fortran/2021-July/056243.html
+ else if (unlikely (compile_options.bounds_check)
+ && type < 0)
Pointless line break.
+ fprintf (stderr, "CFI_establish: Extents must be nonnegative "
+ "(extents[%d] = %d).\n", i, (int)extents[i]);
+ return CFI_INVALID_EXTENT;
+ }
How about PRIiPTR + ptrdiff_t instead of %d + (int) cast? At least as
positive value, extent may exceed INT_MAX.
Hmmm, there are similar problems in existing code in other functions in
this file (e.g., CFI_section).
+ if (source->attribute == CFI_attribute_other
+ && source->rank > 0
+ && source->dim[source->rank - 1].extent == -1)
+ {
+ fprintf (stderr, "CFI_setpointer: The source is a "
+ "nonallocatable nonpointer object that is an "
+ "assumed-size array.\n");
I think you could just check for assumed rank – without
CFI_attribute_other in the 'if' and 'nonallocatable nonpointer' in the
error message. Only nonallocatable nonpointer variables can be of
assumed size (in Fortran); I think that makes the message simpler
(focusing on the issue) and if the C user passes an allocatable/pointer,
which is assumed rank, it is also a bug.
The wording of the message reflects the language of the standard:
"source shall be a null pointer or the address of a C descriptor for an
allocated allocatable object, a data pointer object, or a nonallocatable
nonpointer data object that is not an assumed-size array.
-Sandra
