Message ID | trinity-c8a9dd37-c359-4424-a864-155fc9645a52-1606601800712@3c-app-gmx-bs11 |
---|---|
State | New |
Headers | show |
Series | PR fortran/98017 - [8/9/10/11 Regression] Suspected regression using PACK | expand |
HI Harald, > When substituting an array-valued character parameter variable, the call to > gfc_copy_expr returns character length 1. Fix up the resulting length. > > I could not figure out whether this is a bug or a feature of gfc_copy_expr. > But the fix to simplify_parameter_variable would not do any harm in any case. Thanks for your analysis! I don't think that this is a feature of gfc_copy_expr, I think it is a bug which probably also bites us in other circumstances which we may work around in other places and/or which causes other instances of wrong code. So, I'd very much prefer that the character length is set correctly in gfc_copy_expr. If that works, we should definitely backport to all open branches. Best regards Thomas
On 28.11.20 23:16, Harald Anlauf wrote: > When substituting an array-valued character parameter variable, the call to > gfc_copy_expr returns character length 1. Fix up the resulting length. > [...] I disagree. > @@ -2096,6 +2096,10 @@ simplify_parameter_variable (gfc_expr *p, int type) > return false; > > e->rank = p->rank; > + > + /* Fix up character length since gfc_copy_expr may not preserve it. */ > + if (e->ts.type == BT_CHARACTER && p->ts.u.cl) > + e->ts.u.cl = gfc_new_charlen (gfc_current_ns, p->ts.u.cl); The comment looks wrong and I think also that the fix is at the wrong place. else { e = gfc_copy_expr (p->symtree->n.sym->value); ... Here, "p" is "s(AR_FULL)". p->symtree->n.sym's ts.u.cl also has the correct length; the only problem is that 'p->symtree->n.sym->value' has the wrong length. The question is what sets the wrong string length of 1? I think that should be fixed. As band aid, you could use: e->ts = p->ts; or e->ts = p->symtree->n.sym->ts; or either of those but with '.u.cl' appended + a FIXME comment that '->value->ts.u.cl' might be wrong. That is acceptable to me. But I am against the current patch – it pointlessly duplicates the string length variable and the comment is completely misleading as it is fake news. Tobias
Hi Thomas, > I don't think that this is a feature of gfc_copy_expr, I think it is a > bug which probably also bites us in other circumstances which we may > work around in other places and/or which causes other instances of > wrong code. > > So, I'd very much prefer that the character length is set correctly > in gfc_copy_expr. If that works, we should definitely backport > to all open branches. I spent some time on understanding what happens. But after receiving Tobias' mail, I sort of lost motivation to pursue this further. I've committed a slightly adjusted fix with a slightly enhanced testcase that better exhibits that there were actually two regressions: one with gcc-8, and another one with gcc-9. Just look at the tree-dumps for the attached version. Unless somebody stops me in a constructive way, I'll backport very slowly - and hopefully carefully - but not waste any time on this. Thanks, Harald
diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c index 32d905ad179..8f1a3a34053 100644 --- a/gcc/fortran/expr.c +++ b/gcc/fortran/expr.c @@ -2096,6 +2096,10 @@ simplify_parameter_variable (gfc_expr *p, int type) return false; e->rank = p->rank; + + /* Fix up character length since gfc_copy_expr may not preserve it. */ + if (e->ts.type == BT_CHARACTER && p->ts.u.cl) + e->ts.u.cl = gfc_new_charlen (gfc_current_ns, p->ts.u.cl); } if (e->ts.type == BT_CHARACTER && e->ts.u.cl == NULL) diff --git a/gcc/testsuite/gfortran.dg/pr98017.f90 b/gcc/testsuite/gfortran.dg/pr98017.f90 new file mode 100644 index 00000000000..24d7adadb40 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/pr98017.f90 @@ -0,0 +1,9 @@ +! { dg-do run } +! PR98017 - [8/9/10/11 Regression] Suspected regression using PACK + +program p + implicit none + character(*), parameter :: s(1) = ['abc()'] + if (len (pack (s, s(:)(:1) =='a')) /= len (s)) stop 1 + if (any (pack (s, s(:)(:1) =='a') /= s)) stop 2 +end