[Bug fortran/46244] New: gfc_compare_derived_types is buggy

[Bug fortran/46244] New: gfc_compare_derived_types is buggy

[janus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

           Summary: gfc_compare_derived_types is buggy
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: janus@gcc.gnu.org


As noted by Dominique in PR 46196 comment #5, there is a problem in
gfc_compare_derived_types (interface.c):

--- ../_clean/gcc/fortran/interface.c    2010-10-27 23:47:20.000000000 +0200
+++ gcc/fortran/interface.c    2010-10-29 10:55:07.000000000 +0200
@@ -445,16 +445,16 @@ gfc_compare_derived_types (gfc_symbol *d
       /* Make sure that link lists do not put this function into an 
     endless recursive loop!  */
       if (!(dt1->ts.type == BT_DERIVED && derived1 == dt1->ts.u.derived)
-        && !(dt1->ts.type == BT_DERIVED && derived1 == dt1->ts.u.derived)
+        && !(dt2->ts.type == BT_DERIVED && derived2 == dt2->ts.u.derived)
        && gfc_compare_types (&dt1->ts, &dt2->ts) == 0)
    return 0;

       else if ((dt1->ts.type == BT_DERIVED && derived1 == dt1->ts.u.derived)
-        && !(dt1->ts.type == BT_DERIVED && derived1 == dt1->ts.u.derived))
+        && !(dt2->ts.type == BT_DERIVED && derived2 == dt2->ts.u.derived))
    return 0;

       else if (!(dt1->ts.type == BT_DERIVED && derived1 == dt1->ts.u.derived)
-        && (dt1->ts.type == BT_DERIVED && derived1 == dt1->ts.u.derived))
+        && (dt2->ts.type == BT_DERIVED && derived2 == dt2->ts.u.derived))
    return 0;

       dt1 = dt1->next;

[Bug fortran/46244] gfc_compare_derived_types is buggy

