Patchwork [Fortran] PR57217 - re-add type checks for TBP overriding

login
register
mail settings
Submitter Tobias Burnus
Date May 10, 2013, 10:33 a.m.
Message ID <518CCCED.5080104@net-b.de>
Download mbox | patch
Permalink /patch/242957/
State New
Headers show

Comments

Tobias Burnus - May 10, 2013, 10:33 a.m.
GCC 4.7 added some additional checks for type-bound procedure 
overriding. However, doing so it weakened the check whether the nonpass 
argument has the same type.

While for normal arguments, passing the parent type to an extended type 
is fine, for overriding the type (of nonpass arguments) must be exactly 
the same as in the original type.

The attached patch re-adds this check.

Build and regtested on x86-64-gnu-linux.
OK for the trunk and the 4.7/4.8 branches?

Tobias
Janus Weil - May 10, 2013, 4:16 p.m.
Hi Tobias,

> GCC 4.7 added some additional checks for type-bound procedure overriding.
> However, doing so it weakened the check whether the nonpass argument has the
> same type.
>
> While for normal arguments, passing the parent type to an extended type is
> fine, for overriding the type (of nonpass arguments) must be exactly the
> same as in the original type.

in principle I think your idea to tighten up the type check by
symmetrizing it is ok.

However, I'm not sure if the place where you do it is the most
preferable: I think it should rather go inside of
check_dummy_characteristics (since any check for the dummy
characteristics should include the 'strict' type check, and not only a
check of type compatibility, cf. F08:12.3.2.2).

Anyway, there already is a call to 'compare_type_rank' in
'check_dummy_characteristics'. So, you could either just symmetrize
this, or alternatively use a double 'gfc_compare_types' and a separate
check for the rank.

Moreover, I think your current patch misses e.g. the assumed-type
handling that is present in compare_type_rank.

Cheers,
Janus
Tobias Burnus - May 10, 2013, 4:33 p.m.
Hi Janus,

Janus Weil wrote:
> in principle I think your idea to tighten up the type check by
> symmetrizing it is ok.

I actually kind of copied it from the proc-pointer assignment check, 
which does the same.

> However, I'm not sure if the place where you do it is the most
> preferable: I think it should rather go inside of
> check_dummy_characteristics (since any check for the dummy
> characteristics should include the 'strict' type check, and not only a
> check of type compatibility, cf. F08:12.3.2.2).

Okay. Maybe one can at the same time replace the rank+type check by a 
type and a rank check to provide a better error message, i.e. show the 
type for a type mismatch (showing both types)* and the rank for a rank 
mismatch (showing both ranks).

(* best with special handling of "no_arg_check")

> Moreover, I think your current patch misses e.g. the assumed-type
> handling that is present in compare_type_rank.

Good point. Another reason to think about splitting the two.


Would you interested updating this patch? Or should I put it again on my 
growing to-do list?

Tobias
Janus Weil - May 10, 2013, 5:05 p.m.
Hi,

>> However, I'm not sure if the place where you do it is the most
>> preferable: I think it should rather go inside of
>> check_dummy_characteristics (since any check for the dummy
>> characteristics should include the 'strict' type check, and not only a
>> check of type compatibility, cf. F08:12.3.2.2).
>
> Okay. Maybe one can at the same time replace the rank+type check by a type
> and a rank check to provide a better error message, i.e. show the type for a
> type mismatch (showing both types)* and the rank for a rank mismatch
> (showing both ranks).

yes, actually I also thought that splitting it up might be a good idea.


>> Moreover, I think your current patch misses e.g. the assumed-type
>> handling that is present in compare_type_rank.
>
> Good point. Another reason to think about splitting the two.

Right.

However, the question is whether the patch will not become too
involved for backporting.


> Would you interested updating this patch? Or should I put it again on my
> growing to-do list?

My weekend is extremely tight, but I might find a quite hour in the
evenings during the next week (unless you can beat me to it) ...

Cheers,
Janus
Janus Weil - May 11, 2013, 3:58 p.m.
>>> However, I'm not sure if the place where you do it is the most
>>> preferable: I think it should rather go inside of
>>> check_dummy_characteristics (since any check for the dummy
>>> characteristics should include the 'strict' type check, and not only a
>>> check of type compatibility, cf. F08:12.3.2.2).
>>
>> Okay. Maybe one can at the same time replace the rank+type check by a type
>> and a rank check to provide a better error message, i.e. show the type for a
>> type mismatch (showing both types)* and the rank for a rank mismatch
>> (showing both ranks).
>
> yes, actually I also thought that splitting it up might be a good idea.
>
>
>>> Moreover, I think your current patch misses e.g. the assumed-type
>>> handling that is present in compare_type_rank.
>>
>> Good point. Another reason to think about splitting the two.
>
> Right.
>
> However, the question is whether the patch will not become too
> involved for backporting.

