[PATCH] PR fortran/99112 - [11 Regression] ICE with runtime diagnostics for SIZE intrinsic function

[PATCH] PR fortran/99112 - [11 Regression] ICE with runtime diagnostics for SIZE intrinsic function

[Harald Anlauf]

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

Dear all,

the addition of runtime checks for the SIZE intrinsic created a regression
that showed up for certain CLASS arguments to procedures.  Paul did most of
the work (~ 99%), but asked me to dig into an issue with an inappropriately
selected error message.  This actually turned out to be a simple one-liner
on top of Paul's patch.

Regtested on x86_64-pc-linux-gnu.  OK for mainline?

Thanks,
Harald

P.S.: I couldn't find a Changelog entry that uses co-authors.  Is the version
below correct?


PR fortran/99112 - ICE with runtime diagnostics for SIZE intrinsic function

Add/fix handling of runtime checks for CLASS arguments with ALLOCATABLE
or POINTER attribute.

gcc/fortran/ChangeLog:

	* trans-expr.c (gfc_conv_procedure_call): Fix runtime checks for
	CLASS arguments.
	* trans-intrinsic.c (gfc_conv_intrinsic_size): Likewise.

gcc/testsuite/ChangeLog:

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

Co-authored-by: Paul Thomas  <pault@gcc.gnu.org>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pr99112.patch --]
[-- Type: text/x-patch, Size: 4130 bytes --]

diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
index 85c16d7f4c3..53c47e18dfd 100644
--- a/gcc/fortran/trans-expr.c
+++ b/gcc/fortran/trans-expr.c
@@ -6662,6 +6662,7 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
 	  symbol_attribute attr;
 	  char *msg;
 	  tree cond;
+	  tree temp;

 	  if (e->expr_type == EXPR_VARIABLE || e->expr_type == EXPR_FUNCTION)
 	    attr = gfc_expr_attr (e);
@@ -6732,16 +6733,25 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
 	      else
 		goto end_pointer_check;

-	      tmp = parmse.expr;
+	      if (fsym && fsym->ts.type == BT_CLASS)
+		{
+		  temp = build_fold_indirect_ref_loc (input_location,
+						      parmse.expr);
+		  temp = gfc_class_data_get (temp);
+		  if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (temp)))
+		    temp = gfc_conv_descriptor_data_get (temp);
+		}
+	      else
+		temp = parmse.expr;

 	      /* If the argument is passed by value, we need to strip the
 		 INDIRECT_REF.  */
-	      if (!POINTER_TYPE_P (TREE_TYPE (parmse.expr)))
-		tmp = gfc_build_addr_expr (NULL_TREE, tmp);
+	      if (!POINTER_TYPE_P (TREE_TYPE (temp)))
+		temp = gfc_build_addr_expr (NULL_TREE, temp);

 	      cond = fold_build2_loc (input_location, EQ_EXPR,
-				      logical_type_node, tmp,
-				      fold_convert (TREE_TYPE (tmp),
+				      logical_type_node, temp,
+				      fold_convert (TREE_TYPE (temp),
 						    null_pointer_node));
 	    }

diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c
index 9cf3642f694..5e53d1162fa 100644
--- a/gcc/fortran/trans-intrinsic.c
+++ b/gcc/fortran/trans-intrinsic.c
@@ -8006,8 +8006,10 @@ gfc_conv_intrinsic_size (gfc_se * se, gfc_expr * expr)
     {
       symbol_attribute attr;
       char *msg;
+      tree temp;
+      tree cond;

-      attr = gfc_expr_attr (e);
+      attr = sym ? sym->attr : gfc_expr_attr (e);
       if (attr.allocatable)
 	msg = xasprintf ("Allocatable argument '%s' is not allocated",
 			 e->symtree->n.sym->name);
@@ -8017,14 +8019,24 @@ gfc_conv_intrinsic_size (gfc_se * se, gfc_expr * expr)
       else
 	goto end_arg_check;

-      argse.descriptor_only = 1;
-      gfc_conv_expr_descriptor (&argse, actual->expr);
-      tree temp = gfc_conv_descriptor_data_get (argse.expr);
-      tree cond = fold_build2_loc (input_location, EQ_EXPR,
-				   logical_type_node, temp,
-				   fold_convert (TREE_TYPE (temp),
-						 null_pointer_node));
+      if (sym)
+	{
+	  temp = gfc_class_data_get (sym->backend_decl);
+	  temp = gfc_conv_descriptor_data_get (temp);
+	}
+      else
+	{
+	  argse.descriptor_only = 1;
+	  gfc_conv_expr_descriptor (&argse, actual->expr);
+	  temp = gfc_conv_descriptor_data_get (argse.expr);
+	}
+
+      cond = fold_build2_loc (input_location, EQ_EXPR,
+			      logical_type_node, temp,
+			      fold_convert (TREE_TYPE (temp),
+					    null_pointer_node));
       gfc_trans_runtime_check (true, false, cond, &argse.pre, &e->where, msg);
+
       free (msg);
     }
  end_arg_check:
