[PATCH,fortran]: proposed fix for PR 33020

[PATCH,fortran]: proposed fix for PR 33020

[Christopher D. Rickett]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 925 bytes --]

hi all,

the attached patch fixes PR 33020.  currently, the kind of the SHAPE 
parameter to c_f_pointer is set once the actual is seen.  however, this 
can prevent shape arguments of different kinds being used for different 
calls within the same namespace.  the SHAPE parameter should not be set 
because it needs to allow any valid integer kind.

bootstrapped and regtested on x86 linux with no new failures.

Chris

:ADDPATCH fortran:

ChangeLog entry:

2007-08-09  Christopher D. Rickett  <crickett@lanl.gov>

 	PR fortran/33020
 	* resolve.c (gfc_iso_c_sub_interface): Remove setting of type and
 	kind for optional SHAPE parameter of C_F_POINTER.

2007-08-09  Christopher D. Rickett  <crickett@lanl.gov>

 	PR fortran/33020
 	* gfortran.dg/c_f_pointer_shape_tests_2.f03: Update test to
 	include multiple kinds for SHAPE parameter within a single
 	namespace.
 	* gfortran.dg/c_f_pointer_shape_tests_2_driver.c: Ditto.

[-- Attachment #2: Type: TEXT/plain, Size: 3507 bytes --]

Index: gcc/testsuite/gfortran.dg/c_f_pointer_shape_tests_2_driver.c
===================================================================
--- gcc/testsuite/gfortran.dg/c_f_pointer_shape_tests_2_driver.c	(revision 127182)
+++ gcc/testsuite/gfortran.dg/c_f_pointer_shape_tests_2_driver.c	(working copy)
@@ -7,6 +7,7 @@ void test_long_long_2d(int *array, int n
 void test_long_1d(int *array, int num_elems);
 void test_int_1d(int *array, int num_elems);
 void test_short_1d(int *array, int num_elems);
+void test_mixed(int *array, int num_elems);
 
 int main(int argc, char **argv)
 {
@@ -36,6 +37,10 @@ int main(int argc, char **argv)
 
   /* Test c_f_pointer where SHAPE is of type integer, kind=c_short.  */
   test_short_1d(my_array, NUM_ELEMS);
-  
+
+  /* Test c_f_pointer where SHAPE is of type integer, kind=c_int and
+	  kind=c_long_long.  */
+  test_mixed(my_array, NUM_ELEMS);
+
   return 0;
 }
Index: gcc/testsuite/gfortran.dg/c_f_pointer_shape_tests_2.f03
===================================================================
--- gcc/testsuite/gfortran.dg/c_f_pointer_shape_tests_2.f03	(revision 127182)
+++ gcc/testsuite/gfortran.dg/c_f_pointer_shape_tests_2.f03	(working copy)
@@ -86,6 +86,29 @@ contains
        if(myArrayPtr(i) /= (i-1)) call abort ()
     end do
   end subroutine test_short_1d
+
+  subroutine test_mixed(cPtr, num_elems) bind(c)
+    use, intrinsic :: iso_c_binding
+    type(c_ptr), value :: cPtr
+    integer(c_int), value :: num_elems
+    integer, dimension(:), pointer :: myArrayPtr
+    integer(c_int), dimension(1) :: shape1
+    integer(c_long_long), dimension(1) :: shape2
+    integer :: i
+
+    shape1(1) = num_elems
+    call c_f_pointer(cPtr, myArrayPtr, shape1) 
+    do i = 1, num_elems
+       if(myArrayPtr(i) /= (i-1)) call abort ()
+    end do
+
+    nullify(myArrayPtr)
+    shape2(1) = num_elems
+    call c_f_pointer(cPtr, myArrayPtr, shape2) 
+    do i = 1, num_elems
+       if(myArrayPtr(i) /= (i-1)) call abort ()
+    end do
+  end subroutine test_mixed
 end module c_f_pointer_shape_tests_2
 ! { dg-final { cleanup-modules "c_f_pointer_shape_tests_2" } } 
 
Index: gcc/fortran/resolve.c
===================================================================
--- gcc/fortran/resolve.c	(revision 127182)
+++ gcc/fortran/resolve.c	(working copy)
@@ -2344,11 +2344,6 @@ gfc_iso_c_sub_interface (gfc_code *c, gf
      formal args) before resolving.  */
   gfc_procedure_use (sym, &c->ext.actual, &(c->loc));
 
-  /* Give the optional SHAPE formal arg a type now that we've done our
-     initial checking against the actual.  */
-  if (sym->intmod_sym_id == ISOCBINDING_F_POINTER)
-    sym->formal->next->next->sym->ts.type = BT_INTEGER;
-
   if ((sym->intmod_sym_id == ISOCBINDING_F_POINTER) ||
       (sym->intmod_sym_id == ISOCBINDING_F_PROCPOINTER))
     {
@@ -2389,13 +2384,6 @@ gfc_iso_c_sub_interface (gfc_code *c, gf
 	  /* the 1 means to add the optional arg to formal list */
 	  new_sym = get_iso_c_sym (sym, name, binding_label, 1);
 	 
-	  /* Set the kind for the SHAPE array to that of the actual
-	     (if given).  */
-	  if (c->ext.actual != NULL && c->ext.actual->next != NULL
-	      && c->ext.actual->next->expr->rank != 0)
-	    new_sym->formal->next->next->sym->ts.kind =
-	      c->ext.actual->next->next->expr->ts.kind;
-	 
 	  /* for error reporting, say it's declared where the original was */
 	  new_sym->declared_at = sym->declared_at;
 	}

Re: [PATCH,fortran]: proposed fix for PR 33020

[FX Coudert]

:REVIEWMAIL:

> the attached patch fixes PR 33020.  currently, the kind of the  
> SHAPE parameter to c_f_pointer is set once the actual is seen.   
> however, this can prevent shape arguments of different kinds being  
> used for different calls within the same namespace.  the SHAPE  
> parameter should not be set because it needs to allow any valid  
> integer kind.

Then why also remove the type, as follows?

> -  /* Give the optional SHAPE formal arg a type now that we've done  
> our
> -     initial checking against the actual.  */
> -  if (sym->intmod_sym_id == ISOCBINDING_F_POINTER)
> -    sym->formal->next->next->sym->ts.type = BT_INTEGER;

  I can understand that it needs to not have a kind, but it has to be  
an integer, right? With your patch, do you still correctly diagnose  
if someone tries to use a REAL array as shape?

If you answer to that question, it's OK to commit.

FX

PING Re: [PATCH,fortran]: proposed fix for PR 33020

[Christopher D. Rickett]

PING

On Thu, 9 Aug 2007, Christopher D. Rickett wrote:

> hi all,
>
> the attached patch fixes PR 33020.  currently, the kind of the SHAPE 
> parameter to c_f_pointer is set once the actual is seen.  however, this can 
> prevent shape arguments of different kinds being used for different calls 
> within the same namespace.  the SHAPE parameter should not be set because it 
> needs to allow any valid integer kind.
>
> bootstrapped and regtested on x86 linux with no new failures.
>
> Chris
>
> :ADDPATCH fortran:
>
> ChangeLog entry:
>
> 2007-08-09  Christopher D. Rickett  <crickett@lanl.gov>
>
> 	PR fortran/33020
> 	* resolve.c (gfc_iso_c_sub_interface): Remove setting of type and
> 	kind for optional SHAPE parameter of C_F_POINTER.
>
> 2007-08-09  Christopher D. Rickett  <crickett@lanl.gov>
>
> 	PR fortran/33020
> 	* gfortran.dg/c_f_pointer_shape_tests_2.f03: Update test to
> 	include multiple kinds for SHAPE parameter within a single
> 	namespace.
> 	* gfortran.dg/c_f_pointer_shape_tests_2_driver.c: Ditto.
>

Re: PING Re: [PATCH,fortran]: proposed fix for PR 33020

[Tobias Burnus]

Christopher D. Rickett wrote:
> PING

Well, FX had two questions, which I think remained unanswered:
>> the attached patch fixes PR 33020.  currently, the kind of the SHAPE
>> parameter to c_f_pointer is set once the actual is seen.  however,
>> this can prevent shape arguments of different kinds being used for
>> different calls within the same namespace.  the SHAPE parameter
>> should not be set because it needs to allow any valid integer kind.
>
> Then why also remove the type, as follows?
>
>> -  /* Give the optional SHAPE formal arg a type now that we've done our
>> -     initial checking against the actual.  */
>> -  if (sym->intmod_sym_id == ISOCBINDING_F_POINTER)
>> -    sym->formal->next->next->sym->ts.type = BT_INTEGER;
>
>  I can understand that it needs to not have a kind, but it has to be
> an integer, right? With your patch, do you still correctly diagnose if
> someone tries to use a REAL array as shape?
>
> If you answer to that question, it's OK to commit. 

Tobias

Re: PING Re: [PATCH,fortran]: proposed fix for PR 33020

[Christopher D. Rickett]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1610 bytes --]

i didn't see that mail from FX; i must have deleted it.

>> PING
>
> Well, FX had two questions, which I think remained unanswered:
>>> the attached patch fixes PR 33020.  currently, the kind of the SHAPE
>>> parameter to c_f_pointer is set once the actual is seen.  however,
>>> this can prevent shape arguments of different kinds being used for
>>> different calls within the same namespace.  the SHAPE parameter
>>> should not be set because it needs to allow any valid integer kind.
>>
>> Then why also remove the type, as follows?

i can't remember off-hand, but i think it had to do with code in checking 
the parameters.  the BT_VOID type was used to prevent type/kind checking. 
this checking is now done for c_f_pointer during gfc_iso_c_sub_interface.

>>> -  /* Give the optional SHAPE formal arg a type now that we've done our
>>> -     initial checking against the actual.  */
>>> -  if (sym->intmod_sym_id == ISOCBINDING_F_POINTER)
>>> -    sym->formal->next->next->sym->ts.type = BT_INTEGER;
>>
>>  I can understand that it needs to not have a kind, but it has to be
>> an integer, right? With your patch, do you still correctly diagnose if
>> someone tries to use a REAL array as shape?

yes.  the checks on type and rank for SHAPE are now done during 
gfc_iso_c_sub_interface.  i've attached a new testcase to illustrate that 
both the type and rank of SHAPE are enforced.  regtested fine on x86 
linux.

thanks.
Chris

ChangeLog entry for new test case:

2007-08-22  Christopher D. Rickett  <crickett@lanl.gov>

 	PR fortran/33020
 	* gfortran.dg/c_f_pointer_shape_tests_3.f03: New test case.

[-- Attachment #2: Type: TEXT/PLAIN, Size: 816 bytes --]

! { dg-do compile }
! Verify that the type and rank of the SHAPE argument are enforced.
module c_f_pointer_shape_tests_3
  use, intrinsic :: iso_c_binding
  
contains
  subroutine sub0(my_c_array) bind(c)
    type(c_ptr), value :: my_c_array
    integer(c_int), dimension(:), pointer :: my_array_ptr
    
    call c_f_pointer(my_c_array, my_array_ptr, (/ 10.0 /)) ! { dg-error "must be a rank 1 INTEGER array" }
  end subroutine sub0

  subroutine sub1(my_c_array) bind(c)
    type(c_ptr), value :: my_c_array
    integer(c_int), dimension(:), pointer :: my_array_ptr
    integer(c_int), dimension(1,1) :: shape

    shape(1,1) = 10
    call c_f_pointer(my_c_array, my_array_ptr, shape) ! { dg-error "must be a rank 1 INTEGER array" }
  end subroutine sub1
end module c_f_pointer_shape_tests_3

Re: PING Re: [PATCH,fortran]: proposed fix for PR 33020

[Tobias Burnus]

Hi Chris,

Christopher D. Rickett wrote:
>  regtested fine on x86 linux.
As the patch looks ok and FX question has been answered, I check it in
as Rev. 127719.

Thanks for the patch, which helps with the Fortran bindings for GSL, but ...

 * * *

... If you are looking for another PR to wipe out:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33040

This is yet another bug which prevents the compilation of the Fortran
bindings of GSL (and SEGFAULT on valid ICE). It is a null pointer
problem and crash debugging shows cm->name = "__c_ptr_c_address" which
points to a C bind bug.


Other than that there is (for Bind(C)) only PR 32600 left: Moving
c_associated_1/2 and c_f_pointer w/o shape= into trans*.c.

Tobias

Re: PING Re: [PATCH,fortran]: proposed fix for PR 33020

[Christopher D. Rickett]

hi Tobias,

>
> ... If you are looking for another PR to wipe out:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33040
>
> This is yet another bug which prevents the compilation of the Fortran
> bindings of GSL (and SEGFAULT on valid ICE). It is a null pointer
> problem and crash debugging shows cm->name = "__c_ptr_c_address" which
> points to a C bind bug.

i've just recently looked at this PR but haven't had much time to look 
into it.  oddly, i found that the initialization of the field is ok if you 
declare a variable of type fgsl_matrix; it only appears to ICE if you 
declare the return type of a function to fgsl_matrix and try initializing 
it (as is done in the given test case).

thanks.
Chris