Bug 47455 - [4.6 Regression][OOP] internal compiler error: in fold_convert_loc, at fold-const.c:2028
Summary: [4.6 Regression][OOP] internal compiler error: in fold_convert_loc, at fold-c...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.6.0
: P4 normal
Target Milestone: 4.6.0
Assignee: janus
URL:
Keywords: ice-on-valid-code, wrong-code
Depends on: 47474
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-25 10:47 UTC by Thomas Henlich
Modified: 2011-02-02 18:13 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-01-31 20:28:40


Attachments
Draft patch, working but failing due to another related bug (957 bytes, patch)
2011-01-26 10:36 UTC, Tobias Burnus
Details | Diff
Test case (hopefully correct) (591 bytes, text/x-fortran)
2011-01-26 18:47 UTC, Tobias Burnus
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Henlich 2011-01-25 10:47:40 UTC
The following example results in an internal compiler error.

Version: gcc version 4.6.0 20110122 (experimental) (GCC)
Platform: i686-pc-mingw32
Expected result: Produce proper source diagnostics/error message for the error.

module class_t
    type :: tx
        integer, dimension(:), allocatable :: i
    end type tx
    type :: t
        type(tx), pointer :: x
    contains
        procedure :: calc
        procedure :: find_x
    end type t
contains
    subroutine calc(this)
        class(t), target :: this
        this%x = this%find_x()
    end subroutine calc
    function find_x(this)
        class(t), intent(in) :: this
        type(tx), pointer :: find_x
        find_x => null()
    end function find_x
end module class_t

$ gfortran -c class_t.f90
class_t.f90: In function 'calc':
class_t.f90:14:0: internal compiler error: in fold_convert_loc, at fold-const.c:2028
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
Comment 1 Tobias Burnus 2011-01-25 11:19:44 UTC
(I have not checked the validity of the code, but ifort 11.1, gfortran 4.5 and NAG 5.1 accept the code.)
Comment 2 Tobias Burnus 2011-01-25 13:24:39 UTC
Regarding the regression:

  FAILS: 2010-05-03-r158988
  WORKS: 2010-04-29-r158905

  That includes a huge merge by Paul (r158910, 2010-04-29) - seemingly the
  OOP branch merge.

 * * *

The failure occurs for the assignment in gfc_trans_scalar_assign, which is called by gfc_trans_assignment_1 for:
expr1 is BT_DERIVED with "this"->"x"
expr2 is EXPR_FUNCTION with "this"->"_vptr"->"find_x"

The failure occurs for:

5281          gfc_add_modify (&block, lse->expr,
5282                          fold_convert (TREE_TYPE (lse->expr), rse->expr));

If one looks at "rhs->expr.common.type" and "lhs->expr.common.type" both are essentially the same - except that the RHS is a pointer:

RHS:
 <pointer_type 0x2aaaace325e8
    type <record_type 0x2aaaaac2fb28 tx BLK
LHS:
 <record_type 0x2aaaaac2fb28 tx BLK


In the working version from April 2010, the dump looks like:
    *this->$data->x = *find_x ((struct .class.t *) this);

The "*" on the RHS seems to be missing ...
Comment 3 Tobias Burnus 2011-01-25 20:32:53 UTC
RFC patch. Janus, what do you think?

(Compiles and works for the example; no further tests.)

--- a/gcc/fortran/trans-expr.c
+++ b/gcc/fortran/trans-expr.c
@@ -5929,6 +5929,9 @@ gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr * expr2, bool init_flag,
       gfc_add_expr_to_block (&loop.post, tmp);
     }
 