Ok, so: How about the attached patch as a simple & backportable fix
for the regression? (Ok for trunk/4.8/4.7?)

I suggest the rest (i.e. splitting up compare_type_rank, improving
error messages, etc) should be done in a follow-up patch on trunk
only. (I might even volunteer do to this once I find the time ...)

Cheers,
Janus
Janus Weil - May 11, 2013, 4:03 p.m.
>> in principle I think your idea to tighten up the type check by
>> symmetrizing it is ok.
>
> I actually kind of copied it from the proc-pointer assignment check, which
> does the same.

Btw, I think you refer to this piece of code in expr.c
(gfc_check_pointer_assign), right?


      if (!gfc_compare_interfaces (s1, s2, name, 0, 1,
                   err, sizeof(err), NULL, NULL))
    {
      gfc_error ("Interface mismatch in procedure pointer assignment "
             "at %L: %s", &rvalue->where, err);
      return false;
    }

      if (!gfc_compare_interfaces (s2, s1, name, 0, 1,
                   err, sizeof(err), NULL, NULL))
    {
      gfc_error ("Interface mismatch in procedure pointer assignment "
             "at %L: %s", &rvalue->where, err);
      return false;
    }


Since this will ultimately rely on check_dummy_characteristics, too,
it might even be possible to remove the duplication here, after my
patch introduces the symmetrization on a deeper level ... ?

Cheers,
Janus
Tobias Burnus - May 12, 2013, 4:18 p.m.
Janus Weil wrote:
> Ok, so: How about the attached patch as a simple & backportable fix 
> for the regression? (Ok for trunk/4.8/4.7?)

I think that part is okay - but as you mentioned TYPE(*) in your last 
email: That doesn't work; I think compare_type_rank should be made 
asymmetrical in this regard (ditto for "!gcc$ attributes no_arg_check"). 
Thus, could you fix that part as well?

> I suggest the rest (i.e. splitting up compare_type_rank, improving 
> error messages, etc) should be done in a follow-up patch on trunk 
> only. (I might even volunteer do to this once I find the time ...)

I am fine with that suggestion. Regarding the your other email in this 
thread: I was indeed talking about that part of the 
proc-pointer-assignment check.

Tobias
Janus Weil - May 27, 2013, 10:43 a.m.
Sorry for taking so long to come back to this (I was traveling all of
last week) ...


>> Ok, so: How about the attached patch as a simple & backportable fix for
>> the regression? (Ok for trunk/4.8/4.7?)
>
> I think that part is okay - but as you mentioned TYPE(*) in your last email:
> That doesn't work; I think compare_type_rank should be made asymmetrical in
> this regard (ditto for "!gcc$ attributes no_arg_check"). Thus, could you fix
> that part as well?

What do you mean? That "type(t)" arguments should be able to override
"type(*)", but not vice versa?

I think that would interfere with the current patch, which symmetrizes
the call to "compare_type_rank". If one direction gives a negative
result, then both cases will be rejected.

Anyway, anything in this direction is probably a non-regression and
should rather be handled as a follow-up. Is the current patch
(http://gcc.gnu.org/ml/fortran/2013-05/msg00045.html) ok for
trunk/4.8/4.7?

Cheers,
Janus
Tobias Burnus - May 27, 2013, 11:15 a.m.
Janus Weil wrote:
>>> Ok, so: How about the attached patch as a simple & backportable fix for
>>> the regression? (Ok for trunk/4.8/4.7?)
>> I think that part is okay - but as you mentioned TYPE(*) in your last email:
>> That doesn't work; I think compare_type_rank should be made asymmetrical in
>> this regard (ditto for "!gcc$ attributes no_arg_check"). Thus, could you fix
>> that part as well?
> What do you mean?

Try the four attached test cases - they should all be rejected, but they 
are accepted.

> Anyway, anything in this direction is probably a non-regression and
> should rather be handled as a follow-up. Is the current patch
> (http://gcc.gnu.org/ml/fortran/2013-05/msg00045.html) ok for
> trunk/4.8/4.7?

OK. Still, I would like if the attached test cases would be rejected. 
(The first one only affects GCC 4.9, the others also GCC4.8).

Tobias

PS: If you have time, could you please review 
http://gcc.gnu.org/ml/fortran/2013-05/msg00081.html (dealloc intent(in) 
poly array)?

(For the finalization wrapper patch, I will later send a follow-up patch 
which addresses some additional issues, I found while testing.)
Janus Weil - May 28, 2013, 11:52 a.m.
>> Anyway, anything in this direction is probably a non-regression and
>> should rather be handled as a follow-up. Is the current patch
>> (http://gcc.gnu.org/ml/fortran/2013-05/msg00045.html) ok for
>> trunk/4.8/4.7?
>
> OK.

Thanks. Committed to trunk as r199375. Will do 4.8 and 4.7 soon.


> Still, I would like if the attached test cases would be rejected. (The
> first one only affects GCC 4.9, the others also GCC4.8).

I'm leaving that for the follow-up (more of which was discussed upthread).

I guess one can argue whether the first one should actually be
rejected (the other two: certainly).


> PS: If you have time, could you please review
> http://gcc.gnu.org/ml/fortran/2013-05/msg00081.html (dealloc intent(in) poly
> array)?

Time is a rare commodity, but I'll see what I can do ... ;)

Cheers,
Janus

Patch

2013-05-10  Tobias Burnus  <burnus@net-b.de>

	PR fortran/57217
	* interface.c (gfc_check_typebound_override): Check whether
	nonpass types are identical same.

2013-05-10  Tobias Burnus  <burnus@net-b.de>

	PR fortran/57217
	* gfortran.dg/typebound_override_4.f90: New.
	* gfortran.dg/typebound_proc_6.f03: Update dg-error.

diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c
index 1b967fa..8f22e4c 100644
--- a/gcc/fortran/interface.c
+++ b/gcc/fortran/interface.c
@@ -4128,6 +4128,17 @@  gfc_check_typebound_override (gfc_symtree* proc, gfc_symtree* old)
 	}
 
       check_type = proc_pass_arg != argpos && old_pass_arg != argpos;
