Re: [Patch, fortran] PR102689 - Segfault with RESHAPE of CLASS as actual argument
Hi Paul,
In
@@ -1335,19 +1340,49 @@ get_class_info_from_ss (stmtblock_t * pre, gfc_ss *ss,
tree *eltype) rhs_function = true;
}
}
+ else if (cntnr != NULL_TREE)
+{
+ tmp = gfc_class_vptr_get (rhs_class_expr);
+ gfc_add_modify (pre, tmp, fold_convert (TREE_TYPE (tmp),
+ gfc_class_vptr_get (cntnr)));
+ if (unlimited_rhs)
+ {
+ tmp = gfc_class_len_get (rhs_class_expr);
+ if (unlimited_arg1)
+ gfc_add_modify (pre, tmp, gfc_class_len_get (cntnr));
+ }
+ tmp = NULL_TREE;
+}
else
tmp = NULL_TREE;
I am confused here, because you are assigning to rhs. When that is correct, why
is there no else assigning zero to the rhs->_len when arg1 is not unlimited?
@@ -1176,6 +1176,21 @@ gfc_conv_class_to_class (gfc_se *parmse, gfc_expr *e,
gfc_typespec class_ts, stmtblock_t block;
bool full_array = false;
+ /* Class transformational function results are the data field of a class
+ temporary and so the class expression canbe obtained directly. */
s/canbe/can be/
Besides those small knits the patch looks fine to me. I am wondering though,
why gfortran is still using a non-class aware pack? To "move" the content of a
class object the _copy function of the vtype is to be used, right? In my
current PR I try to implement a class aware internal pack (and unpack) to
correctly apply the element sequence as of F2018 15.5.2.11. But I am
struggling when the rank changes. I found the idea how to do this correctly in
your code, thanks.
Regards,
Andre
On Mon, 1 Jul 2024 22:17:11 +0100
Paul Richard Thomas wrote:
> Hi All,
>
> This is one of those PRs where one thing led to another I think that
> the patch is pretty complete and, while apparently quite heavy, is more or
> less self explanatory through comments and the ChangeLog.
>
> The first testcase concentrates on reshape in various guises, while the
> second deals with all the other affected transformational intrinsic
> functions. In the first, most of the test statements are factored out into
> their own subroutines in order to expose the code generated for each. This
> was essential for the debugging but can be undone if preferred.
>
> Regtests just fine - OK for mainline?
>
> Paul
--
Andre Vehreschild * Email: vehre ad gmx dot de
Re: [Patch, fortran] PR102689 - Segfault with RESHAPE of CLASS as actual argument
Hi Andre, Thank you for the review. > ...snip... > > I am confused here, because you are assigning to rhs. When that is > correct, why > is there no else assigning zero to the rhs->_len when arg1 is not > unlimited? 'rhs_class_expr' is highly confusing and came from the original use of this part of the code. With this function call, it is actually the lhs! I will think of some less confusing name and extend the comment above its extraction from the ss chain. d._vptr = b._vptr; d._len = b._len;// Here is the assignment that you pointed out. D.5162 = d._vptr != 0B ? d._vptr->_size : 0; D.5163 = D.5162; D.5164 = d._len; D.5165 = D.5164 > 0 ? D.5163 * D.5164 : D.5163; typedef character(kind=1) [10][1:D.5165]; ctmp.123 = d; // This looks a bit silly but it is effective for more complicated objects - eg. class components. ctmp.123._data = atmp.122; ctmp.123._data.dtype = d._data.dtype; ctmp.123._data.dtype.rank = 1; ctmp.123._data.dim[0].stride = 1; ctmp.123._data.dim[0].lbound = 0; ctmp.123._data.dim[0].ubound = 9; ctmp.123._data.span = D.5165; D.5174 = (void * restrict) __builtin_malloc (MAX_EXPR <(unsigned long) (D.5165 * 10), 1>); D.5175 = D.5174; ctmp.123._data.data = D.5175; ctmp.123._data.offset = 0; _gfortran_reshape (&ctmp.123._data, D.5152, D.5161, 0B, 0B); > > @@ -1176,6 +1176,21 @@ gfc_conv_class_to_class (gfc_se *parmse, gfc_expr > *e, > gfc_typespec class_ts, stmtblock_t block; >bool full_array = false; > > + /* Class transformational function results are the data field of a class > + temporary and so the class expression canbe obtained directly. */ > > s/canbe/can be/ > > Indeed! > > Besides those small knits the patch looks fine to me. I am wondering > though, > why gfortran is still using a non-class aware pack? To "move" the content > of a > class object the _copy function of the vtype is to be used, right? In my > current PR I try to implement a class aware internal pack (and unpack) to > correctly apply the element sequence as of F2018 15.5.2.11. But I am > struggling when the rank changes. I found the idea how to do this > correctly in > your code, thanks. > It crossed my mind that class aware transformationals would have been the path of least resistance *after* I had fought my way through the ss chains. The full list of affected transformational intrinsics that operate on any type is found in the second testcase. If you tackle pack first, I would be happy to do the rest and to assign this patch to the dustbin of history. It should be rather straightforward to provide an interface to the existing library functions that produces significantly less inline code than my implementation. I will commit the patch, though, and will revert as and when class-aware library functions are in place. Thanks again Paul
Re: [Patch, fortran] PR102689 - Segfault with RESHAPE of CLASS as actual argument
Hi Paul, yes, please go ahead with the merge. To my astonishment, I had no conflicts with your patch. Mine is addressing copy-in/(out) aka packing/unpacking of derived-type to class-type arguments. Thanks for the patch. - Andre On Tue, 2 Jul 2024 09:21:26 +0100 Paul Richard Thomas wrote: > Hi Andre, > > Thank you for the review. > > > > ...snip... > > > > I am confused here, because you are assigning to rhs. When that is > > correct, why > > is there no else assigning zero to the rhs->_len when arg1 is not > > unlimited? > > > 'rhs_class_expr' is highly confusing and came from the original use of this > part of the code. With this function call, it is actually the lhs! I will > think of some less confusing name and extend the comment above its > extraction from the ss chain. > > d._vptr = b._vptr; > d._len = b._len;// Here is the assignment that you pointed out. > D.5162 = d._vptr != 0B ? d._vptr->_size : 0; > D.5163 = D.5162; > D.5164 = d._len; > D.5165 = D.5164 > 0 ? D.5163 * D.5164 : D.5163; > typedef character(kind=1) [10][1:D.5165]; > ctmp.123 = d; // This looks a bit silly but it is effective for > more complicated objects - eg. class components. > ctmp.123._data = atmp.122; > ctmp.123._data.dtype = d._data.dtype; > ctmp.123._data.dtype.rank = 1; > ctmp.123._data.dim[0].stride = 1; > ctmp.123._data.dim[0].lbound = 0; > ctmp.123._data.dim[0].ubound = 9; > ctmp.123._data.span = D.5165; > D.5174 = (void * restrict) __builtin_malloc (MAX_EXPR <(unsigned long) > (D.5165 * 10), 1>); > D.5175 = D.5174; > ctmp.123._data.data = D.5175; > ctmp.123._data.offset = 0; > _gfortran_reshape (&ctmp.123._data, D.5152, D.5161, 0B, 0B); > > > > > > @@ -1176,6 +1176,21 @@ gfc_conv_class_to_class (gfc_se *parmse, gfc_expr > > *e, > > gfc_typespec class_ts, stmtblock_t block; > >bool full_array = false; > > > > + /* Class transformational function results are the data field of a class > > + temporary and so the class expression canbe obtained directly. */ > > > > s/canbe/can be/ > > > > Indeed! > > > > > Besides those small knits the patch looks fine to me. I am wondering > > though, > > why gfortran is still using a non-class aware pack? To "move" the content > > of a > > class object the _copy function of the vtype is to be used, right? In my > > current PR I try to implement a class aware internal pack (and unpack) to > > correctly apply the element sequence as of F2018 15.5.2.11. But I am > > struggling when the rank changes. I found the idea how to do this > > correctly in > > your code, thanks. > > > > It crossed my mind that class aware transformationals would have been the > path of least resistance *after* I had fought my way through the ss chains. > The full list of affected transformational intrinsics that operate on any > type is found in the second testcase. If you tackle pack first, I would be > happy to do the rest and to assign this patch to the dustbin of history. It > should be rather straightforward to provide an interface to the existing > library functions that produces significantly less inline code than my > implementation. > > I will commit the patch, though, and will revert as and when class-aware > library functions are in place. > > Thanks again > > Paul -- Andre Vehreschild * Email: vehre ad gmx dot de
Fwd: [Linaro-TCWG-CI] gcc patch #93154: FAIL: 1 regressions on arm
Hi Andre, I have to hold back on the commit until the business below is sorted out. Cheers Paul -- Forwarded message - From: Paul Richard Thomas Date: Tue, 2 Jul 2024 at 08:48 Subject: [Linaro-TCWG-CI] gcc patch #93154: FAIL: 1 regressions on arm To: Hi there, You detected a failure in gfortran.dg/class_transformational_2.f90: PASS: gfortran.dg/class_transformational_2.f90 -O0 (test for excess errors) PASS: gfortran.dg/class_transformational_2.f90 -O0 execution test PASS: gfortran.dg/class_transformational_2.f90 -O1 (test for excess errors) FAIL: gfortran.dg/class_transformational_2.f90 -O1 execution test PASS: gfortran.dg/class_transformational_2.f90 -O2 (test for excess errors) PASS: gfortran.dg/class_transformational_2.f90 -O2 execution test PASS: gfortran.dg/class_transformational_2.f90 -O3 -fomit-frame-pointer ...snip... PASS: gfortran.dg/class_transformational_2.f90 -O3 -fomit-frame-pointer ...snip... PASS: gfortran.dg/class_transformational_2.f90 -O3 -g (test for excess errors) PASS: gfortran.dg/class_transformational_2.f90 -O3 -g execution test PASS: gfortran.dg/class_transformational_2.f90 -Os (test for excess errors) PASS: gfortran.dg/class_transformational_2.f90 -Os execution test The stop message in the full log indicates a numeric error in the first test. I am unable to reproduce the error. Adding deallocation of all the allocated variables (which I should have done in the first place) and running valgrind with -s shows no errors and no memory loss. I find it odd that it should fail once at -O1 and not at -O2 and higher. Can you provide me with any insights; eg, by rerunning the testcase outside of the dejagnu framework? Thank you for doing this testing, by the way, even if the failure is a bit obscure at the moment. Best regards Paul
Re: [Linaro-TCWG-CI] gcc patch #93154: FAIL: 1 regressions on arm
Hi Paul, please, please, please let me know what the cause was. I have a similar error that my testcase fails, but for -O2, -O3 and -Os, which I have not figured yet. - Andre On Tue, 2 Jul 2024 11:10:37 +0100 Paul Richard Thomas wrote: > Hi Andre, > > I have to hold back on the commit until the business below is sorted out. > > Cheers > > Paul > > > -- Forwarded message - > From: Paul Richard Thomas > Date: Tue, 2 Jul 2024 at 08:48 > Subject: [Linaro-TCWG-CI] gcc patch #93154: FAIL: 1 regressions on arm > To: > > > Hi there, > > You detected a failure in gfortran.dg/class_transformational_2.f90: > PASS: gfortran.dg/class_transformational_2.f90 -O0 (test for excess > errors) > PASS: gfortran.dg/class_transformational_2.f90 -O0 execution test > PASS: gfortran.dg/class_transformational_2.f90 -O1 (test for excess > errors) > FAIL: gfortran.dg/class_transformational_2.f90 -O1 execution test > PASS: gfortran.dg/class_transformational_2.f90 -O2 (test for excess > errors) > PASS: gfortran.dg/class_transformational_2.f90 -O2 execution test > PASS: gfortran.dg/class_transformational_2.f90 -O3 -fomit-frame-pointer > ...snip... > PASS: gfortran.dg/class_transformational_2.f90 -O3 -fomit-frame-pointer > ...snip... > PASS: gfortran.dg/class_transformational_2.f90 -O3 -g (test for excess > errors) > PASS: gfortran.dg/class_transformational_2.f90 -O3 -g execution test > PASS: gfortran.dg/class_transformational_2.f90 -Os (test for excess > errors) > PASS: gfortran.dg/class_transformational_2.f90 -Os execution test > > The stop message in the full log indicates a numeric error in the first > test. I am unable to reproduce the error. Adding deallocation of all the > allocated variables (which I should have done in the first place) and > running valgrind with -s shows no errors and no memory loss. > > I find it odd that it should fail once at -O1 and not at -O2 and higher. > Can you provide me with any insights; eg, by rerunning the testcase outside > of the dejagnu framework? > > Thank you for doing this testing, by the way, even if the failure is a bit > obscure at the moment. > > Best regards > > Paul -- Andre Vehreschild * Email: vehre ad gmx dot de
Re: [Linaro-TCWG-CI] gcc patch #93154: FAIL: 1 regressions on arm
Andre, I am sorry but I don't have the slightest idea. I don't have an ARM system to test it on and so I am waiting for some indications from them. As soon as I know, you will too :-) As I indicated in my email to them, I think that the FAIL at -O1 is bogus. Cheers Paul On Tue, 2 Jul 2024 at 11:20, Andre Vehreschild wrote: > Hi Paul, > > please, please, please let me know what the cause was. I have a similar > error > that my testcase fails, but for -O2, -O3 and -Os, which I have not figured > yet. > > - Andre > > On Tue, 2 Jul 2024 11:10:37 +0100 > Paul Richard Thomas wrote: > > > Hi Andre, > > > > I have to hold back on the commit until the business below is sorted out. > > > > Cheers > > > > Paul > > > > > > -- Forwarded message - > > From: Paul Richard Thomas > > Date: Tue, 2 Jul 2024 at 08:48 > > Subject: [Linaro-TCWG-CI] gcc patch #93154: FAIL: 1 regressions on arm > > To: > > > > > > Hi there, > > > > You detected a failure in gfortran.dg/class_transformational_2.f90: > > PASS: gfortran.dg/class_transformational_2.f90 -O0 (test for excess > > errors) > > PASS: gfortran.dg/class_transformational_2.f90 -O0 execution test > > PASS: gfortran.dg/class_transformational_2.f90 -O1 (test for excess > > errors) > > FAIL: gfortran.dg/class_transformational_2.f90 -O1 execution test > > PASS: gfortran.dg/class_transformational_2.f90 -O2 (test for excess > > errors) > > PASS: gfortran.dg/class_transformational_2.f90 -O2 execution test > > PASS: gfortran.dg/class_transformational_2.f90 -O3 -fomit-frame-pointer > > ...snip... > > PASS: gfortran.dg/class_transformational_2.f90 -O3 -fomit-frame-pointer > > ...snip... > > PASS: gfortran.dg/class_transformational_2.f90 -O3 -g (test for excess > > errors) > > PASS: gfortran.dg/class_transformational_2.f90 -O3 -g execution test > > PASS: gfortran.dg/class_transformational_2.f90 -Os (test for excess > > errors) > > PASS: gfortran.dg/class_transformational_2.f90 -Os execution test > > > > The stop message in the full log indicates a numeric error in the first > > test. I am unable to reproduce the error. Adding deallocation of all the > > allocated variables (which I should have done in the first place) and > > running valgrind with -s shows no errors and no memory loss. > > > > I find it odd that it should fail once at -O1 and not at -O2 and higher. > > Can you provide me with any insights; eg, by rerunning the testcase > outside > > of the dejagnu framework? > > > > Thank you for doing this testing, by the way, even if the failure is a > bit > > obscure at the moment. > > > > Best regards > > > > Paul > > > -- > Andre Vehreschild * Email: vehre ad gmx dot de >
[PATCH] Fortran: fix associate with assumed-length character array [PR115700]
Dear all,
the attached patch addresses an effectively bogus warning about
uninitialized temporary string lengths of associate selectors.
The primary reason is that the array descriptor for a character
array is created before the corresponding string length is set.
Moving the setting of the string length temporary to the beginning
of the block solves the issue.
The patch does not solve the case for the target containing
substring references. This needs to be addressed separately.
(So far I could not find a solution that does not regress.)
Regtested on x86_64-pc-linux-gnu. OK for mainline?
As the PR is marked as a regression, is it also OK for backporting?
Thanks,
Harald
From 930a1be8c623cf03f9b2e6dbddb45d0b69e152dd Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Tue, 2 Jul 2024 21:26:05 +0200
Subject: [PATCH] Fortran: fix associate with assumed-length character array
[PR115700]
gcc/fortran/ChangeLog:
PR fortran/115700
* trans-stmt.cc (trans_associate_var): When the associate target
is an array-valued character variable, the length is known at entry
of the associate block. Move setting of string length of the
selector to the initialization part of the block.
gcc/testsuite/ChangeLog:
PR fortran/115700
* gfortran.dg/associate_69.f90: New test.
---
gcc/fortran/trans-stmt.cc | 18 +---
gcc/testsuite/gfortran.dg/associate_69.f90 | 33 ++
2 files changed, 47 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/associate_69.f90
diff --git a/gcc/fortran/trans-stmt.cc b/gcc/fortran/trans-stmt.cc
index 60275e18867..703a705e7ca 100644
--- a/gcc/fortran/trans-stmt.cc
+++ b/gcc/fortran/trans-stmt.cc
@@ -1911,6 +1911,8 @@ trans_associate_var (gfc_symbol *sym, gfc_wrapped_block *block)
gfc_se se;
tree desc;
bool cst_array_ctor;
+ stmtblock_t init;
+ gfc_init_block (&init);
desc = sym->backend_decl;
cst_array_ctor = e->expr_type == EXPR_ARRAY
@@ -1935,10 +1937,17 @@ trans_associate_var (gfc_symbol *sym, gfc_wrapped_block *block)
&& !sym->attr.select_type_temporary
&& sym->ts.u.cl->backend_decl
&& VAR_P (sym->ts.u.cl->backend_decl)
+ && se.string_length
&& se.string_length != 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));
+ {
+ /* When the target is a variable, its length is already known. */
+ tree len = fold_convert (TREE_TYPE (sym->ts.u.cl->backend_decl),
+ se.string_length);
+ if (e->expr_type == EXPR_VARIABLE)
+ gfc_add_modify (&init, sym->ts.u.cl->backend_decl, len);
+ else
+ gfc_add_modify (&se.pre, sym->ts.u.cl->backend_decl, len);
+ }
/* If we didn't already do the pointer assignment, set associate-name
descriptor to the one generated for the temporary. */
@@ -1978,7 +1987,8 @@ trans_associate_var (gfc_symbol *sym, gfc_wrapped_block *block)
}
/* Done, register stuff as init / cleanup code. */
- gfc_add_init_cleanup (block, gfc_finish_block (&se.pre),
+ gfc_add_block_to_block (&init, &se.pre);
+ gfc_add_init_cleanup (block, gfc_finish_block (&init),
gfc_finish_block (&se.post));
}
diff --git a/gcc/testsuite/gfortran.dg/associate_69.f90 b/gcc/testsuite/gfortran.dg/associate_69.f90
new file mode 100644
index 000..28f488bb274
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/associate_69.f90
@@ -0,0 +1,33 @@
+! { dg-do compile }
+! { dg-options "-Og -Wuninitialized -Wmaybe-uninitialized -fdump-tree-optimized" }
+!
+! PR fortran/115700 - Bogus warning for associate with assumed-length character array
+!
+subroutine mvce(x)
+ implicit none
+ character(len=*), dimension(:), intent(in) :: x
+
+ associate (tmp1 => x)
+if (len (tmp1) /= len (x)) stop 1
+ end associate
+
+ associate (tmp2 => x(1:))
+if (len (tmp2) /= len (x)) stop 2
+ end associate
+
+ associate (tmp3 => x(1:)(:))
+if (len (tmp3) /= len (x)) stop 3
+ end associate
+
+! The following associate blocks still produce bogus warnings:
+
+! associate (tmp4 => x(:)(1:))
+! if (len (tmp4) /= len (x)) stop 4
+! end associate
+!
+! associate (tmp5 => x(1:)(1:))
+! if (len (tmp5) /= len (x)) stop 5
+! end associate
+end
+
+! { dg-final { scan-tree-dump-not " \\.tmp" "optimized" } }
--
2.35.3
