Re: [Patch, Fortran] PR55758 - Non-C_Bool handling with BIND(C)

[Janne Blomqvist <blomqvist.janne@gmail.com>]

On Sun, Jan 6, 2013 at 6:52 PM, Tobias Burnus <burnus@net-b.de> wrote:
> ** ping **
>
> Attached is a small variation, which in addition handles the case that a
> non-BOOL_C LOGICAL, Bind(C) dummy argument (or result variable) is used in a
> procedure call. In that case, the variable is now converted to a
> TYPE_PRECISION == 1 variable. -- The updated patch was build and regtested
> successfully.

Nice, this should fix a pitfall with the previous patch. I still worry
about these almost-but-not-quite logicals causing weird and very hard
to track down bugs. A slightly safer variant of the approach you
describe above would be to convert the variable directly after the
bind(c) procedure call; that should make it pretty fool-proof, AFAICS?
(in some cases that would be a bit of extra useless work, but I doubt
it would matter performance-wise).

> As written before, I believe that the patch avoids some pitfalls with C
> interoperability of logical variables:  On one hand, it improves
> cross-compiler portability by rejecting non C_BOOL ones with
> -std=f2003/f2008/f2008ts;

This part is certainly ok.

> on the other hand, it makes wrong-code issues due
> to using non-0/1 integers from C much less likely. In both cases, the
> type-precision==1 handling for non-BIND(C) Fortran LOGICALs or for Bind(C)
> LOGICAL(kind=C_BOOL) remains the same; hence, no optimization issue is
> caused.
>
>
> OK for the trunk?
>
> Tobias
>
> PS: If there is consensus that this patch is a bad idea, I propose to reject
> non-C_BOOL LOGICALs unconditionally as dummy argument or result variable of
> BIND(C) procedures. Or do you have a better suggestion?
>
>
>
> On December 30, 2012, Tobias Burnus wrote:
>>
>> Janne Blomqvist wrote:
>>>
>>> On Fri, Dec 28, 2012 at 12:31 AM, Tobias Burnus <burnus@net-b.de> wrote:
>>>>
>>>> a) The Fortran standard only defines LOGICAL(kind=C_Bool) as being
>>>> interoperable with C - no other LOGICAL type. That matches GCC: With gcc
>>>> (the C compiler) only _Bool is a BOOLEAN_TYPE with TYPE_PRECISION == 1.
>>>> Hence, this patch rejects other logical kinds as dummy argument/result
>>>> variable in BIND(C) procedures if -std=f2003/f2008/f2008ts is specified
>>>> (using -pedantic, one gets a warning).
>>>
>>> Sorry, I don't understand, what is the -pedantic warning about if it's
>>> already rejected? Or do you mean std=gnu -pedantic?
>>
>>
>> The latter. Actually, I use "gfc_notify_std(GFC_STD_GNU, ..." and just
>> observed the -pedantic result. (I have to admit that I never quite
>> understood - and still don't - what -pedantic exactly does.)
>>
>>>> b) As GNU extension, other logical kinds are accepted in BIND(C)
>>>> procedures;
>>>> however, as the main use of "LOGICAL(kind=4)" (for BIND(C) procedures)
>>>> is to
>>>> handle logical expressions which use C's int, one has to deal with all
>>>> integer values and not only 0 and 1. Hence, a normal integer type is
>>>> used
>>>> internally in that case. That has been done to avoid surprises of users
>>>> and
>>>> hard to trace bugs.
>>>
>>> Does this actually work robustly?
>>
>>
>> I think it does in the sense that it mitigates the problems related to
>> LOGICAL(kind=4) and BIND(C) procedures. No, if one thinks of it as full cure
>> for the problem. The only way to ensure this is to turn all of gfortran's
>> LOGICALs into integers - and even that won't prevent issues related to
>> interoperability with C's _Bool as that one expects only 0 and 1. Thus,
>> either C<->Fortran or Fortran <-> Fortran logical(kind=C_Bool) could still
>> lead to problems.
>>
>>> E.g. if you have a logical but really integer under the covers, what
>>> happens if you equivalence it with a "normal" logical variable.
>>
>>
>> Well, equivalencing of actual arguments / result variables is not allowed
>> (I think, not checked). Besides, if you have equivalenced two variables, if
>> you have set one, you may not access the other, e.g.:
>>
>> logical :: A
>> integer :: B
>> equivalence (A,B)
>> A = .true.
>> B = 1
>> if (A) ...
>>
>> is invalid as "A" is not defined, even if A = .true. and B = 1 have
>> exactly the same storage size and bit patterns and, hence, in practice "A"
>> would be a well defined .true.
>>
>>> Or pass it as an argument to a procedure expecting a normal logical etc.
>>
>>
>> If the value is only 1 or 0, there shouldn't be any problems. Only if one
>> uses in turn ".not. dummy" there might be one.
>>
>> The idea of the patch was only to mitigate the problems - a full cure is
>> not possible (cf. above). I think the most likely problematic code is
>>    if (.not. c_function())
>> which is fixed by the patch. And the hope is that fold-converting to a
>> type-precision=1, Boolean-type logical fixes most of the remaining issues.
>>
>> I think the current solution which only affects non-C_BOOL-kind actual
>> arguments and result variables of BIND(C) procedures is a good compromise.
>>
>> * * *
>>
>> But if others do not like this approach, one could turn the gfc_notify_std
>> into a gfc_error are completely reject logicals with kinds /= C_Bool for
>> dummy arguments/result variables in BIND(C) procedures. Would you prefer
>> that approach?
>>
>> (Doing so will break user code (e.g. Open MPI) and make users unhappy but
>> it will be a tad safer as the current patch.)
>>
>> Tobias
>>
>



-- 
Janne Blomqvist