+      if (check_type
+          && (!gfc_compare_types (&proc_formal->sym->ts, &old_formal->sym->ts)
+	      || !gfc_compare_types (&old_formal->sym->ts,
+				     &proc_formal->sym->ts)))
+	{
+	  gfc_error ("Argument type mismatch for the overriding procedure "
+		     "'%s' at %L: %s shall be %s", proc->name, &where,
+		     gfc_typename (&proc_formal->sym->ts),
+		     gfc_typename (&old_formal->sym->ts));
+	  return false;
+	}
       if (!check_dummy_characteristics (proc_formal->sym, old_formal->sym, 
 					check_type, err, sizeof(err)))
 	{
diff --git a/gcc/testsuite/gfortran.dg/typebound_override_4.f90 b/gcc/testsuite/gfortran.dg/typebound_override_4.f90
new file mode 100644
index 0000000..e6f9805
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/typebound_override_4.f90
@@ -0,0 +1,36 @@ 
+! { dg-do compile }
+!
+! PR fortran/57217
+!
+! Contributed Salvatore Filippone
+!
+module base_mod
+  type base_type
+    integer :: kind
+  contains
+    procedure, pass(map)  :: clone    => base_clone
+  end type base_type
+contains
+  subroutine  base_clone(map,mapout,info)
+    implicit none
+    class(base_type), intent(inout) :: map
+    class(base_type), intent(inout) :: mapout
+    integer     :: info
+  end subroutine base_clone
+end module base_mod
+
+module r_mod
+  use base_mod
+  type, extends(base_type) :: r_type
+    real  :: dat
+  contains
+    procedure, pass(map)  :: clone    => r_clone ! { dg-error "Argument type mismatch for the overriding procedure 'clone' at .1.: CLASS.r_type. shall be CLASS.base_type." }
+  end type r_type
+contains
+  subroutine  r_clone(map,mapout,info)
+    implicit none
+    class(r_type), intent(inout) :: map
+    class(r_type), intent(inout) :: mapout
+    integer     :: info
+  end subroutine r_clone
+end module r_mod
diff --git a/gcc/testsuite/gfortran.dg/typebound_proc_6.f03 b/gcc/testsuite/gfortran.dg/typebound_proc_6.f03
index 3a32cbc..1fe2580 100644
--- a/gcc/testsuite/gfortran.dg/typebound_proc_6.f03
+++ b/gcc/testsuite/gfortran.dg/typebound_proc_6.f03
@@ -89,7 +89,7 @@  MODULE testmod
     ! For corresponding dummy arguments.
     PROCEDURE, PASS :: corresp1 => proc_tmeint ! Ok.
     PROCEDURE, PASS :: corresp2 => proc_tmeintx ! { dg-error "should be named 'a'" }
-    PROCEDURE, PASS :: corresp3 => proc_tmereal ! { dg-error "Type/rank mismatch in argument 'a'" }
+    PROCEDURE, PASS :: corresp3 => proc_tmereal ! { dg-error "Argument type mismatch for the overriding procedure 'corresp3' at .1.: REAL.4. shall be INTEGER.4." }
 
   END TYPE t