Re: [PATCH, Fortran] [PR libfortran/101317] Bind(c): Improve error checking in CFI_* functions
Hi Sandra,
On 21.07.21 20:01, Sandra Loosemore wrote:
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.
Congratulation – we have found a bug in the spec, which is also
present in the current draft (21-007). I have now written to J3:
https://mailman.j3-fortran.org/pipermail/j3/2021-July/013189.html
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
:-)
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).
I think that you could fix as well. At least for size(array), it is not
uncommon that this exceeds MAX_INT.
On the other hand, I think it is unlikely to occur for a single
dimension (→ extent). In particular, the most likely way to get a
negative value is doing 'int' calculations with an overflow – and then
assigning the result "array->dim[i].extent". But in that case, that
(possibly negative) value fits into an int by construction.
+ 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.
I know – but the wording is such that it permits all 'nonallocatable
nonpointer data object' – with one exception.
This does not mean that 'assumed-size array' is only invalid for
'nonallocatable nonpointer' – it is also invalid for
allocatables/pointers. The latter cannot occur for Fortran code as only
deferred-shape arrays are permitted in that case, but from the C side,
you can easily set it to the wrong value.
Thus, by simplifying the wording, the error message is clearer (directly
pointing to the issue) and it additionally catches another wrong use of
the array descriptor, which can be (only) triggered from C.
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
Re: [PATCH, Fortran] Bind(c): CFI_signed_char is not a Fortran character type
On 16.07.21 05:46, Sandra Loosemore wrote: When I was reading code in conjunction with fixing PR101317, I noticed an unrelated bug in the implementation of CFI_allocate and CFI_select_part: they were mis-handling the CFI_signed_char type as if it were a Fortran character type for the purposes of deciding whether to use the elem_len argument to those functions. It's really an integer type that has the size of signed char. I checked similar code in other functions in ISO_Fortran_binding.c and these were the only two that were incorrect. I think there was at least one other place, but that one has been fixed in the meanwhile, missing the other two occurrences you found. Bind(c): signed char is not a Fortran character type CFI_allocate and CFI_select_part were incorrectly treating CFI_type_signed_char as a Fortran character type for the purpose of deciding whether or not to use the elem_len argument. It is a Fortran integer type per table 18.2 in the 2018 Fortran standard. Other functions in ISO_Fortran_binding.c appeared to handle this case correctly already. 2021-07-15 Sandra Loosemore gcc/testsuite/ * gfortran.dg/ts29113/library/allocate-c.c (ctest): Also test handling of elem_len for CFI_type_char vs CFI_type_signed_char. * gfortran.dg/ts29113/library/select-c.c (ctest): Likewise. libgfortran/ * runtime/ISO_Fortran_binding.c (CFI_allocate) LGTM. Thanks! 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
Re: [PATCH] PR fortran/101536 - ICE in gfc_conv_expr_descriptor, at fortran/trans-array.c:7324
Hi Harald,
On 21.07.21 22:22, Harald Anlauf via Fortran wrote:
Another one of Gerhard's infamous testcases. We did not properly detect
and reject array elements of type CLASS as argument to an intrinsic when
it should be an array.
Regtested on x86_64-pc-linux-gnu. OK for mainline / 11-branch when it
reopens?
...
+class(t), allocatable :: x(:)
+f = size (x(1)) ! { dg-error "must be an array" }
...
if (e->ts.type == BT_CLASS && gfc_expr_attr (e).class_ok
&& CLASS_DATA (e)->attr.dimension
&& CLASS_DATA (e)->as->rank)
{
+ if (e->ref && e->ref->type == REF_ARRAY
+ && e->ref->u.ar.type == AR_ELEMENT)
+ goto error;
I think that one is wrong. While CLASS_DATA (e) accesses
e->ts.u.derived->components,
which always works, your code assumes that there is only 'c' and not 'x%c' where
'c' is of type BT_CLASS and 'x' is of type BT_DERIVED.
I wonder whether it works if you simply remove 'return true;'
as gfc_add_class_array_ref sets 'e->rank = CLASS(e)->rank (and
adds an AR_FULL ref, if needed). In the nonerror case, the
'return true' is obtained via:
if (e->rank != 0 && e->ts.type != BT_PROCEDURE)
return true;
And, otherwise, it falls through to the error.
OK if that works – but please also add a test like
type t
class(*), allocatable :: c(:)
end type t
type(t) :: x
x%c = [1,2,3,4]
print *, size(x%c)
print *, size(x%c(1)) ! { dg-error ... }
end
Thanks,
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
Re: [PATCH] PR fortrsn/101564 - ICE in resolve_allocate_deallocate, at fortran/resolve.c:8169
On 21.07.21 22:36, Harald Anlauf via Gcc-patches wrote:
Anyway, here's a straightforward fix for a NULL pointer dereference for
an invalid argument to STAT. For an alternative patch by Steve see PR.
Regtested on x86_64-pc-linux-gnu. OK for mainline / 11-branch when it
reopens?
..
Fortran: ICE in resolve_allocate_deallocate for invalid STAT argument
gcc/fortran/ChangeLog:
PR fortran/101564
* resolve.c (resolve_allocate_deallocate): Avoid NULL pointer
dereference and shortcut for bad STAT argument to (DE)ALLOCATE.
gcc/testsuite/ChangeLog:
PR fortran/101564
* gfortran.dg/pr101564.f90: New test.
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 45c3ad387ac..51d312116eb 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -8165,6 +8165,9 @@ resolve_allocate_deallocate (gfc_code *code, const char
*fcn)
gfc_error ("Stat-variable at %L must be a scalar INTEGER "
"variable", &stat->where);
+ if (stat->expr_type == EXPR_CONSTANT || stat->symtree == NULL)
+ goto done_stat;
+
I wonder whether this will catch all cases, e.g. stat->symtree != NULL
but using something else than '->n.sym'. I currently cannot spot
whether a user operator or a type-bound procedure is possible
in this case, but if so, n.sym->something is not well defined.
Additionally, I wonder whether that will work with:
integer, pointer :: ptr
integer function f()
pointer :: f
f = ptr
end
allocate(A, stat=f())
The f() is a variable and definable – but I am currently not sure it sets
stat->symtree
and not only stat->value.function.esym, but I have not tested it.
(Answer: it does set it - at least there is an assert in
gfc_check_vardef_context
that symtree != NULL for EXPR_FUNCTION.)
Can't we just as a 'if (!' + ') goto done_stat;' around:
gfc_check_vardef_context (stat, false, false, false,
_("STAT variable"));
Additionally, I have to admit that I do not understand the
following existing condition, which you did not touch:
if ((stat->ts.type != BT_INTEGER
&& !(stat->ref && (stat->ref->type == REF_ARRAY
|| stat->ref->type == REF_COMPONENT)))
|| stat->rank > 0)
gfc_error ("Stat-variable at %L must be a scalar INTEGER "
"variable", &stat->where);
I mean the ts.type != BT_INTEGER and stat->rank != 0 is clear,
but what's the reason for the refs?
My impression is that it is supposed to handle REF_INQUIRY
such as x%kind – but that does not seem to handle x%y%kind.
It looks as if gfc_check_vardef_context needs an additional
check for REF_INQUIRY – and then the check above can be
simplified to the obvious version.
Can you check? That's
* use if (!gfc_check_vardef_context ()) goto done_stat;
* Add REF_INQUIRY check to gfc_check_vardef_context
* Simplify the check to !BT_INTEGER || rank != 0
And possibly add a testcase for stat=f() [valid]
and stat=x%y%kind [invalid] as well?
Thanks,
Tobias
for (p = code->ext.alloc.list; p; p = p->next)
if (p->expr->symtree->n.sym->name == stat->symtree->n.sym->name)
{
@@ -8192,6 +8195,8 @@ resolve_allocate_deallocate (gfc_code *code, const char
*fcn)
}
}
+done_stat:
+
/* Check the errmsg variable. */
if (errmsg)
{
diff --git a/gcc/testsuite/gfortran.dg/pr101564.f90
b/gcc/testsuite/gfortran.dg/pr101564.f90
new file mode 100644
index 000..1e7c9911ce6
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr101564.f90
@@ -0,0 +1,9 @@
+! { dg-do compile }
+! PR fortran/101564 - ICE in resolve_allocate_deallocate
+
+program p
+ integer, allocatable :: x(:)
+ integer :: stat
+ allocate (x(2), stat=stat)
+ deallocate (x, stat=stat%kind) ! { dg-error "(STAT variable)" }
+end
-
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
Re: [PATCH] PR fortran/101536 - ICE in gfc_conv_expr_descriptor, at fortran/trans-array.c:7324
Hi Tobias,
you are right in that I was barking up the wrong tree.
I was focussed too much on the testcase in the PR.
> I think that one is wrong. While CLASS_DATA (e) accesses
> e->ts.u.derived->components,
> which always works, your code assumes that there is only 'c' and not 'x%c'
> where
> 'c' is of type BT_CLASS and 'x' is of type BT_DERIVED.
>
> I wonder whether it works if you simply remove 'return true;'
> as gfc_add_class_array_ref sets 'e->rank = CLASS(e)->rank (and
> adds an AR_FULL ref, if needed). In the nonerror case, the
> 'return true' is obtained via:
> if (e->rank != 0 && e->ts.type != BT_PROCEDURE)
> return true;
> And, otherwise, it falls through to the error.
>
> OK if that works
Well, I tried and this does not work.
However, an additional plain check on e->rank != 0 also in the
CLASS cases fixes the original issue as well as your example:
> type t
>class(*), allocatable :: c(:)
> end type t
> type(t) :: x
> x%c = [1,2,3,4]
> print *, size(x%c)
> print *, size(x%c(1)) ! { dg-error ... }
> end
And regtests ok. :-)
See attached updated patch.
Anything else I am missing?
Thanks for the constructive review!
Harald
pr101536.patch-v2
Description: Binary data
Re: [PATCH] PR fortrsn/101564 - ICE in resolve_allocate_deallocate, at fortran/resolve.c:8169
Hi Tobias,
I am afraid we're really opening a can of worms here
> Additionally, I wonder whether that will work with:
>
> integer, pointer :: ptr
> integer function f()
>pointer :: f
>f = ptr
> end
> allocate(A, stat=f())
>
> The f() is a variable and definable – but I am currently not sure it sets
> stat->symtree
> and not only stat->value.function.esym, but I have not tested it.
I think a "working" testcase for this could be:
program p
implicit none
integer, target :: ptr
integer, pointer :: A
allocate (A, stat=f())
print *, ptr
contains
function f()
integer, pointer :: f
f => ptr
end function f
end
This works as expected with Intel and AOCC, but gives a
syntax error with every gfortran tested because of match.c:
alloc_opt_list:
m = gfc_match (" stat = %v", &tmp);
where the comment before gfc_match states:
%v Matches a variable expression (an lvalue)
although it matches only variables and not every type of lvalue.
We therefore never get to the interesting checks elsewhere...
> Additionally, I have to admit that I do not understand the
> following existing condition, which you did not touch:
>
>if ((stat->ts.type != BT_INTEGER
> && !(stat->ref && (stat->ref->type == REF_ARRAY
>|| stat->ref->type == REF_COMPONENT)))
>|| stat->rank > 0)
> gfc_error ("Stat-variable at %L must be a scalar INTEGER "
> "variable", &stat->where);
>
> I mean the ts.type != BT_INTEGER and stat->rank != 0 is clear,
> but what's the reason for the refs?
Well, that needs to be answered by Steve (see commit 3759634).
[...]
> And possibly add a testcase for stat=f() [valid]
> and stat=x%y%kind [invalid] as well?
Well, I need to go back to the drawing board then...
Thanks,
Harald
