[PATCH] fortran: Unshare associate var charlen [PR104228]

[PATCH] fortran: Unshare associate var charlen [PR104228]

[Mikael Morin]

[-- Attachment #1: Type: text/plain, Size: 245 bytes --]

Hello,

the attached patch is a fix for PR104228.
Even if simple, I wouldn’t call it obvious, as it’s involving character 
length and associate, so I don’t mind some extra review eyes.

Tested on x86_64-pc-linux-gnu. Ok for master/11/10/9?

[-- Attachment #2: 0001-fortran-Unshare-associate-var-charlen-PR104228.patch --]
[-- Type: text/x-patch, Size: 4081 bytes --]

From 0819226560387b2953622ee3d5d051a35606d504 Mon Sep 17 00:00:00 2001
From: Mikael Morin <mikael@gcc.gnu.org>
Date: Fri, 28 Jan 2022 22:00:57 +0100
Subject: [PATCH] fortran: Unshare associate var charlen [PR104228]

PR104228 showed that character lengths were shared between associate
variable and associate targets.  This is problematic when the associate
target is itself a variable and gets a variable to hold the length, as
the length variable is added (and all the variables following it in the chain)
to both the associate variable scope and the target variable scope.
This caused an ICE when compiling with -O0 -fsanitize=address.

This change forces the creation of a separate character length for the
associate variable.  It also forces the initialization of the character
length variable to avoid regressing associate_32 and associate_47 tests.

gcc/fortran/ChangeLog:

	* resolve.cc (resolve_assoc_var): Also create a new character
	length for non-dummy associate targets.
	* trans-stmt.cc (trans_associate_var): Initialize character length
	even if no temporary is used for the associate variable.

gcc/testsuite/ChangeLog:

	* gfortran.dg/asan/associate_1.f90: New test.
	* gfortran.dg/asan/associate_2.f90: New test.
---
 gcc/fortran/resolve.cc                        |  1 -
 gcc/fortran/trans-stmt.cc                     |  2 +-
 .../gfortran.dg/asan/associate_1.f90          | 19 +++++++++++++++++++
 .../gfortran.dg/asan/associate_2.f90          | 19 +++++++++++++++++++
 4 files changed, 39 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/asan/associate_1.f90
 create mode 100644 gcc/testsuite/gfortran.dg/asan/associate_2.f90

diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc
index 835a4783718..266e41e25b1 100644
--- a/gcc/fortran/resolve.cc
+++ b/gcc/fortran/resolve.cc
@@ -9227,7 +9227,6 @@ resolve_assoc_var (gfc_symbol* sym, bool resolve_target)
 	sym->ts.u.cl = target->ts.u.cl;
 
       if (sym->ts.deferred && target->expr_type == EXPR_VARIABLE
-	  && target->symtree->n.sym->attr.dummy
 	  && sym->ts.u.cl == target->ts.u.cl)
 	{
 	  sym->ts.u.cl = gfc_new_charlen (sym->ns, NULL);
diff --git a/gcc/fortran/trans-stmt.cc b/gcc/fortran/trans-stmt.cc
index 04f8147d23b..30b6bd5dd2a 100644
--- a/gcc/fortran/trans-stmt.cc
+++ b/gcc/fortran/trans-stmt.cc
@@ -1918,7 +1918,7 @@ trans_associate_var (gfc_symbol *sym, gfc_wrapped_block *block)
       gfc_conv_expr_descriptor (&se, e);
 
       if (sym->ts.type == BT_CHARACTER
-	  && !se.direct_byref && sym->ts.deferred
+	  && sym->ts.deferred
 	  && !sym->attr.select_type_temporary
 	  && VAR_P (sym->ts.u.cl->backend_decl)
 	  && se.string_length != sym->ts.u.cl->backend_decl)
diff --git a/gcc/testsuite/gfortran.dg/asan/associate_1.f90 b/gcc/testsuite/gfortran.dg/asan/associate_1.f90
new file mode 100644
index 00000000000..b5ea75498b7
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/asan/associate_1.f90
@@ -0,0 +1,19 @@
+! { dg-do compile }
+! { dg-additional-options "-O0" }
+!
+! PR fortran/104228
+! The code generated code for the program below wrongly pushed the Y character
+! length variable to both P and S scope, which was leading to an ICE when
+! address sanitizer was in effect
+
+program p
+   character(:), save, allocatable :: x(:)
+   call s
+contains
+   subroutine s
+      associate (y => x)
+         y = [x]
+      end associate
+   end
+end
+
diff --git a/gcc/testsuite/gfortran.dg/asan/associate_2.f90 b/gcc/testsuite/gfortran.dg/asan/associate_2.f90
new file mode 100644
index 00000000000..9bfb2bfbafb
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/asan/associate_2.f90
@@ -0,0 +1,19 @@
+! { dg-do compile }
+! { dg-additional-options "-O0" }
+!
+! PR fortran/104228
+! The code generated code for the program below wrongly pushed the Y character
+! length variable to both P and S scope, which was leading to an ICE when
+! address sanitizer was in effect
+
+program p
+   character(:), allocatable :: x(:)
+   call s
+contains
+   subroutine s
+      associate (y => x)
+         y = [x]
+      end associate
+   end
+end
+
-- 
2.34.1

Re: [PATCH] fortran: Unshare associate var charlen [PR104228]

[Harald Anlauf]

Hi Mikael,

Am 29.01.22 um 15:24 schrieb Mikael Morin:
> Hello,
>
> the attached patch is a fix for PR104228.
> Even if simple, I wouldn’t call it obvious, as it’s involving character
> length and associate, so I don’t mind some extra review eyes.

I am probably not experienced enough to review this.  Paul?

Nevertheless, I looked at the commits that introduced the code
you touched.  It appears that these did not cover, maybe even
avoid the current case where the associate target is not a dummy.
Your changes seem to make sense and fix the issue.

> Tested on x86_64-pc-linux-gnu. Ok for master/11/10/9?

OK, and thanks for the patch!

Harald

Re: [PATCH] fortran: Unshare associate var charlen [PR104228]

[Paul Richard Thomas]

Hi Harald and Mikael,

This looks fine to me. OK for all the listed branches.

Thanks for the patch

Paul


On Wed, 2 Feb 2022 at 20:20, Harald Anlauf via Fortran <fortran@gcc.gnu.org>
wrote:

> Hi Mikael,
>
> Am 29.01.22 um 15:24 schrieb Mikael Morin:
> > Hello,
> >
> > the attached patch is a fix for PR104228.
> > Even if simple, I wouldn’t call it obvious, as it’s involving character
> > length and associate, so I don’t mind some extra review eyes.
>
> I am probably not experienced enough to review this.  Paul?
>
> Nevertheless, I looked at the commits that introduced the code
> you touched.  It appears that these did not cover, maybe even
> avoid the current case where the associate target is not a dummy.
> Your changes seem to make sense and fix the issue.
>
> > Tested on x86_64-pc-linux-gnu. Ok for master/11/10/9?
>
> OK, and thanks for the patch!
>
> Harald
>


-- 
"If you can't explain it simply, you don't understand it well enough" -
Albert Einstein