diff mbox series

PR fortran/98017 - [8/9/10/11 Regression] Suspected regression using PACK

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

Commit Message

Harald Anlauf Nov. 28, 2020, 10:16 p.m. UTC 
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.

Regtested on x86_64-pc-linux-gnu.  OK for mainline, and since this is a
bad code regression, backports down to 8-branch?

Thanks,
Harald


PR fortran/98017 - Suspected regression using PACK

When substituting a parameter variable of type character, the character
length was reset to 1.  Fix this by copying the length.

gcc/fortran/ChangeLog:

	* expr.c (simplify_parameter_variable): Fix up character length
	after copying an array-valued expression.

gcc/testsuite/ChangeLog:

	* gfortran.dg/pr98017.f90: New test.

Comments

Thomas Koenig Nov. 29, 2020, 8:16 a.m. UTC | #1 
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
Tobias Burnus Nov. 29, 2020, 10:07 a.m. UTC | #2 
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
Harald Anlauf Nov. 29, 2020, 10:34 p.m. UTC | #3 
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 mbox series

Patch

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