diff --git a/gcc/testsuite/gfortran.dg/pr99112.f90 b/gcc/testsuite/gfortran.dg/pr99112.f90
new file mode 100644
index 00000000000..94010615b83
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr99112.f90
@@ -0,0 +1,27 @@
+! { dg-do compile }
+! { dg-options "-fcheck=pointer -fdump-tree-original" }
+! PR99112 - ICE with runtime diagnostics for SIZE intrinsic function
+
+module m
+  type t
+  end type
+contains
+  function f (x, y) result(z)
+    class(t) :: x(:)
+    class(t) :: y(size(x))
+    type(t)  :: z(size(x))
+  end
+  function g (x) result(z)
+    class(*) :: x(:)
+    type(t)  :: z(size(x))
+  end
+  subroutine s ()
+    class(t), allocatable :: a(:), b(:), c(:), d(:)
+    class(t), pointer     :: p(:)
+    c = f (a, b)
+    d = g (p)
+  end
+end
+! { dg-final { scan-tree-dump-times "_gfortran_runtime_error_at" 3 "original" } }
+! { dg-final { scan-tree-dump-times "Allocatable actual argument" 2 "original" } }
+! { dg-final { scan-tree-dump-times "Pointer actual argument" 1 "original" } }

Re: [PATCH] PR fortran/99112 - [11 Regression] ICE with runtime diagnostics for SIZE intrinsic function

[Paul Richard Thomas]

Hi Harald,

I am not sure of the etiquette for this - it looks OK to me :-)

Cheers

Paul


On Fri, 12 Mar 2021 at 21:20, Harald Anlauf via Fortran <fortran@gcc.gnu.org>
wrote:

> Dear all,
>
> the addition of runtime checks for the SIZE intrinsic created a regression
> that showed up for certain CLASS arguments to procedures.  Paul did most of
> the work (~ 99%), but asked me to dig into an issue with an inappropriately
> selected error message.  This actually turned out to be a simple one-liner
> on top of Paul's patch.
>
> Regtested on x86_64-pc-linux-gnu.  OK for mainline?
>
> Thanks,
> Harald
>
> P.S.: I couldn't find a Changelog entry that uses co-authors.  Is the
> version
> below correct?
>
>
> PR fortran/99112 - ICE with runtime diagnostics for SIZE intrinsic function
>
> Add/fix handling of runtime checks for CLASS arguments with ALLOCATABLE
> or POINTER attribute.
>
> gcc/fortran/ChangeLog:
>
>         * trans-expr.c (gfc_conv_procedure_call): Fix runtime checks for
>         CLASS arguments.
>         * trans-intrinsic.c (gfc_conv_intrinsic_size): Likewise.
>
> gcc/testsuite/ChangeLog:
>
>         * gfortran.dg/pr99112.f90: New test.
>
> Co-authored-by: Paul Thomas  <pault@gcc.gnu.org>
>
>

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

Re: [PATCH] PR fortran/99112 - [11 Regression] ICE with runtime diagnostics for SIZE intrinsic function

[Tobias Burnus]

Hi Harald, hi Paul,

On 13.03.21 09:58, Paul Richard Thomas via Fortran wrote:
> I am not sure of the etiquette for this - it looks OK to me :-)

:-)

On Fri, 12 Mar 2021 at 21:20, Harald Anlauf via Fortran 
<fortran@gcc.gnu.org>

> the addition of runtime checks for the SIZE intrinsic created a regression
> that showed up for certain CLASS arguments to procedures.  Paul did most of
> the work (~ 99%), but asked me to dig into an issue with an inappropriately
> selected error message.  This actually turned out to be a simple one-liner
> on top of Paul's patch.
>
> Regtested on x86_64-pc-linux-gnu.  OK for mainline?

