Hi all,
thanks for the explanations, Christophe. This is very much appreciated. And
sorry, I can't follow all presentations, conferences and publications. There is
meanwhile way too much for me to process out there.
Anyway, the regression I produced in gomp should be fixed by the new version of
patch attached. I only can speculate what is going wrong with the first version
of the patch. Obviously the type of array the first patch introduced can not be
layed out correctly. There is a mult of arg 0 NOP arg 0 NOP in there which is
more than odd. I would expect something like mult arg 0 .len arg 1 char_size.
The second version of the patch is some what longer, because it no longer tries
to change the type of the static allocatable string to a char array, but sticks
to the pointer to char there. In the gfc_conv_substring pointer arithmetic is
applied to compute the correct offset of the reference.
Regtests ok on x86_64-pc-linux-gnu / F41. Ok for mainline?
Regards,
Andre
On Tue, 3 Jun 2025 10:14:29 +0200
Christophe Lyon <christophe.l...@linaro.org> wrote:
> Hi!
>
>
> On Mon, 2 Jun 2025 at 20:53, Andre Vehreschild <ve...@gmx.de> wrote:
> >
> > Hi Thomas,
> >
> > thanks for the ok. Unfortunately does the patch regress in gomp (test case
> > gomp/pr104382 when I am not mistaken ; the one with the lone 'save'
> > statement). This was reported by the regression testing host at first for
> > arm, but also occurs on x86_64. Since when are proposed patches checked by
> > a CI? That's fantastic!
>
> On the Linaro side, we put "postcommit" CI in production e/o summer
> 2023, and "precommit" CI a few weeks/months later. We made
> presentations during the GNU Cauldron 2023 and 2024, as well as during
> Linaro Connect 2024 :-) In summary we test various configurations of
> "arm" and "aarch64" targets.
>
> If you didn't notice before, it's because your patches were regression-free
> :-)
>
> Always happy to read positive feedback!
>
> Thanks
>
> Christophe
>
> > I will continue to investigate how to fix that issue.
> >
> > Regards,
> > Andre
> > Andre Vehreschild * ve...@gmx.de
> >
> > Am 2. Juni 2025 20:10:06 schrieb Thomas Koenig <tkoe...@netcologne.de>:
> >
> >> Hi Andre,
> >>
> >>
> >>> attached patch fixes a missing substring ref on a saved allocatable
> >>> string. The issue seems to be, that the variable is declared to be a
> >>> character pointer and not a character array. When using the latter (why
> >>> not), it works as expected and does not produce any regressions.
> >>>
> >>> Regtests ok on x86_64-pc-linux-gnu / F41. Ok for mainlines?
> >>
> >>
> >> OK for trunk and also for backporting to gcc 15 (it is a 15/16
> >> regression).
> >>
> >> Best regards
> >>
> >> Thomas
> >
> >
--
Andre Vehreschild * Email: vehre ad gmx dot de
From 4edf6439d56034954d44c53a8e078a9ab7769967 Mon Sep 17 00:00:00 2001
From: Andre Vehreschild <ve...@gcc.gnu.org>
Date: Mon, 2 Jun 2025 10:41:48 +0200
Subject: [PATCH] Fortran: Fix missing substring ref for allocatable saved vars
[PR120483]
Compute a substring ref on an allocatable static character array
using pointer arithmetic. Using an array type corrupts type
layouting and crashes omp generation.
PR fortran/120483
gcc/fortran/ChangeLog:
* trans-expr.cc (gfc_conv_substring): Use pointer arithmetic on
static allocatable char arrays.
gcc/testsuite/ChangeLog:
* gfortran.dg/save_8.f90: New test.
---
gcc/fortran/trans-expr.cc | 16 +++++++++++++---
gcc/testsuite/gfortran.dg/save_8.f90 | 13 +++++++++++++
2 files changed, 26 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/save_8.f90
diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index 8d9448eb9b6..74d4265f27d 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -2782,9 +2782,11 @@ gfc_conv_substring (gfc_se * se, gfc_ref * ref, int kind,
start.expr = gfc_evaluate_now (start.expr, &se->pre);
/* Change the start of the string. */
- if ((TREE_CODE (TREE_TYPE (se->expr)) == ARRAY_TYPE
- || TREE_CODE (TREE_TYPE (se->expr)) == INTEGER_TYPE)
- && TYPE_STRING_FLAG (TREE_TYPE (se->expr)))
+ if (((TREE_CODE (TREE_TYPE (se->expr)) == ARRAY_TYPE
+ || TREE_CODE (TREE_TYPE (se->expr)) == INTEGER_TYPE)
+ && TYPE_STRING_FLAG (TREE_TYPE (se->expr)))
+ || (POINTER_TYPE_P (TREE_TYPE (se->expr))
+ && TREE_CODE (TREE_TYPE (TREE_TYPE (se->expr))) != ARRAY_TYPE))
tmp = se->expr;
else
tmp = build_fold_indirect_ref_loc (input_location,
@@ -2795,6 +2797,14 @@ gfc_conv_substring (gfc_se * se, gfc_ref * ref, int kind,
tmp = gfc_build_array_ref (tmp, start.expr, NULL_TREE, true);
se->expr = gfc_build_addr_expr (type, tmp);
}
+ else if (POINTER_TYPE_P (TREE_TYPE (tmp)))
+ {
+ tree diff;
+ diff = fold_build2 (MINUS_EXPR, size_type_node, start.expr,
+ build_one_cst (size_type_node));
+ se->expr
+ = fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (tmp), tmp, diff);
+ }
}
/* Length = end + 1 - start. */
diff --git a/gcc/testsuite/gfortran.dg/save_8.f90 b/gcc/testsuite/gfortran.dg/save_8.f90
new file mode 100644
index 00000000000..8e9198caeb1
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/save_8.f90
@@ -0,0 +1,13 @@
+!{ dg-do run }
+
+! Check PR120483 is fixed.
+! Contributed by Thomas Koenig <tkoe...@gcc.gnu.org>
+! and Peter Güntert <pe...@guentert.com>
+
+program save_8
+ implicit none
+ character(len=:), allocatable, save :: s1
+ s1 = 'ABC'
+ if (s1(3:3) /= 'C') stop 1
+end program save_8
+
--
2.49.0
