[fortran] PR87477 - (associate) - [meta-bug] [F03] issues concerning the ASSOCIATE statement
Checks
Commit Message
Hi All,
Three more fixes for PR87477. Please note that PR99350 was a blocker
but, as pointed out in comment #5 of the PR, this has nothing to do
with the associate construct.
All three fixes are straight forward and the .diff + ChangeLog suffice
to explain them. 'rankguessed' was made redundant by the last PR87477
fix.
Regtests on x86_64 - good for mainline?
Paul
Fortran: Fix some more blockers in associate meta-bug [PR87477]
2023-06-07 Paul Thomas <pault@gcc.gnu.org>
gcc/fortran
PR fortran/99350
* decl.cc (char_len_param_value): Simplify a copy of the expr
and replace the original if there is no error.
* gfortran.h : Remove the redundant field 'rankguessed' from
'gfc_association_list'.
* resolve.cc (resolve_assoc_var): Remove refs to 'rankguessed'.
PR fortran/107281
* resolve.cc (resolve_variable): Associate names with constant
or structure constructor targets cannot have array refs.
PR fortran/109451
* trans-array.cc (gfc_conv_expr_descriptor): Guard expression
character length backend decl before using it. Suppress the
assignment if lhs equals rhs.
* trans-io.cc (gfc_trans_transfer): Scalarize transfer of
associate variables pointing to a variable. Add comment.
* trans-stmt.cc (trans_associate_var): Remove requirement that
the character length be deferred before assigning the value
returned by gfc_conv_expr_descriptor. Also, guard the backend
decl before testing with VAR_P.
gcc/testsuite/
PR fortran/99350
* gfortran.dg/pr99350.f90 : New test.
PR fortran/107281
* gfortran.dg/associate_5.f03 : Changed error message.
* gfortran.dg/pr107281.f90 : New test.
PR fortran/109451
* gfortran.dg/associate_61.f90 : New test
Comments
Hi Paul!
On 6/7/23 18:10, Paul Richard Thomas via Gcc-patches wrote:
> Hi All,
>
> Three more fixes for PR87477. Please note that PR99350 was a blocker
> but, as pointed out in comment #5 of the PR, this has nothing to do
> with the associate construct.
>
> All three fixes are straight forward and the .diff + ChangeLog suffice
> to explain them. 'rankguessed' was made redundant by the last PR87477
> fix.
>
> Regtests on x86_64 - good for mainline?
>
> Paul
>
> Fortran: Fix some more blockers in associate meta-bug [PR87477]
>
> 2023-06-07 Paul Thomas <pault@gcc.gnu.org>
>
> gcc/fortran
> PR fortran/99350
> * decl.cc (char_len_param_value): Simplify a copy of the expr
> and replace the original if there is no error.
This seems to lack a gfc_free_expr (p) in case the gfc_replace_expr
is not executed, leading to a possible memleak. Can you check?
@@ -1081,10 +1082,10 @@ char_len_param_value (gfc_expr **expr, bool
*deferred)
if (!gfc_expr_check_typed (*expr, gfc_current_ns, false))
return MATCH_ERROR;
- /* If gfortran gets an EXPR_OP, try to simplify it. This catches things
- like CHARACTER(([1])). */
- if ((*expr)->expr_type == EXPR_OP)
- gfc_simplify_expr (*expr, 1);
+ /* Try to simplify the expression to catch things like
CHARACTER(([1])). */
+ p = gfc_copy_expr (*expr);
+ if (gfc_is_constant_expr (p) && gfc_simplify_expr (p, 1))
+ gfc_replace_expr (*expr, p);
else
gfc_free_expr (p);
> * gfortran.h : Remove the redundant field 'rankguessed' from
> 'gfc_association_list'.
> * resolve.cc (resolve_assoc_var): Remove refs to 'rankguessed'.
>
> PR fortran/107281
> * resolve.cc (resolve_variable): Associate names with constant
> or structure constructor targets cannot have array refs.
>
> PR fortran/109451
> * trans-array.cc (gfc_conv_expr_descriptor): Guard expression
> character length backend decl before using it. Suppress the
> assignment if lhs equals rhs.
> * trans-io.cc (gfc_trans_transfer): Scalarize transfer of
> associate variables pointing to a variable. Add comment.
> * trans-stmt.cc (trans_associate_var): Remove requirement that
> the character length be deferred before assigning the value
> returned by gfc_conv_expr_descriptor. Also, guard the backend
> decl before testing with VAR_P.
>
> gcc/testsuite/
> PR fortran/99350
> * gfortran.dg/pr99350.f90 : New test.
>
> PR fortran/107281
> * gfortran.dg/associate_5.f03 : Changed error message.
> * gfortran.dg/pr107281.f90 : New test.
>
> PR fortran/109451
> * gfortran.dg/associate_61.f90 : New test
Otherwise LGTM.
Thanks for the patch!
Harald
Hi Harald,
In answer to your question:
void
gfc_replace_expr (gfc_expr *dest, gfc_expr *src)
{
free_expr0 (dest);
*dest = *src;
free (src);
}
So it does indeed do the job.
I should perhaps have remarked that, following the divide error,
gfc_simplify_expr was returning a mutilated version of the expression
and this was somehow connected with successfully simplifying the
parentheses. Copying and replacing on no errors deals with the
problem.
Thanks
Paul
On Wed, 7 Jun 2023 at 19:38, Harald Anlauf <anlauf@gmx.de> wrote:
>
> Hi Paul!
>
> On 6/7/23 18:10, Paul Richard Thomas via Gcc-patches wrote:
> > Hi All,
> >
> > Three more fixes for PR87477. Please note that PR99350 was a blocker
> > but, as pointed out in comment #5 of the PR, this has nothing to do
> > with the associate construct.
> >
> > All three fixes are straight forward and the .diff + ChangeLog suffice
> > to explain them. 'rankguessed' was made redundant by the last PR87477
> > fix.
> >
> > Regtests on x86_64 - good for mainline?
> >
> > Paul
> >
> > Fortran: Fix some more blockers in associate meta-bug [PR87477]
> >
> > 2023-06-07 Paul Thomas <pault@gcc.gnu.org>
> >
> > gcc/fortran
> > PR fortran/99350
> > * decl.cc (char_len_param_value): Simplify a copy of the expr
> > and replace the original if there is no error.
>
> This seems to lack a gfc_free_expr (p) in case the gfc_replace_expr
> is not executed, leading to a possible memleak. Can you check?
>
> @@ -1081,10 +1082,10 @@ char_len_param_value (gfc_expr **expr, bool
> *deferred)
> if (!gfc_expr_check_typed (*expr, gfc_current_ns, false))
> return MATCH_ERROR;
>
> - /* If gfortran gets an EXPR_OP, try to simplify it. This catches things
> - like CHARACTER(([1])). */
> - if ((*expr)->expr_type == EXPR_OP)
> - gfc_simplify_expr (*expr, 1);
> + /* Try to simplify the expression to catch things like
> CHARACTER(([1])). */
> + p = gfc_copy_expr (*expr);
> + if (gfc_is_constant_expr (p) && gfc_simplify_expr (p, 1))
> + gfc_replace_expr (*expr, p);
> else
> gfc_free_expr (p);
>
> > * gfortran.h : Remove the redundant field 'rankguessed' from
> > 'gfc_association_list'.
> > * resolve.cc (resolve_assoc_var): Remove refs to 'rankguessed'.
> >
> > PR fortran/107281
> > * resolve.cc (resolve_variable): Associate names with constant
> > or structure constructor targets cannot have array refs.
> >
> > PR fortran/109451
> > * trans-array.cc (gfc_conv_expr_descriptor): Guard expression
> > character length backend decl before using it. Suppress the
> > assignment if lhs equals rhs.
> > * trans-io.cc (gfc_trans_transfer): Scalarize transfer of
> > associate variables pointing to a variable. Add comment.
> > * trans-stmt.cc (trans_associate_var): Remove requirement that
> > the character length be deferred before assigning the value
> > returned by gfc_conv_expr_descriptor. Also, guard the backend
> > decl before testing with VAR_P.
> >
> > gcc/testsuite/
> > PR fortran/99350
> > * gfortran.dg/pr99350.f90 : New test.
> >
> > PR fortran/107281
> > * gfortran.dg/associate_5.f03 : Changed error message.
> > * gfortran.dg/pr107281.f90 : New test.
> >
> > PR fortran/109451
> > * gfortran.dg/associate_61.f90 : New test
>
> Otherwise LGTM.
>
> Thanks for the patch!
>
> Harald
>
>
Le 08/06/2023 à 07:57, Paul Richard Thomas via Fortran a écrit :
> Hi Harald,
>
> In answer to your question:
> void
> gfc_replace_expr (gfc_expr *dest, gfc_expr *src)
> {
> free_expr0 (dest);
> *dest = *src;
> free (src);
> }
> So it does indeed do the job.
>
Sure, but his comment was about the case gfc_replace_expr is *not* executed.
> I should perhaps have remarked that, following the divide error,
> gfc_simplify_expr was returning a mutilated version of the expression
> and this was somehow connected with successfully simplifying the
> parentheses. Copying and replacing on no errors deals with the
> problem.
>
Is the expression mutilated enough that it can't be safely freed?
On 6/8/23 09:46, Mikael Morin wrote:
> Le 08/06/2023 à 07:57, Paul Richard Thomas via Fortran a écrit :
>> Hi Harald,
>>
>> In answer to your question:
>> void
>> gfc_replace_expr (gfc_expr *dest, gfc_expr *src)
>> {
>> free_expr0 (dest);
>> *dest = *src;
>> free (src);
>> }
>> So it does indeed do the job.
>>
> Sure, but his comment was about the case gfc_replace_expr is *not*
> executed.
Right. The following legal code exhibits the leak, pointing
to the gfc_copy_expr:
subroutine paul (n)
integer :: n
character(n) :: c
end
>> I should perhaps have remarked that, following the divide error,
>> gfc_simplify_expr was returning a mutilated version of the expression
>> and this was somehow connected with successfully simplifying the
>> parentheses. Copying and replacing on no errors deals with the
>> problem.
>>
> Is the expression mutilated enough that it can't be safely freed?
>
>
>
Thanks Gents!
The solution is to gfc_free_expr (p) if the replacement is not made.
I am regtesting a patch for PR107900. I'll include the fix for the
memory leak in the patch for that.
Cheers
Paul
On Thu, 8 Jun 2023 at 09:30, Harald Anlauf <anlauf@gmx.de> wrote:
>
> On 6/8/23 09:46, Mikael Morin wrote:
> > Le 08/06/2023 à 07:57, Paul Richard Thomas via Fortran a écrit :
> >> Hi Harald,
> >>
> >> In answer to your question:
> >> void
> >> gfc_replace_expr (gfc_expr *dest, gfc_expr *src)
> >> {
> >> free_expr0 (dest);
> >> *dest = *src;
> >> free (src);
> >> }
> >> So it does indeed do the job.
> >>
> > Sure, but his comment was about the case gfc_replace_expr is *not*
> > executed.
>
> Right. The following legal code exhibits the leak, pointing
> to the gfc_copy_expr:
>
> subroutine paul (n)
> integer :: n
> character(n) :: c
> end
>
> >> I should perhaps have remarked that, following the divide error,
> >> gfc_simplify_expr was returning a mutilated version of the expression
> >> and this was somehow connected with successfully simplifying the
> >> parentheses. Copying and replacing on no errors deals with the
> >> problem.
> >>
> > Is the expression mutilated enough that it can't be safely freed?
> >
> >
> >
>
@@ -1056,6 +1056,7 @@ static match
char_len_param_value (gfc_expr **expr, bool *deferred)
{
match m;
+ gfc_expr *p;
*expr = NULL;
*deferred = false;
@@ -1081,10 +1082,10 @@ char_len_param_value (gfc_expr **expr, bool *deferred)
if (!gfc_expr_check_typed (*expr, gfc_current_ns, false))
return MATCH_ERROR;
- /* If gfortran gets an EXPR_OP, try to simplify it. This catches things
- like CHARACTER(([1])). */
- if ((*expr)->expr_type == EXPR_OP)
- gfc_simplify_expr (*expr, 1);
+ /* Try to simplify the expression to catch things like CHARACTER(([1])). */
+ p = gfc_copy_expr (*expr);
+ if (gfc_is_constant_expr (p) && gfc_simplify_expr (p, 1))
+ gfc_replace_expr (*expr, p);
if ((*expr)->expr_type == EXPR_FUNCTION)
{
@@ -2914,9 +2914,6 @@ typedef struct gfc_association_list
for memory handling. */
unsigned dangling:1;
- /* True when the rank of the target expression is guessed during parsing. */
- unsigned rankguessed:1;
-
char name[GFC_MAX_SYMBOL_LEN + 1];
gfc_symtree *st; /* Symtree corresponding to name. */
locus where;
@@ -5872,7 +5872,15 @@ resolve_variable (gfc_expr *e)
if (sym->ts.type == BT_CLASS)
gfc_fix_class_refs (e);
if (!sym->attr.dimension && e->ref && e->ref->type == REF_ARRAY)
- return false;
+ {
+ /* Unambiguously scalar! */
+ if (sym->assoc->target
+ && (sym->assoc->target->expr_type == EXPR_CONSTANT
+ || sym->assoc->target->expr_type == EXPR_STRUCTURE))
+ gfc_error ("Scalar variable %qs has an array reference at %L",
+ sym->name, &e->where);
+ return false;
+ }
else if (sym->attr.dimension && (!e->ref || e->ref->type != REF_ARRAY))
{
/* This can happen because the parser did not detect that the
@@ -9279,7 +9287,7 @@ resolve_assoc_var (gfc_symbol* sym, bool resolve_target)
gfc_array_spec *as;
/* The rank may be incorrectly guessed at parsing, therefore make sure
it is corrected now. */
- if (sym->ts.type != BT_CLASS && (!sym->as || sym->assoc->rankguessed))
+ if (sym->ts.type != BT_CLASS && !sym->as)
{
if (!sym->as)
sym->as = gfc_get_array_spec ();
@@ -9292,8 +9300,7 @@ resolve_assoc_var (gfc_symbol* sym, bool resolve_target)
sym->attr.codimension = 1;
}
else if (sym->ts.type == BT_CLASS
- && CLASS_DATA (sym)
- && (!CLASS_DATA (sym)->as || sym->assoc->rankguessed))
+ && CLASS_DATA (sym) && !CLASS_DATA (sym)->as)
{
if (!CLASS_DATA (sym)->as)
CLASS_DATA (sym)->as = gfc_get_array_spec ();
@@ -7934,7 +7934,8 @@ gfc_conv_expr_descriptor (gfc_se *se, gfc_expr *expr)
else
tmp = se->string_length;
- if (expr->ts.deferred && VAR_P (expr->ts.u.cl->backend_decl))
+ if (expr->ts.deferred && expr->ts.u.cl->backend_decl
+ && VAR_P (expr->ts.u.cl->backend_decl))
gfc_add_modify (&se->pre, expr->ts.u.cl->backend_decl, tmp);
else
expr->ts.u.cl->backend_decl = tmp;
@@ -7999,6 +8000,15 @@ gfc_conv_expr_descriptor (gfc_se *se, gfc_expr *expr)
}
}
+ if (expr->ts.type == BT_CHARACTER
+ && VAR_P (TYPE_SIZE_UNIT (gfc_get_element_type (TREE_TYPE (parm)))))
+ {
+ tree elem_len = TYPE_SIZE_UNIT (gfc_get_element_type (TREE_TYPE (parm)));
+ gfc_add_modify (&loop.pre, elem_len,
+ fold_convert (TREE_TYPE (elem_len),
+ gfc_get_array_span (desc, expr)));
+ }
+
/* Set the span field. */
tmp = NULL_TREE;
if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (desc)))
@@ -2620,9 +2620,13 @@ gfc_trans_transfer (gfc_code * code)
gcc_assert (ref && ref->type == REF_ARRAY);
}
+ /* These expressions don't always have the dtype element length set
+ correctly, rendering them useless for array transfer. */
if (expr->ts.type != BT_CLASS
&& expr->expr_type == EXPR_VARIABLE
&& ((expr->symtree->n.sym->ts.type == BT_DERIVED && expr->ts.deferred)
+ || (expr->symtree->n.sym->assoc
+ && expr->symtree->n.sym->assoc->variable)
|| gfc_expr_attr (expr).pointer))
goto scalarize;
@@ -1930,15 +1930,13 @@ trans_associate_var (gfc_symbol *sym, gfc_wrapped_block *block)
gfc_conv_expr_descriptor (&se, e);
if (sym->ts.type == BT_CHARACTER
- && sym->ts.deferred
&& !sym->attr.select_type_temporary
+ && sym->ts.u.cl->backend_decl
&& VAR_P (sym->ts.u.cl->backend_decl)
&& se.string_length != sym->ts.u.cl->backend_decl)
- {
- gfc_add_modify (&se.pre, sym->ts.u.cl->backend_decl,
+ gfc_add_modify (&se.pre, sym->ts.u.cl->backend_decl,
fold_convert (TREE_TYPE (sym->ts.u.cl->backend_decl),
se.string_length));
- }
/* If we didn't already do the pointer assignment, set associate-name
descriptor to the one generated for the temporary. */
@@ -11,7 +11,7 @@ PROGRAM main
INTEGER, POINTER :: ptr
ASSOCIATE (a => 5) ! { dg-error "is used as array" }
- PRINT *, a(3)
+ PRINT *, a(3) ! { dg-error "has an array reference" }
END ASSOCIATE
ASSOCIATE (a => nontarget)