[janus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #1 from janus at gcc dot gnu.org 2010-10-30 14:30:27 UTC ---
Related test case by Mikael, resulting in a segfault (PR 46196 comment #11):

      module type_a
      type a
              sequence
      end type a
      end module type_a

      module type_a_bis
      type a
              sequence
      end type a
      end module type_a_bis

      use type_a
      use type_a_bis, only: b => a

      type(b) :: y
      call bar(y)

    contains

      subroutine bar (x)
        type(a) :: x
      end subroutine bar

  end

[Bug fortran/46244] gfc_compare_derived_types is buggy

[janus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #2 from janus at gcc dot gnu.org 2010-10-30 14:54:49 UTC ---
The buggy code in comment #0 is due to:

http://gcc.gnu.org/viewcvs?view=revision&revision=131238
http://gcc.gnu.org/viewcvs?view=revision&revision=131239

When looking for a test case which is sensitive to the change,
linked_list_1.f90 may be a good starting point.

[Bug fortran/46244] gfc_compare_derived_types is buggy

[janus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #3 from janus at gcc dot gnu.org 2010-10-30 15:04:29 UTC ---
The test case in comment #1 can be fixed by:

Index: gcc/fortran/interface.c
===================================================================
--- gcc/fortran/interface.c     (revision 166089)
+++ gcc/fortran/interface.c     (working copy)
@@ -424,6 +424,11 @@ gfc_compare_derived_types (gfc_symbol *derived1, g
      match.  */
   for (;;)
     {
+      if (dt1 == NULL && dt2 == NULL)
+       break;
+      if (dt1 == NULL || dt2 == NULL)
+       return 0;
+      
       if (strcmp (dt1->name, dt2->name) != 0)
        return 0;

@@ -459,11 +464,6 @@ gfc_compare_derived_types (gfc_symbol *derived1, g

       dt1 = dt1->next;
       dt2 = dt2->next;
-
-      if (dt1 == NULL && dt2 == NULL)
-       break;
-      if (dt1 == NULL || dt2 == NULL)
-       return 0;
     }

   return 1;

[Bug fortran/46244] gfc_compare_derived_types is buggy

[janus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #4 from janus at gcc dot gnu.org 2010-10-30 15:11:19 UTC ---
(In reply to comment #3)
> The test case in comment #1 can be fixed by:

Or better:

Index: gcc/fortran/interface.c
===================================================================
--- gcc/fortran/interface.c     (revision 166089)
+++ gcc/fortran/interface.c     (working copy)
@@ -422,8 +422,11 @@ gfc_compare_derived_types (gfc_symbol *derived1, g
   /* Since subtypes of SEQUENCE types must be SEQUENCE types as well, a
      simple test can speed things up.  Otherwise, lots of things have to
      match.  */
-  for (;;)
+  while (dt1 || dt2)
     {
+      if (dt1 == NULL || dt2 == NULL)
+       return 0;
+      
       if (strcmp (dt1->name, dt2->name) != 0)
        return 0;

@@ -459,11 +462,6 @@ gfc_compare_derived_types (gfc_symbol *derived1, g

       dt1 = dt1->next;
       dt2 = dt2->next;
-
-      if (dt1 == NULL && dt2 == NULL)
-       break;
-      if (dt1 == NULL || dt2 == NULL)
-       return 0;
     }

   return 1;

[Bug fortran/46244] gfc_compare_derived_types is buggy

[mikael at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

Mikael Morin <mikael at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikael at gcc dot gnu.org

--- Comment #5 from Mikael Morin <mikael at gcc dot gnu.org> 2010-10-30 16:02:27 UTC ---
Another one, with mutually recursive types

  module type_a_b
      type a
              sequence
              type(b), pointer :: pb
      end type a
      type b
              sequence
              type(a), pointer :: pa
      end type b
  end module type_a_b

  module type_a_b_bis
      type a
              sequence
              type(b), pointer :: pb
      end type a
      type b
              sequence
              type(a), pointer :: pa
      end type b
  end module type_a_b_bis

      use type_a_b, only: a1 => a, b1 => b
      use type_a_b_bis, only: a2 => a, b2 => b

      type(a2) :: y
      call foo(y)

    contains

      subroutine foo(x)
        type(a1) :: x
      end subroutine foo
  end

Infinite loop at compile time. I couldn't think of a sane person writing such
code anyway.

[Bug fortran/46244] gfc_compare_derived_types is buggy

[mikael at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #6 from Mikael Morin <mikael at gcc dot gnu.org> 2010-10-30 16:03:58 UTC ---
(In reply to comment #4)
> (In reply to comment #3)
> > The test case in comment #1 can be fixed by:
> 
> Or better:
> 
Pre-approved.

[Bug fortran/46244] gfc_compare_derived_types is buggy

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

Dominique d'Humieres <dominiq at lps dot ens.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus@net-b.de, pault at
                   |                            |gcc dot gnu.org

--- Comment #7 from Dominique d'Humieres <dominiq at lps dot ens.fr> 2010-11-05 13:56:29 UTC ---
Is the code in comment #5 valid Fortran? If no, what kind of error should it
emit? (If yes, I would be interested to know what it would be supposed to
do!-).

If I replace one of the 

      type b
              sequence
              type(a), pointer :: pa
      end type b

with

      type b
              sequence
              class(a), pointer :: pa
      end type b

the code is rejected with

Error: Type mismatch in argument 'x' at (1); passed TYPE(a) to TYPE(a)

Is this correct?

If I do the replacement in both places, I am back to the infinite loop (-> it
gives a segmentation fault).
Again is the modified code valid?

[Bug fortran/46244] gfc_compare_derived_types is buggy

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #8 from Tobias Burnus <burnus at gcc dot gnu.org> 2010-11-05 15:01:17 UTC ---
(In reply to comment #7)
>       type b
>               sequence
>               class(a), pointer :: pa
>       end type b

That's invalid per F2008's

"C436 (R425) If SEQUENCE appears, each data component shall be declared to be
of an intrinsic type or of a sequence type, and a type-bound-procedure-part
shall not appear."

Or as NAG puts it:
  Error: ac.f90, line 8: CLASS entity of SEQUENCE type A
and ifort has:

  ac.f90(8): error #8224: A derived type used with the CLASS keyword shall not
have the BIND attribute or SEQUENCE property.   [A]

[Bug fortran/46244] gfc_compare_derived_types is buggy

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #9 from Tobias Burnus <burnus at gcc dot gnu.org> 2010-11-06 13:00:27 UTC ---
(In reply to comment #8)
> (In reply to comment #7)
> >       type b
> >               sequence
> >               class(a), pointer :: pa
> >       end type b
> 
> That's invalid per F2008's C436

combined with C405 + 1.3.147.6. Draft patch:

--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -11493,7 +11493,8 @@ resolve_fl_derived (gfc_symbol *sym)

       if (sym->attr.sequence)
        {
-         if (c->ts.type == BT_DERIVED && c->ts.u.derived->attr.sequence == 0)
+         if ((c->ts.type == BT_DERIVED && c->ts.u.derived->attr.sequence == 0)
+             || c->ts.type == BT_CLASS)
            {
              gfc_error ("Component %s of SEQUENCE type declared at %L does "
                         "not have the SEQUENCE attribute",

[Bug fortran/46244] gfc_compare_derived_types is buggy

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #10 from Tobias Burnus <burnus at gcc dot gnu.org> 2010-11-10 17:45:10 UTC ---
Author: burnus
Date: Wed Nov 10 17:44:58 2010
New Revision: 166547

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=166547
Log:
2010-11-10  Tobias Burnus <burnus@net-b.de>

        PR fortran/46244
        * resolve.c (resolve_fl_derived): Don't allow CLASS in
        sequence/BIND(C) types.

2010-11-10  Tobias Burnus <burnus@net-b.de>

        PR fortran/46244
        * gfortran.dg/class_30.f90: New.


Added:
    trunk/gcc/testsuite/gfortran.dg/class_30.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/resolve.c
    trunk/gcc/testsuite/ChangeLog

[Bug fortran/46244] gfc_compare_derived_types is buggy

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #11 from Tobias Burnus <burnus at gcc dot gnu.org> 2010-11-10 17:49:24 UTC ---
The commit fixes the issue mentioned in comment 7 to comment 9.

TODO: comment 0 to comment 6, i.e. the test cases of comment 0, comment 1 and
comment 5. Comment 4 contains a patch (pre-approved in comment 6).

[Bug fortran/46244] gfc_compare_derived_types is buggy

[mikael at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #12 from Mikael Morin <mikael at gcc dot gnu.org> 2011-02-20 16:49:48 UTC ---
Created attachment 23412
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23412
Patch, no regression modulo an error message change in null_1.f90

[Bug fortran/46244] gfc_compare_derived_types is buggy

[mikael at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

Mikael Morin <mikael at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2011.02.20 16:50:19
     Ever Confirmed|0                           |1

[Bug fortran/46244] gfc_compare_derived_types is buggy

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #13 from Dominique d'Humieres <dominiq at lps dot ens.fr> 2011-02-20 20:49:44 UTC ---
I have applied the patch in comment #12 on top of revision 170344.

> Patch, no regression modulo an error message change in null_1.f90

The error is changed from

Error: Different types in pointer assignment at (1); attempted assignment of
REAL(8) to REAL(4)

to

Error: Different kind type parameters in pointer assignment at (1)

Although I prefer the first form, if it is proven that it is too much work to
recover it, the second one requires only to adjust the tests. More annoying the
patch breaks the 'move_alloc()' calls, e.g., pr42274 comment #1 or pr42769
comment #1 (apparently this new feature is not tested in the test suite).

While looking at the code I have noticed something odd at lines 408 of
gcc/fortran/interface.c (patched file):

  if (derived1 != NULL && derived2 != NULL
      && strcmp (derived1->name, derived2->name) == 0
      && derived1->module != NULL && derived2->module != NULL
      && strcmp (derived1->module, derived2->module) == 0)
    return 1;

  /* Compare type via the rules of the standard.  Both types must have
     the SEQUENCE attribute to be equal.  */

  if (strcmp (derived1->name, derived2->name))
    return 0;

If the test 'derived1 != NULL && derived2 != NULL'  is really required (i.e.,
derived1 or derived2 can be NULL when entering the proc), is not it also
required later in the code (e.g., strcmp (derived1->name, derived2->name))?

[Bug fortran/46244] gfc_compare_derived_types is buggy

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #14 from Dominique d'Humieres <dominiq at lps dot ens.fr> 2011-02-20 22:01:54 UTC ---
> (apparently this new feature is not tested in the test suite)

This wrong, there are several tests;-) The one that is missing is of the form:


module psb_base_mat_mod
  type  :: psb_base_sparse_mat
    integer, private     :: m, n
    integer, private     :: state, duplicate 
    logical, private     :: triangle, unitd, upper, sorted
  end type psb_base_sparse_mat
end module psb_base_mat_mod

module psb_d_base_mat_mod
  use psb_base_mat_mod
  integer, parameter  :: psb_dpk_ = kind(1.d0)
  type, extends(psb_base_sparse_mat) :: psb_d_base_sparse_mat
  end type psb_d_base_sparse_mat

  type, extends(psb_d_base_sparse_mat) :: psb_d_coo_sparse_mat
    integer              :: nnz
    integer, allocatable :: ia(:), ja(:)
    real(psb_dpk_), allocatable :: val(:)
  end type psb_d_coo_sparse_mat

end module psb_d_base_mat_mod

module psb_d_mat_mod
  use psb_d_base_mat_mod
  type :: psb_d_sparse_mat
    class(psb_d_base_sparse_mat), allocatable  :: a 
  contains
    procedure, pass(a) :: d_csclip
    generic, public    :: csclip =>  d_csclip
  end type psb_d_sparse_mat

contains 

  subroutine d_csclip(a,b,info)
    use psb_d_base_mat_mod
    implicit none

    class(psb_d_sparse_mat), intent(in) :: a
    class(psb_d_sparse_mat), intent(out) :: b
    integer,intent(out)                  :: info
    type(psb_d_coo_sparse_mat), allocatable  :: acoo

    info = 0
    allocate(acoo,stat=info)    
    if (info == 0) call move_alloc(acoo,b%a)

  end subroutine d_csclip

end module psb_d_mat_mod

[Bug fortran/46244] gfc_compare_derived_types is buggy

[mikael at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #15 from Mikael Morin <mikael at gcc dot gnu.org> 2011-02-20 23:13:32 UTC ---
(In reply to comment #13)
> Although I prefer the first form, if it is proven that it is too much work to
> recover it, the second one requires only to adjust the tests. 
I didn't notice there was a second part in the error message, so I thought the
new message was better (more precise). 
It is just a gfc_compare_type VS gfc_TK_compatible change, I believe. 

Actually none of the gfc_compare_type/gfc_TK_compatible changes are absolutely
necessary to fix this bug. It is just I found it odd that gfc_compare_type was
calling gfc_type_compatible (compatible types are not necessary
equal/equivalent), so I changed it so that gfc_type_compatible calls
gfc_compare_type instead. And then the new gfc_TK_compatible function to not
upset the testsuite. 

Maybe I just don't understand what "compare types" means. :-(


> More annoying the
> patch breaks the 'move_alloc()' calls, e.g., pr42274 comment #1 or pr42769
> comment #1 (apparently this new feature is not tested in the test suite).
Will look into it later.

> 
> While looking at the code I have noticed something odd at lines 408 of
> gcc/fortran/interface.c (patched file):
> 
>   if (derived1 != NULL && derived2 != NULL
>       && strcmp (derived1->name, derived2->name) == 0
>       && derived1->module != NULL && derived2->module != NULL
>       && strcmp (derived1->module, derived2->module) == 0)
>     return 1;
> 
>   /* Compare type via the rules of the standard.  Both types must have
>      the SEQUENCE attribute to be equal.  */
> 
>   if (strcmp (derived1->name, derived2->name))
>     return 0;
> 
> If the test 'derived1 != NULL && derived2 != NULL'  is really required (i.e.,
> derived1 or derived2 can be NULL when entering the proc), is not it also
> required later in the code (e.g., strcmp (derived1->name, derived2->name))?

Hem, yes, who wrote this? ( I hope it's not me ;-) ).

[Bug fortran/46244] gfc_compare_derived_types is buggy

[paul.richard.thomas at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #16 from paul.richard.thomas at gmail dot com <paul.richard.thomas at gmail dot com> 2011-02-21 08:25:51 UTC ---
Dear Mikael,

....snip....

> Actually none of the gfc_compare_type/gfc_TK_compatible changes are absolutely
> necessary to fix this bug. It is just I found it odd that gfc_compare_type was
> calling gfc_type_compatible (compatible types are not necessary
> equal/equivalent), so I changed it so that gfc_type_compatible calls
> gfc_compare_type instead. And then the new gfc_TK_compatible function to not
> upset the testsuite.
>
> Maybe I just don't understand what "compare types" means. :-(

....snip....

> Hem, yes, who wrote this? ( I hope it's not me ;-) ).

There have been a number of contributors over the years.  Therein lies
the problem.  Quick fix has been piled on quick fix and the result is
something of a mess.

"It is just I found it odd that gfc_compare_type was calling
gfc_type_compatible (compatible types are not necessary
equal/equivalent), so I changed it so that gfc_type_compatible calls
gfc_compare_type instead."

That was indeed very peculiar.  I know who the culprit was but I am
not telling :-)

If you have time to put this right I, for one, would be very pleased.

A

Paul

[Bug fortran/46244] gfc_compare_derived_types is buggy

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #17 from Dominique d'Humieres <dominiq at lps dot ens.fr> 2011-10-02 17:46:44 UTC ---
Created attachment 25397
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25397
updated patch for revision 179415

The patch in comment #12 no longer applies cleanly (the changes in resolve.c
are now obsolete, I have mechanically replaced four times !gfc_compare_types
with !gfc_TK_compatible, although it does not introduce regression, this should
be checked by someone understanding the code better than I do). I had also to
do a similar change in gcc/fortran/check.c to allow move_alloc_5.f90 and
select_type_23.f03 to pass (otherwise they fail with

Error: 'from' argument of 'move_alloc' intrinsic at (1) must be the same type
and kind as 'to'

I have also updated null_1.f90. With these changes I had no regression and my
tests passed without the glitch reported in comment #13 (the previous patch
probably exposed some rampant bug that has been fixed).

[Bug fortran/46244] gfc_compare_derived_types is buggy

[mikael at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46244

--- Comment #18 from Mikael Morin <mikael at gcc dot gnu.org> 2011-10-02 22:05:20 UTC ---
Created attachment 25400
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25400
the farthest I went about this pr

For what's worth, I have just unburied the patch above (only updated to recent
trunk, not tested in any way).
This is the farthest I went in fixing this PR before moving to something else.
As it wasn't submitted, I suppose it comes with some regressions.

[Bug fortran/46244] gfc_compare_derived_types is buggy

[mikael at gcc dot gnu.org]

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

Mikael Morin <mikael at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
           Assignee|mikael at gcc dot gnu.org          |unassigned at gcc dot gnu.org

--- Comment #21 from Mikael Morin <mikael at gcc dot gnu.org> ---
(In reply to Thomas Koenig from comment #20)
> Mikael, are you still planning to work on this?

No, I'm not working on this any more.