* [PATCH] PR fortran/98017 - [8/9/10/11 Regression] Suspected regression using PACK
@ 2020-11-28 22:16 Harald Anlauf
2020-11-29 8:16 ` Thomas Koenig
2020-11-29 10:07 ` Tobias Burnus
0 siblings, 2 replies; 4+ messages in thread
From: Harald Anlauf @ 2020-11-28 22:16 UTC (permalink / raw)
To: fortran, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 822 bytes --]
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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pr98017.patch --]
[-- Type: text/x-patch, Size: 1047 bytes --]
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PR fortran/98017 - [8/9/10/11 Regression] Suspected regression using PACK
2020-11-28 22:16 [PATCH] PR fortran/98017 - [8/9/10/11 Regression] Suspected regression using PACK Harald Anlauf
@ 2020-11-29 8:16 ` Thomas Koenig
2020-11-29 22:34 ` Harald Anlauf
2020-11-29 10:07 ` Tobias Burnus
1 sibling, 1 reply; 4+ messages in thread
From: Thomas Koenig @ 2020-11-29 8:16 UTC (permalink / raw)
To: Harald Anlauf, fortran, gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PR fortran/98017 - [8/9/10/11 Regression] Suspected regression using PACK
2020-11-28 22:16 [PATCH] PR fortran/98017 - [8/9/10/11 Regression] Suspected regression using PACK Harald Anlauf
2020-11-29 8:16 ` Thomas Koenig
@ 2020-11-29 10:07 ` Tobias Burnus
1 sibling, 0 replies; 4+ messages in thread
From: Tobias Burnus @ 2020-11-29 10:07 UTC (permalink / raw)
To: Harald Anlauf, fortran, gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PR fortran/98017 - [8/9/10/11 Regression] Suspected regression using PACK
2020-11-29 8:16 ` Thomas Koenig
@ 2020-11-29 22:34 ` Harald Anlauf
0 siblings, 0 replies; 4+ messages in thread
From: Harald Anlauf @ 2020-11-29 22:34 UTC (permalink / raw)
To: Thomas Koenig; +Cc: fortran, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 949 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pr98017.patch --]
[-- Type: text/x-patch, Size: 1214 bytes --]
diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c
index 32d905ad179..ae9b0a79474 100644
--- a/gcc/fortran/expr.c
+++ b/gcc/fortran/expr.c
@@ -2096,6 +2096,9 @@ simplify_parameter_variable (gfc_expr *p, int type)
return false;
e->rank = p->rank;
+
+ if (e->ts.type == BT_CHARACTER && p->ts.u.cl)
+ e->ts = p->ts;
}
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..ab60407bf1d
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr98017.f90
@@ -0,0 +1,14 @@
+! { dg-do run }
+! PR98017 - [8/9/10/11 Regression] Suspected regression using PACK
+
+program p
+ implicit none
+ character(*), parameter :: s(1) = ['abc()']
+ character(*), parameter :: t(*) = s(:)(:1)
+ if (len (pack (s, s(:)(:1) == 'a')) /= len (s)) stop 1
+ if (any (pack (s, s(:)(:1) == 'a') /= s)) stop 2
+ if (len (pack (s, t == 'a')) /= len (s)) stop 3
+ if (any (pack (s, t == 'a') /= s)) stop 4
+ if (len (pack (s(:)(1:5), t == 'a')) /= len (s)) stop 5
+ if (any (pack (s(:)(1:5), t == 'a') /= s)) stop 6
+end
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-29 22:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-28 22:16 [PATCH] PR fortran/98017 - [8/9/10/11 Regression] Suspected regression using PACK Harald Anlauf
2020-11-29 8:16 ` Thomas Koenig
2020-11-29 22:34 ` Harald Anlauf
2020-11-29 10:07 ` Tobias Burnus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).