Re: PR 60414: Patch proposal

Re: PR 60414: Patch proposal

[FX]

Hi Andre, and welcome aboard!

The explanation you give is nice, your patch submission looks clean. Two things:

  1. Do you have a copyright assignment on file with the FSF? See https://gcc.gnu.org/contribute.html

  2. Normally, all GCC patch submissions should be accompanied by a statement saying how you tested the patch. The standard thing to do is to check that the patched sources still bootstrap and that there is no regression in the testsuite. This can be stated as “Patch bootstrapped and regtested on <insert here your testing platform triplet, like x86_64-apple-darwin13>”. In that particular case, it might also be nice to indicate that not only the testcase doesn’t crash the compiler any more, but to confirm that it now generates the correct code (i.e. that we don’t turn an ice-on-valid bug into a wrong-code bug!).

Cheers,
FX

Re: PR 60414: Patch proposal

[Andre Vehreschild]

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

Hi FX,

thank you for your help:

ad 1) I thought the fixes to small for all the trouble to go through with a
copyright assignment. But if it is needed, I will happily give copyright to the
FSF and negotiate with my client all formal requirements. Unfortunately my
English is not well enough to understand the legal English on the site you
pointed me to. May I ask you to help me there? Is there a form I have to sign?
Where to I get it? What do you need from my client?

ad 2) Patch bootstrapped and regtested on x86_64-unknown-linux-gnu on a
standard PC running Fedora 19 (64 bit).

I have to apologize for the incomplete patch: The testcase
unlimited_polymorphism_18.f90 was missing. My fault. Please find the corrected
patch attached.

Regards,
	Andre

On Sat, 19 Jul 2014 00:12:13 +0200
FX <fxcoudert@gmail.com> wrote:

> Hi Andre, and welcome aboard!
> 
> The explanation you give is nice, your patch submission looks clean. Two
> things:
> 
>   1. Do you have a copyright assignment on file with the FSF? See
> https://gcc.gnu.org/contribute.html
> 
>   2. Normally, all GCC patch submissions should be accompanied by a statement
> saying how you tested the patch. The standard thing to do is to check that
> the patched sources still bootstrap and that there is no regression in the
> testsuite. This can be stated as “Patch bootstrapped and regtested on <insert
> here your testing platform triplet, like x86_64-apple-darwin13>”. In that
> particular case, it might also be nice to indicate that not only the testcase
> doesnÂ’t crash the compiler any more, but to confirm that it now generates the
> correct code (i.e. that we donÂ’t turn an ice-on-valid bug into a wrong-code
> bug!).
> 
> Cheers,
> FX
> 


-- 
Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
Tel.: +49 241 9291018 * Email: vehre@gmx.de 

