Bug 54917 - [4.7/4.8 Regression] [OOP] TRANSFER on polymorphic variable causes ICE
Summary: [4.7/4.8 Regression] [OOP] TRANSFER on polymorphic variable causes ICE
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.7.3
: P3 normal
Target Milestone: 4.7.3
Assignee: janus
URL:
Keywords: ice-on-valid-code
Depends on:
Blocks: 31237
  Show dependency treegraph
 
Reported: 2012-10-12 19:21 UTC by Sean Santos
Modified: 2016-11-16 15:15 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2012-10-12 00:00:00


Attachments
Short program that causes the bug (221 bytes, text/plain)
2012-10-12 19:21 UTC, Sean Santos
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sean Santos 2012-10-12 19:21:48 UTC
Created attachment 28436 [details]
Short program that causes the bug

When compiling a routine that uses transfer on a polymorphic variable, the following command fails:

gfortran -c -Wsurprising test_module.F90

The error message is:



test_module.F90:18.22:

end module test_module
                      1
Internal Error at (1):
Invalid expression in gfc_target_expr_size.



This error does not occur if -Wsurprising is off and the polymorphic variable is the "source". It does occur if the polymorphic variable is the "mold", even with no warnings on.


Obviously, you would rarely-or-never want to actually do such a transfer, but I did notice this problem because I have been using the following macro to turn off warnings about unused arguments:

#define UNUSED_VAR(arg) if (.false.) write(*,*) transfer(arg,1)

A bit hack-ish, but gets the job done in impure routines. In any case, even if gfortran rejects this, getting a specific error message rather than an internal error would be nice.


I'm in a somewhat limited environment right now, so I can't tell for sure whether this is an old problem or a regression. This has happened with one of the 4.7.3 unofficial prerelease builds:

gcc-4.7 -v
Using built-in specs.
COLLECT_GCC=gcc-4.7
COLLECT_LTO_WRAPPER=/home/santos/gcc-4.7/bin/../libexec/gcc/x86_64-unknown-linux-gnu/4.7.3/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: ../gcc-4.7-source/gcc-4.7-20120929/configure --enable-languages=c,c++,fortran --enable-checking=release --disable-bootstrap --disable-libmudflap --enable-libgomp --enable-lto --enable-gold --with-plugin-ld=/usr/bin/gold --prefix=/usr/local/gcc-4.7
Thread model: posix
Comment 1 janus 2012-10-12 21:31:15 UTC
Thanks for reporting this. The following should already be enough to fix the ICE on -Wsurprising:

Index: gcc/fortran/target-memory.c
===================================================================
--- gcc/fortran/target-memory.c (revision 192392)
+++ gcc/fortran/target-memory.c (working copy)
@@ -121,6 +121,7 @@ gfc_target_expr_size (gfc_expr *e)
     case BT_HOLLERITH:
       return e->representation.length;
     case BT_DERIVED:
+    case BT_CLASS:
       {
        /* Determine type size without clobbering the typespec for ISO C
           binding types.  */


But certainly some more modifications will be needed. I think our implementation of TRANSFER is not really fit for handling polymorphic arguments yet.
Comment 2 Dominique d'Humieres 2012-10-12 21:56:30 UTC
Revision 176696 (2011-07-23) is OK; revision 177649 (2011-08-11) is not.
Comment 3 Dominique d'Humieres 2012-10-12 22:06:44 UTC
It could be r177486 or 177486?
Comment 4 janus 2012-10-13 10:01:00 UTC
(In reply to comment #3)
> It could be r177486 or 177486?

Not sure. (Note: Both revisions you quote are the same.)


Anyway, I can confirm that at least the ICE with -Wsurprising is a regression in 4.7 and trunk, which does not appear with 4.5 and 4.6:

! The following causes an ICE if -Wsurprising is on.
subroutine test_routine1(arg)
  implicit none
  type test_type
    integer :: test_comp
  end type
  class(test_type) :: arg
  integer :: i
  i = transfer(arg, 1)
end subroutine
Comment 5 janus 2012-10-13 10:05:13 UTC
However, the other ICE (which does not require -Wsurprising) appears with all gfortran versions I tried from 4.5 to trunk:


subroutine test_routine2(arg)
  implicit none
  type test_type
    integer :: test_comp
  end type test_type
  class(test_type) :: arg
  class(test_type), allocatable :: copy
  copy = transfer(1,arg)
end subroutine
Comment 6 Dominique d'Humieres 2012-10-13 10:12:28 UTC
> > It could be r177486 or 177486?
>
> Not sure. (Note: Both revisions you quote are the same.)

Oops! (never copy and paste after midnight.) r177486 or r177527 (see pr50004). 

Note the "could"; I am not sure either, but it looks the most likely in the set of patches for the time period.
If my guess is right, the patchlet in comment #1 makes sense.
Comment 7 janus 2012-10-13 13:08:11 UTC
(In reply to comment #6)
> r177486 or r177527 (see pr50004). 

Out of these two, I'd rather guess for the latter (but this is not more than a guess).

Anyway, I think whatever revision caused the regression probably just 'uncovered' the fact that TRANSFER does not handle polymorphics.
Comment 8 janus 2012-10-13 21:52:39 UTC
The following fixes the ICE(s) on comment 5 ...


Index: gcc/fortran/target-memory.c
===================================================================
--- gcc/fortran/target-memory.c	(revision 192392)
+++ gcc/fortran/target-memory.c	(working copy)
@@ -121,6 +121,7 @@ gfc_target_expr_size (gfc_expr *e)
     case BT_HOLLERITH:
       return e->representation.length;
     case BT_DERIVED:
+    case BT_CLASS:
       {
 	/* Determine type size without clobbering the typespec for ISO C
 	   binding types.  */
@@ -572,6 +573,9 @@ gfc_target_interpret_expr (unsigned char *buffer,
         gfc_interpret_character (buffer, buffer_size, result);
       break;
 
+    case BT_CLASS:
+      result->ts = result->ts.u.derived->components->ts;
+      /* Fall through.  */
     case BT_DERIVED:
       result->representation.length = 
         gfc_interpret_derived (buffer, buffer_size, result);


... which is then rejected with:

  copy = transfer(1,arg)
  1
Error: Variable must not be polymorphic in intrinsic assignment at (1) - check that there is a matching specific subroutine for '=' operator


Intrinsic assignment to polymorphic variables is an F08 feature, which is not yet implemented in gfortran, cf. PR 43366.
Comment 9 janus 2012-10-14 11:15:25 UTC
Ok, after the ICEs are fixed, let's come to runtime behavior. Here is a test case:

  implicit none
  type test_type
    integer :: i
  end type
  class(test_type), allocatable :: c
  type(test_type) :: t

  allocate(c)
  c%i=3
  t = transfer(c, t)
  print *,t

end


With ifort and sunf95 this gives the expected output of "3", while gfortran just spits out random numbers. -fdump-tree-original shows the following:

{
  struct test_type transfer.0;
  integer(kind=8) D.1884;
  integer(kind=8) D.1883;
  integer(kind=8) D.1882;

  D.1882 = 16;
  D.1883 = 4;
  __builtin_memcpy ((void *) &transfer.0, (void *) &c, MAX_EXPR <MIN_EXPR <D.1883, D.1882>, 0>);
  t = transfer.0;
}

Clearly, the "&c" here should be "&c._data".
Comment 10 janus 2012-10-14 12:01:39 UTC
(In reply to comment #9)
> -fdump-tree-original shows the following:
> 
> [...]
>   __builtin_memcpy ((void *) &transfer.0, (void *) &c, MAX_EXPR <MIN_EXPR
> <D.1883, D.1882>, 0>);
> [...]
> 
> Clearly, the "&c" here should be "&c._data".

Moreover, the size of the memcpy should be determined by "c._vptr->_size". Both is done by the following patch:


Index: gcc/fortran/trans-intrinsic.c
===================================================================
--- gcc/fortran/trans-intrinsic.c	(revision 192392)
+++ gcc/fortran/trans-intrinsic.c	(working copy)
@@ -5376,18 +5376,28 @@ gfc_conv_intrinsic_transfer (gfc_se * se, gfc_expr
   if (arg->expr->rank == 0)
     {
       gfc_conv_expr_reference (&argse, arg->expr);
-      source = argse.expr;
+      if (arg->expr->ts.type == BT_CLASS)
+	source = gfc_class_data_get (argse.expr);
+      else
+	source = argse.expr;
 
-      source_type = TREE_TYPE (build_fold_indirect_ref_loc (input_location,
-							argse.expr));
-
       /* Obtain the source word length.  */
-      if (arg->expr->ts.type == BT_CHARACTER)
-	tmp = size_of_string_in_bytes (arg->expr->ts.kind,
-				       argse.string_length);
-      else
-	tmp = fold_convert (gfc_array_index_type,
-			    size_in_bytes (source_type)); 
+      switch (arg->expr->ts.type)
+	{
+	case BT_CHARACTER:
+	  tmp = size_of_string_in_bytes (arg->expr->ts.kind,
+					 argse.string_length);
+	  break;
+	case BT_CLASS:
+	  tmp = gfc_vtable_size_get (argse.expr);
+	  break;
+	default:
+	  source_type = TREE_TYPE (build_fold_indirect_ref_loc (input_location,
+								source));
+	  tmp = fold_convert (gfc_array_index_type,
+			      size_in_bytes (source_type));
+	  break;
+	}
     }
   else
     {
Comment 11 janus 2012-10-14 15:55:00 UTC
Note: The combined patches of comment 8 and 10 regtest cleanly.
Comment 12 janus 2012-10-14 15:55:51 UTC
Btw, this PR is closely related to PR 46783.
Comment 13 janus 2012-10-14 16:31:31 UTC
Transferring CLASS to TYPE has been discussed in comment 9 and 10. Now: TYPE to CLASS ...

Here the test case gets a bit larger, since gfortran does not yet support intrinsic assignment to polymorphic variables (PR 43366), so we need a defined assignment procedure:

module m
  implicit none
  type test_type
    integer :: i
  contains
    procedure :: ass
    generic :: assignment(=) => ass
  end type
contains
  subroutine ass (a, b)
    class(test_type), intent(out) :: a
    class(test_type), intent(in)  :: b
    a%i = b%i
  end subroutine
end module

program p
  use m
  implicit none

  class(test_type), allocatable :: c
  type(test_type) :: t

  t%i=3
  allocate(c)
  c = transfer(t, c)
  print *,c%i
end


With ifort, the expected output ("3") is produced. sunf95 refuses to compile it (doesn't like type-bound procedures). gfortran compiles it, but produces a segfault at runtime. The dump looks like this:

{
  struct __class_m_Test_type_a D.1895;
  struct __class_m_Test_type_a transfer.0;
  integer(kind=8) D.1893;
  integer(kind=8) D.1892;
  integer(kind=8) D.1891;

  D.1891 = 4;
  D.1892 = 16;
  __builtin_memcpy ((void *) &transfer.0, (void *) &t, MAX_EXPR <MIN_EXPR <D.1892, D.1891>, 0>);
  D.1895 = transfer.0;
  ass (&c, &D.1895);
}

So, again, _data and _size references missing.
Comment 14 janus 2012-10-14 22:56:11 UTC
(In reply to comment #13)
> So, again, _data and _size references missing.

Moreover, we need to set the _vptr component and allocate _data.
Comment 15 janus 2012-11-06 10:15:46 UTC
Author: janus
Date: Tue Nov  6 10:15:42 2012
New Revision: 193226

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193226
Log:
2012-11-06  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/54917
	* target-memory.c (gfc_target_expr_size,gfc_target_interpret_expr):
	Handle BT_CLASS.
	* trans-intrinsic.c (gfc_conv_intrinsic_transfer): Add support for
	polymorphic arguments.

2012-11-06  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/54917
	* gfortran.dg/transfer_class_1.f90: New.
	* gfortran.dg/transfer_class_2.f90: New.

Added:
    trunk/gcc/testsuite/gfortran.dg/transfer_class_1.f90
    trunk/gcc/testsuite/gfortran.dg/transfer_class_2.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/target-memory.c
    trunk/gcc/fortran/trans-intrinsic.c
    trunk/gcc/testsuite/ChangeLog
Comment 16 janus 2012-11-06 22:44:59 UTC
Author: janus
Date: Tue Nov  6 22:44:47 2012
New Revision: 193262

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193262
Log:
2012-11-06  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/54917
	* target-memory.c (gfc_target_expr_size,gfc_target_interpret_expr):
	Handle BT_CLASS.

2012-11-06  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/54917
	* gfortran.dg/transfer_class_1.f90: New.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gfortran.dg/transfer_class_1.f90
Modified:
    branches/gcc-4_7-branch/gcc/fortran/ChangeLog
    branches/gcc-4_7-branch/gcc/fortran/target-memory.c
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog
Comment 17 janus 2012-11-06 22:52:35 UTC
With r193226 and r193262, the ICEs and runtime problems have been fixed on trunk, and the regression ICE has also been backported to 4.7.

Closing as fixed. Thanks for the bugreport!