+  if (expr2->expr_type == EXPR_FUNCTION && gfc_expr_attr (expr2).proc_pointer)
+    rse.expr = build_fold_indirect_ref_loc (input_location, rse.expr);
+
   tmp = gfc_trans_scalar_assign (&lse, &rse, expr1->ts,
                                 l_is_temp || init_flag,
                                 expr_is_variable (expr2) || scalar_to_array,
Comment 4 Tobias Burnus 2011-01-26 10:36:41 UTC
Created attachment 23130 [details]
Draft patch, working but failing due to another related bug

(In reply to comment #3)
[First patch]

Works for the test case, but does not regtest. The following makes more sense: Replace "proc_pointer" by "pointer || allocatable"

 * * *

However, the test case - in the attachment - fails due to a different and in principle unrelated reason:

find_x2 (struct __class_class_t_T & restrict this)
{
  struct tx * res;

  res.i.data = 0B;
  res = 0B;

The "res.i.data = 0" is obviously wrong: The pointer "res" should be already be set to NULL - thus the component should in that case never be set.
Comment 5 Tobias Burnus 2011-01-26 18:47:00 UTC
Created attachment 23136 [details]
Test case (hopefully correct)

Updated test case, hopefully correct. Fails with gfortran due to this PR - and with ifort 11.1 and crayftn due to other compiler bugs; thus, I could not properly test it.

The result with the patch is kind of OK for the pointer version (find_x):
  this%x = this%find_x(that, .false.)
is translated into:
  *this->_data->x = *this->_vptr->find_x ((struct __class_class_t_T *) this,
                                          &that, &C.1605);

so far so good. But for a normal assignment, allocatable components require an allocate + deep copy with memcpy.


For the allocatable, there is an additional issue:
  this%y = this%find_y()
is translated into
  this->_data->y = *this->_vptr->find_y ((struct __class_class_t_T *) this);

At a glance that looks OK. However, it immediately segfaults. One thing which 
and fails immediately (segfault) - though, I do not see why it segfaults.
Comment 6 janus 2011-01-31 20:28:40 UTC
(In reply to comment #3)
> RFC patch. Janus, what do you think?

I think I'd prefer the following version:

Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c	(revision 169442)
+++ gcc/fortran/trans-expr.c	(working copy)
@@ -3606,10 +3606,9 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
         x = f()
      where f is pointer valued, we have to dereference the result.  */
   if (!se->want_pointer && !byref
-      && (sym->attr.pointer || sym->attr.allocatable)
-      && !gfc_is_proc_ptr_comp (expr, NULL))
-    se->expr = build_fold_indirect_ref_loc (input_location,
-					se->expr);
+      && ((!comp && (sym->attr.pointer || sym->attr.allocatable))
+	  || (comp && (comp->attr.pointer || comp->attr.allocatable))))
+    se->expr = build_fold_indirect_ref_loc (input_location, se->expr);
 
   /* f2c calling conventions require a scalar default real function to
      return a double precision result.  Convert this back to default



This fixes all of the following variant of the test case, which was (partially) miscompiled by both previous patches:


module class_t
  type :: tx
    integer :: i
  end type
  type :: t
    type(tx), pointer :: x
    procedure(find_x), pointer :: ppc
  contains
    procedure :: find_x
  end type
contains
  function find_x(this)
    class(t), intent(in) :: this
    type(tx), pointer :: find_x
    find_x => null()
  end function find_x
end module

program test
  use class_t
  class(t),allocatable :: this
  procedure(find_x), pointer :: pp
  allocate(this)
  pp => find_x
  this%ppc => find_x
  this%x = find_x(this)    ! (1) ordinary function
  this%x = pp(this)        ! (2) procedure pointer
  this%x = this%ppc()      ! (3) PPC
  this%x = this%find_x()   ! (4) TBP
end
Comment 7 Dominique d'Humieres 2011-01-31 21:56:08 UTC
The ICEs disappear with the patch in comment #6, but the test in comment #4 fails at

        if(any (this%x%i /= [5, 7])) call abort() ! FAILS

and segfault later if I comment the test. The test in comment #6 segfault also.
Comment 8 janus 2011-01-31 22:00:30 UTC
(In reply to comment #6)
> (In reply to comment #3)
> > RFC patch. Janus, what do you think?
> 
> I think I'd prefer the following version:
> 
> Index: gcc/fortran/trans-expr.c
> ===================================================================
> --- gcc/fortran/trans-expr.c    (revision 169442)
> +++ gcc/fortran/trans-expr.c    (working copy)
> @@ -3606,10 +3606,9 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
>          x = f()
>       where f is pointer valued, we have to dereference the result.  */
>    if (!se->want_pointer && !byref
> -      && (sym->attr.pointer || sym->attr.allocatable)
> -      && !gfc_is_proc_ptr_comp (expr, NULL))
> -    se->expr = build_fold_indirect_ref_loc (input_location,
> -                    se->expr);
> +      && ((!comp && (sym->attr.pointer || sym->attr.allocatable))
> +      || (comp && (comp->attr.pointer || comp->attr.allocatable))))
> +    se->expr = build_fold_indirect_ref_loc (input_location, se->expr);
> 
>    /* f2c calling conventions require a scalar default real function to
>       return a double precision result.  Convert this back to default
> 
> 
> 
> This fixes all of the following variant of the test case


It also regtests cleanly, and I'm pretty sure it's the proper thing to do. So I'll just go ahead and commit it ...

(The test case in comment #5 currently aborts with this patch, though I'm not sure if that is correct behavior.)
Comment 9 janus 2011-01-31 22:04:27 UTC
(In reply to comment #7)
> The test in comment #6 segfault also.

Yes, this is expected. It's not intended to be a run-time test. 'find_x' returns a NULL-pointer, which is being dereferenced in the assignment.
Comment 10 Tobias Burnus 2011-01-31 22:18:41 UTC
(In reply to comment #7)
> The test in comment #6 segfault also.

That's a test-case problem.
  this%x = find_x(this)
is invalid if the LHS is not associated - and a RHS null() is also problematic. Use:
  this%x => find_x(this)
instead (for all assignments in the file).


> The ICEs disappear with the patch in comment #6, but the test in comment #4
> fails at
>         if(any (this%x%i /= [5, 7])) call abort() ! FAILS

I think you mean comment 5. In comment 4, the line:
        this%y = this%find_y()
fails; the LHS is a nonpointer, nonallocatable DT. The RHS returns an (allocated) allocatable; the type is type(tx) w/ allocatable but unallocated components.
I think the test case is valid


Regarding comment 5: As written there, a deep copy is missing
Comment 11 janus 2011-01-31 22:52:03 UTC
Author: janus
Date: Mon Jan 31 22:51:59 2011
New Revision: 169455

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=169455
Log:
2011-01-31  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/47455
	* trans-expr.c (gfc_conv_procedure_call): Handle procedure pointers
	with pointer or allocatable result.


2011-01-31  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/47455
	* gfortran.dg/typebound_call_19.f03: New.

Added:
    trunk/gcc/testsuite/gfortran.dg/typebound_call_19.f03
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog
Comment 12 Tobias Burnus 2011-02-01 07:29:44 UTC
(In reply to comment #11)
>     trunk/gcc/testsuite/gfortran.dg/typebound_call_19.f03

"valgrind ./a.out" shows:
Invalid free() / delete / delete[]
  at 0x4C256FC: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
  by 0x58FA60A: free_mem (in /lib64/libc-2.11.3.so)
  by 0x58FA1B1: __libc_freeres (in /lib64/libc-2.11.3.so)
  by 0x4A205EC: _vgnU_freeres (in /usr/lib64/valgrind/vgpreload_core-amd64-linux.so)
  by 0x581952C: __run_exit_handlers (exit.c:93)
  by 0x58195D4: exit (exit.c:100)
  by 0x5802BC3: (below main) (libc-start.c:258)
Address 0x405f400 is not stack'd, malloc'd or (recently) free'd


Otherwise - except for the comment to the committed test case (which is now a valid "dg-do run") - the remarks of comment 10 still hold.
Comment 13 Tobias Burnus 2011-02-01 08:50:39 UTC
(In reply to comment #12)
> "valgrind ./a.out" shows:

That seems to be a valgrind bug; even a simple Fortran program consisting of "end" causes the problem.


The crash with comment 4 is now a separate bug: PR 47565 

Thus, left to do for this PR is comment 5: A deep copy is missing.
Comment 14 janus 2011-02-02 14:10:41 UTC
(In reply to comment #13)
> Thus, left to do for this PR is comment 5: A deep copy is missing.

This is now PR 47586. Closing this one.
Comment 15 janus 2011-02-02 14:11:33 UTC
(In reply to comment #14)
> Closing this one.

For real!
Comment 16 Diego Novillo 2011-02-02 18:13:45 UTC
Author: dnovillo
Date: Wed Feb  2 18:13:38 2011
New Revision: 169733

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=169733
Log:
2011-01-31  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/47455
	* trans-expr.c (gfc_conv_procedure_call): Handle procedure pointers
	with pointer or allocatable result.


2011-01-31  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/47455
	* gfortran.dg/typebound_call_19.f03: New.

Added:
    branches/google/integration/gcc/testsuite/gfortran.dg/typebound_call_19.f03
Modified:
    branches/google/integration/gcc/fortran/ChangeLog
    branches/google/integration/gcc/fortran/trans-expr.c
    branches/google/integration/gcc/testsuite/ChangeLog