The procedure-call patch looks good to me, except for:
> --- a/gcc/fortran/trans-expr.c
> +++ b/gcc/fortran/trans-expr.c
> @@ -6662,6 +6662,7 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
> [...]
> +	  tree temp;
> ...
> -	      tmp = parmse.expr;
> +	      if (fsym && fsym->ts.type == BT_CLASS)
> +		{
> +		  temp = build_fold_indirect_ref_loc (input_location,
> ...
> +	      else
> +		temp = parmse.expr;
> ...
> -	      if (!POINTER_TYPE_P (TREE_TYPE (parmse.expr)))
> -		tmp = gfc_build_addr_expr (NULL_TREE, tmp);
> +	      if (!POINTER_TYPE_P (TREE_TYPE (temp)))
> +		temp = gfc_build_addr_expr (NULL_TREE, temp);
> ...
> -				      logical_type_node, tmp,
> -				      fold_convert (TREE_TYPE (tmp),
> +				      logical_type_node, temp,
> +				      fold_convert (TREE_TYPE (temp)

I do not see any reason why 'tmp' is replaced by 'temp' in this
code. Also for doing patch archeology, it helps if there are no
changes unless it makes sense. Adding an -e- does not count ;-)

Hence, OK with that change.

  * * *

Regarding the change:
  gfc_conv_intrinsic_size (gfc_se * se, gfc_expr * expr)

It looks as if the pointer/check is done for
   size(dt%foo%bar)
for the 'bar' component' but shouldn't it also be
done for each part ref, if it is a pointer/allocatable?
(i.e. 'dt', 'foo' and 'bar'?

That's independent of the current patch.

Additionally, as there are a lot of special cases for
CLASS – I wonder whether there also needs to be a special
case for
   size(dt%foo%class)
?

In particular, the following does ICE for me:

module m
   type t
     class(*), pointer :: bar(:)
   end type
   type t2
     class(t), allocatable :: my(:)
   end type t2
contains
   function f (x, y) result(z)
     class(t) :: x(:)
     class(t) :: y(size(x(1)%bar))
     type(t)  :: z(size(x(1)%bar))
   end
   function g (x) result(z)
     class(t) :: x(:)
     type(t)  :: z(size(x(1)%bar))
   end
   subroutine s ()
     class(t2), allocatable :: a(:), b(:), c(:), d(:)
     class(t2), pointer     :: p(:)
     c(1)%my = f (a(1)%my, b(1)%my)
     d(1)%my = g (p(1)%my)
   end
end

  * * *
  

> P.S.: I couldn't find a Changelog entry that uses co-authors.  Is the
> version
> below correct?
...
> Co-authored-by: Paul Thomas  <pault@gcc.gnu.org>

I think you have two options: either the GIT way – as you did (although I think the
GIT way usually only has one and not two spaces before the email).

I did not see it in the commit logs, but it is used in the
testcases for the change-log generator, see: contrib/gcc-changelog/test_patches.txt

Alternative way is to specify the authors in the classical
ChangeLog style; latest real-world example is
https://gcc.gnu.org/g:d656bfda2d8316627d0bbb18b10954e6aaf3c88c
but you can also look at the contrib/gcc-changelog/test_patches.txt

Finally, you can also run the script at contrib/gcc-changelog/ yourself
as pre-commit check.
For instance, if you committed but did not yet push it:
  git show -1 | ./contrib/gcc-changelog/git_check_commit.py -p
to errors there, you can use 'git commit --amend' until you have
pushed that commit (then it is too late).

Tobias

Aw: Re: [PATCH] PR fortran/99112 - [11 Regression] ICE with runtime diagnostics for SIZE intrinsic function

[Harald Anlauf]

Hi Tobias,

> I do not see any reason why 'tmp' is replaced by 'temp' in this
> code. Also for doing patch archeology, it helps if there are no
> changes unless it makes sense. Adding an -e- does not count ;-)
> 
> Hence, OK with that change.

I've corrected that.  This also reduces the size of the patch.

> Regarding the change:
>   gfc_conv_intrinsic_size (gfc_se * se, gfc_expr * expr)
> 
> It looks as if the pointer/check is done for
>    size(dt%foo%bar)
> for the 'bar' component' but shouldn't it also be
> done for each part ref, if it is a pointer/allocatable?
> (i.e. 'dt', 'foo' and 'bar'?
> 
> That's independent of the current patch.
> 
> Additionally, as there are a lot of special cases for
> CLASS – I wonder whether there also needs to be a special
> case for
>    size(dt%foo%class)
> ?
> 
> In particular, the following does ICE for me:

This ICEs even without any checks enabled and is now:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99585

> > P.S.: I couldn't find a Changelog entry that uses co-authors.  Is the
> > version
> > below correct?
> ...
> > Co-authored-by: Paul Thomas  <pault@gcc.gnu.org>
> 
> I think you have two options: either the GIT way – as you did (although I think the
> GIT way usually only has one and not two spaces before the email).
> 
> I did not see it in the commit logs, but it is used in the
> testcases for the change-log generator, see: contrib/gcc-changelog/test_patches.txt

I've done it as described above.  In the git log I see the co-author, while

>   git show -1 | ./contrib/gcc-changelog/git_check_commit.py -p

produces:

Checking c2d7c39fcb8a3cb67600cdb6fde49ecb0e951589: OK
------ gcc/fortran/ChangeLog ------ 
2021-03-14  Harald Anlauf  <anlauf@gmx.de>
            Paul Thomas  <pault@gcc.gnu.org>

etc.  Nice!

Thanks for the review.  But as your testcase shows, we're not finished yet...

Thanks,
Harald