[Patch, Fortran] C Binding - module+intrinsic cleanup+bug fixes

[Tobias Burnus]

Hello,

Mikael Morin wrote:
>> The main change of this patch is to move all the special handling of the intrinsics from symbol.c to the normal intrinsics code in intrinsic.{c,h}, check.c, iresolve.c and trans-intrinsics.c. That also implied to do some larger changes to module.c. Additionally, I rewrote all the constraint checks from scratch, based on the Fortran 2003, 2008 and TS29113 standards - and fixed the fallout. Finally, I looked through the bugreports mentioning those intrinsics - and fixed some remaing issues (some were already fixed, either by this patch or since at least GCC 4.6).

Regarding the issues mentioned by Dominque:

* gfortran.dg/c_funloc_tests_8.f90: That was due to a bug in expr.c. I 
had attached an old patch.

* gfortran.dg/transfer_resolve_2.f90:

The way, the value was obtained, was bogus - it is now fixed. While 
doing so, I saw that a valid test case was rejected, cf. 
transfer_resolve_2.f90. Thus, the handling of type(c_ptr) was changed to 
be more in line with the normal code, i.e. symbol_private is properly 
set - and some special code could be removed.

However, that changed the following: gfortran allows as GNU extension to 
do "print *, c_ptr_var"; that continues to work. But it also allowed 
"print *, dt_with_c_ptr_comp" which no longer works. It could be fixed 
by walking all the components of a derived type, but I do not see this 
is important feature. Being able to print a c_ptr for debugging purpose 
can be useful, but printing it as part of a large derived type is 
questionable.


> Really impressive. You can also add PR 55574 to the list (test case
> attached).

Done. And thanks a lot for the quick and thorough review of the long patch!


> There is one thing in the patch I'm uncomfortable with, namely:
>
>> -  isym = gfc_find_subroutine (name);
>> +  if (c->symtree->n.sym->intmod_sym_id)
>> +    isym = gfc_intrinsic_subroutine_by_id ((gfc_isym_id)
>> +					c->symtree->n.sym->intmod_sym_id);
> [...]
> After investigating further, it seems that create_intrinsic_function
> sets the intmod_sym_id field to the gfc_isym_id id (even without your
> patch).
> This is confusing because the non-procedure symbols use as intmod_sym_id
> a ISOCBINDING_* id, whereas procedures use a GFC_ISYM_* id.
> I don't know how it ends up working, but I suggest the following change:
>   - store the ISOCBINDING_* id in intmod_sym_id
>   - retrieve the corresponding GFC_ISYM_* when needed (like above) using
> c_interop_kinds_table[...->intmod_sym_id].value

I followed the suggestion and created two auxiliary functions, 
gfc_isym_id_by_intmod and gfc_isym_id_by_intmod_sym. While calling 
c_interop_kinds_table works for ISO_C_BINDING, for ISO_FORTRAN_ENV, the 
code is a bit longer - hence, I factored it into a function.

> By the way, create_intrinsic_function could certainly be simplified if
> it was called from generate_isocbinding_symbol.

Yes, but only marginally (setting the DT as function result). A lot of 
the rest is required for ISO_Fortran_env's compiler_{options,version} 
and I wouldn't rule out that more such functions will be added in the 
upcoming TS and standards.

>> @@ -693,12 +693,15 @@ gfc_var_strlen (const gfc_expr *a)
>>       {
>>         long start_a, end_a;
>>   
>> +      if (!ra->u.ss.start || !ra->u.ss.end)
>> +	return -1;
>> +
> This is a bit conservative (though not wrong); ra->u.ss.start == NULL at
> least doesn't prevent string length evaluation.

Granted. I modified the code to assumed a start-index of 1 if 
ra->u.ss.start == NULL.


>> +/* Check whether an expression is interoperable. If all_len_okay is true,
>> +   all length-type parameters (for character) are allowed. Required for
>> +   C_LOC (cf. Fortran 2003corr5 or Fortran 2008).  */
> Could you add a comment about MSG?

Done.

> I think full sentences should be used for MSG

Done.

> You should also check ...->from_intmod; just in case there is a vicious
> user using a symbol (from a non-iso_c_binding intrinsic module) with the
> same id as C_FUN/C_FUNPTR. ;-)
Done.

> Could you simplify it a bit like this: if (a == ISOCBINDING_LOC) 
> return_type = c_ptr->n.sym; else if (a == ISOCBINDING_FUNLOC) 
> return_type = c_funptr->n.sym; else return_type = NULL; 
> create_intrinsic_function (..., return_type); 

Done.

>> +! { dg-compile }
> dg-do compile

Ups. Correct all this one and the other copy-and-paste victims.


Is the updated patch now okay for the trunk? (It was build and regtested 
on x86-64-gnu-linux.)

Tobias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: c-loc-cleanup-v2.diff
Type: text/x-patch
Size: 151696 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20130325/035cd3c2/attachment.bin>