[-- Attachment #2: pr60414_2.patch --]
[-- Type: text/x-patch, Size: 3393 bytes --]

diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog
index c33936b..cb01a13 100644
--- a/gcc/fortran/ChangeLog
+++ b/gcc/fortran/ChangeLog
@@ -1,3 +1,11 @@
+2014-07-19  Andre Vehreschild <vehre@gmx.de>
+
+	PR fortran/60414
+	* interface.c (compare_parameter): Fix compile bug: During resolution
+	of generic an array reference in the actual argument was not
+	respected. Fixed by checking, if the ref member is non-null. Testcase
+	unlimited_polymorphism_18.f90 add.
+
 2014-06-15  Tobias Burnus  <burnus@net-b.de>
 
 	* symbol.c (check_conflict): Add codimension conflict with
diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c
index b210d18..8658003 100644
--- a/gcc/fortran/interface.c
+++ b/gcc/fortran/interface.c
@@ -2156,7 +2156,10 @@ compare_parameter (gfc_symbol *formal, gfc_expr *actual,
   if (symbol_rank (formal) == actual->rank || symbol_rank (formal) == -1)
     return 1;
 
+  /* Only check ranks compatibility, if actual is not an array reference,
+     i.e., actual(i) indicated by actual->ref being set. */
   if (actual->ts.type == BT_CLASS && CLASS_DATA (actual)->as
+        && !actual->ref
 	&& CLASS_DATA (actual)->as->rank == symbol_rank (formal))
     return 1;
 
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 3202694..01d770f 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-07-19  Andre Vehreschild <vehre@gmx.de>
+
+	* gfortran.dg/unlimited_polymorphism_18.f90: Check according to 
+	PR 60414
+
 2014-07-18  Uros Bizjak  <ubizjak@gmail.com>
 
 	PR target/61794
diff --git a/gcc/testsuite/gfortran.dg/unlimited_polymorphic_18.f90 b/gcc/testsuite/gfortran.dg/unlimited_polymorphic_18.f90
new file mode 100644
index 0000000..661d0b7
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/unlimited_polymorphic_18.f90
@@ -0,0 +1,66 @@
+! { dg-do run }
+! Tests fix for PR60414 
+!
+module m
+    implicit none
+    Type T
+    contains
+        procedure :: FWrite
+        procedure :: FWriteArr
+        generic :: Write => FWrite, FWriteArr
+    end Type
+
+contains
+
+    subroutine FWrite(this,X)
+        class(T) this
+        class(*) X
+        real :: r
+        select type(X)
+            type is (real)
+                write (*, "(f3.1)", advance='no') X
+            class default 
+                write (*, *) "???"
+         end select
+    end subroutine FWrite
+
+    subroutine FWriteArr(this,X)
+        class(T) this
+        class(*) X(:)
+        integer i
+        do i = 1,6
+            call this%fwrite(X(i))
+            write (*, "(a)", advance="no") ", "
+        end do
+    end subroutine FWriteARr
+
+    subroutine WriteTextVector(vec, n, scal)
+        integer, intent(in) :: n
+        class(*), intent(in) :: vec(n)
+        class(*), intent(in) :: scal
+        integer j
+        Type(T) :: Tester
+
+        ! Write full vector
+        call Tester%Write(vec)
+        print *, ""
+        ! Write a scalar of the same class like the vector
+        call Tester%Write(scal)
+        print *, ""
+        ! Write an element of the vector, which is a scalar
+        j=3
+        call Tester%Write(vec(j))
+
+    end subroutine WriteTextVector
+
+end module
+
+program test
+   use :: m
+   implicit none
+  
+   real :: vec(1:6) = (/ 0, 1, 2, 3, 4, 5 /)
+   call writetextvector(vec, 6, 5.0)
+end program test 
+! { dg-final { cleanup-modules "m" } }
+

Re: PR 60414: Patch proposal

[Mikael Morin]

Hello, thanks for your contribution.

here are some comments about the patch:

Le 21/07/2014 15:03, Andre Vehreschild a écrit :
> diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog
> index c33936b..cb01a13 100644
> --- a/gcc/fortran/ChangeLog
> +++ b/gcc/fortran/ChangeLog
Changelogs are preferably provided outside of the patch as they almost
always conflict when applying it.

> @@ -1,3 +1,11 @@
> +2014-07-19  Andre Vehreschild <vehre@gmx.de>
> +
> +	PR fortran/60414
> +	* interface.c (compare_parameter): Fix compile bug: During resolution
> +	of generic an array reference in the actual argument was not
> +	respected. Fixed by checking, if the ref member is non-null.
You don't need to explain why (in the Changelog I mean), only _what_ is
changed should be there.

> Testcase
> +	unlimited_polymorphism_18.f90 add.
No need for that here; it's redundant with the testsuite Changelog.

> +
>  2014-06-15  Tobias Burnus  <burnus@net-b.de>
>
>  	* symbol.c (check_conflict): Add codimension conflict with
> diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c
> index b210d18..d84c888 100644
> --- a/gcc/fortran/interface.c
> +++ b/gcc/fortran/interface.c
> @@ -2156,7 +2156,11 @@ compare_parameter (gfc_symbol *formal, gfc_expr
> *actual,
>    if (symbol_rank (formal) == actual->rank || symbol_rank (formal) == -1)
>      return 1;
>
> +  /* Only check ranks compatibility, when the actual argument is not a
> +     reference of an array (foo(i)). A reference into an array is assumed
> +     when actual->ref is non null. */
>    if (actual->ts.type == BT_CLASS && CLASS_DATA (actual)->as
> +        && !actual->ref
>  	&& CLASS_DATA (actual)->as->rank == symbol_rank (formal))
>      return 1;
>
I think you have spotted the right place where the bug happens, but the
patch provided is not completely right for the following reason:

actual->ref points to the beginning of the reference chain, so
for example if actual is the gfc_expr struct for "var%comp%vec",
actual->ref points to the gfc_ref struct for "comp".
Furthermore, you have to be aware that even bare array variables
without sub-reference get an implicit full array reference in the AST, so
if actual is the gfc_expr struct for "array_var", actual->ref is
non-null and points (indirectly) to a gfc_array_ref of type AR_FULL.

So if you want to check for the absence of trailing sub-reference, you
have to walk down to the last reference chain.
But then you have to also support the case "var%array_comp%vec(1)" which
is supposed to have the rank of array_comp.
In fact I think the conditional supposed to support the CLASS cases is
completely bogus, and I don't see why the generic conditional just above
it wouldn't also support CLASS cases just fine.
Did you try removing the CLASS conditional entirely?

Mikael

Re: PR 60414: Patch proposal

[Dominique d'Humières]

Le 21 juil. 2014 à 15:03, Andre Vehreschild <vehre@gmx.de> a écrit :

> Hi Dominique,
> 
> thank you very much for your comments. I really appreciate them.

;-)

> Unfortunately I am contracted only for a limited number of around 6 bugs. The
> control on which bugs to pick is done by compiling a project and resolving the
> bugs that occur in order of their appearance.

If so, you should resolve (with your "client") the legal issues as soon as possible.

> I have reworked the test:
> 
> - results are now checked automatically against their expected value, and
> - unexpected results lead to an abort.
> 
> I hope this will do. The test for sure can be made more elegant, but my Fortran
> is to limited for that. I consider it pays more to invest my time into resolving
> bugs, then into writing most elegant test. What do you think?

The test is fine with me (I have regtested it). You don’t need the line

+! { dg-final { cleanup-modules "m" } }

This is done automatically by the gfortran testing machinery (I have checked it).

> The comment in interface.c is changed to:
> 
>  /* Only check ranks compatibility, when the actual argument is not a
>     reference of an array (foo(i)). A reference into an array is assumed
>     when actual->ref is non null. */
>  if (actual->ts.type == BT_CLASS && CLASS_DATA (actual)->as
>        && !actual->ref
> 	&& CLASS_DATA (actual)->as->rank == symbol_rank (formal))
>    return 1;

Although English is not my native language, I would prefer

/* Check ranks compatibility only when the actual argument is not a
   a reference to an element of the array, i.e., when actual->ref is null. */

Someone else should give you the formal approval.

Thanks for the patch,

Dominique

> The complete patch with the changes advised is attached in pr60414_3.patch.
> 
> Again, Bootstrapped and regextested on "x86_64-unknown-linux-gnu" w/o any
> regressions.
> 
> Regards,
> 	Andre
> -- 
> Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
> Tel.: +49 241 9291018 * Email: vehre@gmx.de 
> <pr60414_3.patch>

PR 60414: Patch proposal

[Andre Vehreschild]

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

Hi all,

this is my first try to submit a patch, so please be kind and correct me when I
do something wrong.

I was contracted to fix some issues listed in the bugtracker for fortran.
Please find attached my first attempt for bug PR60414 (I'll attach it to the
bug in the tracker in a second).

The bug terminates the compiler, because in the translation phase an unexpected
construction is seen as actual argument. I tracked down the location where the
decision to construct the parse tree seen is made and deduced, that the
parameter matching is not respecting the array_ref done in the code not
compiling. I have fixed this by checking if the ->ref member of the actual
argument is set. The analysis continues and the test case created for it
compile fine now.

Patch content:
- Changelog entry in gcc/fortran/Changelog
- Changelog entry in gcc/testsuite/Changelog
- Patch in interface.c: two line comment, one line actual code
- Testcase in gcc/testsuite/fortran.dg/unlimited_polymorphism_18.f90

Regards,
	Andre
-- 
Andre Vehreschild

[-- Attachment #2: pr60414.patch --]
[-- Type: text/x-patch, Size: 1635 bytes --]

diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog
index c33936b..cb01a13 100644
--- a/gcc/fortran/ChangeLog
+++ b/gcc/fortran/ChangeLog
@@ -1,3 +1,11 @@
+2014-07-19  Andre Vehreschild <vehre@gmx.de>
+
+	PR fortran/60414
+	* interface.c (compare_parameter): Fix compile bug: During resolution
+	of generic an array reference in the actual argument was not
+	respected. Fixed by checking, if the ref member is non-null. Testcase
+	unlimited_polymorphism_18.f90 add.
+
 2014-06-15  Tobias Burnus  <burnus@net-b.de>
 
 	* symbol.c (check_conflict): Add codimension conflict with
diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c
index b210d18..8658003 100644
--- a/gcc/fortran/interface.c
+++ b/gcc/fortran/interface.c
@@ -2156,7 +2156,10 @@ compare_parameter (gfc_symbol *formal, gfc_expr *actual,
   if (symbol_rank (formal) == actual->rank || symbol_rank (formal) == -1)
     return 1;
 
+  /* Only check ranks compatibility, if actual is not an array reference,
+     i.e., actual(i) indicated by actual->ref being set. */
   if (actual->ts.type == BT_CLASS && CLASS_DATA (actual)->as
+        && !actual->ref
 	&& CLASS_DATA (actual)->as->rank == symbol_rank (formal))
     return 1;
 
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index f6e9f23..84e16da 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-07-19  Andre Vehreschild <vehre@gmx.de>
+
+	* gfortran.dg/unlimited_polymorphism_18.f90: Check according to 
+	PR 60414
+
 2014-07-17  Richard Sandiford  <rdsandiford@googlemail.com>
 
 	* gcc.target/mips/umips-lwp-1.c (foo): Use a shift/add sequence