Re: [PATCH, Fortran] Allow CHARACTER literals in assignments and DATA statements

[Mark Eggleston]

PING - OK, to commit? I have a pending patch that needs this in place.

On 05/11/2019 09:55, Mark Eggleston wrote:

>
> On 25/10/2019 09:03, Tobias Burnus wrote:
>> Hello Mark, hi all,
>>
>> On 10/21/19 4:40 PM, Mark Eggleston wrote:
>>> This is an extension to support a legacy feature supported by other
>>> compilers such as flang and the sun compiler.  As I understand it
>>> this feature is associated with DEC so it enabled using
>>> -fdec-char-conversions and by -fdec.
>>>
>>> It allows character literals to be assigned to numeric (INTEGER,
>>> REAL, COMPLEX) and LOGICAL variables by direct assignment or in DATA
>>> statements.
>>
>>>     * arith.c (hollerith2representation): Use
>>> OPT_Wcharacter_truncation in
>>>     call to gfc_warning.
>>
>> This has two effects: First, it permits to toggle the warning on and
>> off; secondly, it disables the warning by default. It is enabled by
>> -Wall, however. – I think that's acceptable: while Holleriths are
>> less transparent as normal strings, for normal strings the result is
>> identical.
>>
>>
>>> + result->representation.string[result_len] = '\0'; /* For debugger  */
>>
>> Tiny nit: full stop after 'debugger'.
> Done.
>>
>>
>>> +/* Convert character to integer. The constant will be padded or
>>> truncated. */
>>
>> And here an extra space before '*/'.
> Done.
>>
>>
>>> +Allowing character literals to be used in a similar way to
>>> Hollerith constants
>>> +is a non-standard extension.
>>> +
>>> +Character literals can be used in @code{DATA} statements and
>>> assignments with
>>
>> I wonder whether one should mention here explicitly that only
>> default-kind (i.e. kind=1) character strings are permitted.
>> Additionally, I wonder whether -fdec-char-conversion should be
>> mentioned here – without, it is not supported and the error message
>> doesn't point to this option.
>>
> Now mentions -fdec-char-conversion and kind=1.
>>
>>> +
>>> +  /* Flang allows character conversions similar to Hollerith
>>> conversions
>>> +     - the first characters will be turned into ascii values. */
>>
>> Is this Flang or DEC or …? I thought we talk about legacy support and
>> Flang is not really legacy.
>>
>>
> Re-worded.
>>> --- a/gcc/fortran/resolve.c
>>> +++ b/gcc/fortran/resolve.c
>>>   +  if ((gfc_numeric_ts (&lhs->ts) || lhs->ts.type == BT_LOGICAL)
>>> +      && rhs->ts.type == BT_CHARACTER
>>> +      && rhs->expr_type != EXPR_CONSTANT)
>>> +    {
>>> +      gfc_error ("Cannot convert %s to %s at %L", gfc_typename (rhs),
>>> +         gfc_typename (lhs), &rhs->where);
>>> +      return false;
>>> +    }
>>
>> Maybe add a comment like:
>> /* Happens with flag_dec_char_conversions for nonconstant strings.  */
>> might help casual readers to understand where this if comes from.
>>
> Done.
>>
>>> @@ -331,8 +332,9 @@ gfc_conv_constant_to_tree (gfc_expr * expr)
>>>               gfc_build_string_const (expr->representation.length,
>>>                           expr->representation.string));
>>>         if (!integer_zerop (tmp) && !integer_onep (tmp))
>>> -        gfc_warning (0, "Assigning value other than 0 or 1 to LOGICAL"
>>> -             " has undefined result at %L", &expr->where);
>>> +        gfc_warning (OPT_Wsurprising, "Assigning value other than 0
>>> or 1 "
>>> +                 "to LOGICAL has undefined result at %L",
>>> +             &expr->where);
>>
>> I am not happy with this. We had odd issues with combining code
>> generated by gfortran and ifort and Booleans types ("logical").
>> Namely, gfortran uses 0 and 1 – while ifort uses -1 and 0. When using
>> ".not. var", it is sufficient to flip a single bit – either the first
>> or the last bit – and it is sufficient to look only a single bit.
>>
>> Hence, one can get ".not. var .eqv. var".
>>
>> The same result one can get when assigning "-1" to logical. Hence, a
>> default warning makes more sense than -Wsurprising. At least,
>> -Wsurprising is enabled by default.
>>
>> Hence, I wonder whether your 'OPT_Wsurprising' or
>> 'flag_dec_char_conversions ? OPT_Wsurprising : 0' makes more sense.
>>
> The latter.
>>
>> Actually, I don't quickly see whether   4_'string'  (i.e. kind=4)
>> strings are rejected or not. The gfc_character2* functions all assume
>> kind=1 characters – while code like gfc_convert_constant or the
>> resolve.c code only looks at BT_CHARACTER.
>> On the other hand, the add_conv calls in intrintrinsic.c's
>> add_conversions are only added for the default-character kind.
>>
>> In any case, can you add a test which checks that – even with
>> -fdec-char-conversion – assigning a 2_'string' and 4_'string' to a
>> integer/real/complex/logical will be rejected at compile time?
>>
> Did not add 2_'string' tests as 2 is not accepted as a valid kind for
> characters. The addition of 4_'string' in a data statement resulted in
> an ICE which has been fixed by only accepting characters of kind=1.
>> Otherwise, it looks okay to me.
>>
>> Tobias
>>
>>
> I noticed that warning were not produced for conversion to logicals,
> re-ordering of an if..else if sequence fixes that problem. Additional
> test cases have been added.
>
> Steve Kargl suggested a revision to the conversion warning adding
> "Nonstandard" to the text this has also been done.
>
> Tested on x86_64 using make -j 8 check-fortran.
>
> Please find attached the updated patch, the change logs follow. OK to
> commit?
>
> regards,
>
> Mark
>
> gcc/fortran/ChangeLog
>
>     Jim MacArthur  <[hidden email]>
>     Mark Eggleston  <[hidden email]>
>
>     * arith.c (hollerith2representation): Use
> OPT_Wcharacter_truncation in
>     call to gfc_warning.  Add character2representation,
> gfc_character2int,
>     gfc_character2real, gfc_character2complex and gfc_character2logical.
>     * arith.h: Add prototypes for gfc_character2int, gfc_character2real,
>     gfc_character2complex and gfc_character2logical.
>     * expr.c (gfc_check_assign): Return true if left hand side is numeric
>     or logical and the right hand side is character and of kind=1.
>     * gfortran.texi: Add -fdec-char-conversions.
>     * intrinsic.c (add_conversions): Add conversions from character to
>     integer, real, complex and logical types for their supported kinds.
>     (gfc_convert_type_warn): Reorder if..else if.. sequence so that
> warnings
>     are produced for conversion to logical.
>     * invoke.texi: Add option to list of options.
>     * invoke.texi: Add Character conversion subsection to Extensions
>     section.
>     * lang.opt: Add new option.
>     * options.c (set_dec_flags): Add SET_BITFLAG for
>     flag_dec_char_conversions.
>     * resolve.c (resolve_ordindary_assign): Issue error if the left hand
>     side is numeric or logical and the right hand side is a character
>     variable.
>     * simplify.c (gfc_convert_constant): Assign the conversion function
>     depending on destination type.
>     * trans-const.c (gfc_constant_to_tree): Use OPT_Wsurprising in
>     gfc_warning allowing the warning to be switched off only if
>     flag_dec_char_conversions is enabled.
>
> gcc/testsuite/gfortran.dg
>
>     Jim MacArthur <[hidden email]>
>     Mark Eggleston <[hidden email]>
>
>     PR fortran/89103
>     * gfortran.dg/dec_char_conversion_in_assignment_1.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_2.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_3.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_4.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_5.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_6.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_7.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_8.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_1.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_2.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_3.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_4.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_5.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_6.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_7.f90: New test.
>     * gfortran.dg/hollerith5.f90: Add -Wsurprising to options.
>     * gfortran.dg/hollerith_legacy.f90: Add -Wsurprising to options.
>     * gfortran.dg/no_char_to_numeric_assign.f90: New test.
>

--
https://www.codethink.co.uk/privacy.html

[Tobias Burnus-3]

On 11/8/19 10:45 AM, Mark Eggleston wrote:
> PING - OK, to commit? I have a pending patch that needs this in place.
Thanks for the ping. — Any chance, that you also work on some of the
general issues once in a while (cf. Bugzilla remark below); e.g. one bug
per week or fortnight? (Can be a simple one, just not to keep the number
of bugs growing.)

@Anyone – side question: Are there other pending patches? Ignoring one
OpenMP and several OpenACC patches, I am currently aware of:
* [Needs review] José's PR92142 - CFI_setpointer corrupts descriptor
patch [FSF copyright assignment exists]
* [Nearly ready, but needs fixes/follow-up patch] Steve's PR91178 for
code like 'call foo(a, (a))'
* [Approved but not committed] Paul's PR92123 patch for bind(C) + alloc
scalars.

Side remark: On the bind(C) side, I think PR 92189 should be fixed.
Additionally, there are several recent reports on segfaults and
regressions, looking Bugzilla (component = fortran, sort by change data).


Back to this pending patch:

On 11/5/19 10:55 AM, Mark Eggleston wrote:
> I noticed that warning were not produced for conversion to logicals,
> re-ordering of an if..else if sequence fixes that problem. Additional
> test cases have been added.
Looks as if reviewing and revisiting the patch was worthwhile :-) Thanks!

> Please find attached the updated patch, the change logs follow. OK to
> commit?

LGTM. Thanks for patch.

Cheers,

Tobias

PS: I still find it interesting, which Fortran code gets used; from a
supercomputing centre, I heard that everything between
afterwards-never-touched FORTRAN IV code to code using the latest
Fortran features is used. (But most amazing I find code, which gets
newly written in a FORTRAN 66 style/features with some sparse F20xx
features in between – like the "publicx" example of recently [instead of
'public x' or 'public::x'; spaces aren't that expensive anymore].)

> gcc/fortran/ChangeLog
>
>     Jim MacArthur  <[hidden email]>
>     Mark Eggleston  <[hidden email]>
>
>     * arith.c (hollerith2representation): Use
> OPT_Wcharacter_truncation in
>     call to gfc_warning.  Add character2representation,
> gfc_character2int,
>     gfc_character2real, gfc_character2complex and gfc_character2logical.
>     * arith.h: Add prototypes for gfc_character2int, gfc_character2real,
>     gfc_character2complex and gfc_character2logical.
>     * expr.c (gfc_check_assign): Return true if left hand side is numeric
>     or logical and the right hand side is character and of kind=1.
>     * gfortran.texi: Add -fdec-char-conversions.
>     * intrinsic.c (add_conversions): Add conversions from character to
>     integer, real, complex and logical types for their supported kinds.
>     (gfc_convert_type_warn): Reorder if..else if.. sequence so that
> warnings
>     are produced for conversion to logical.
>     * invoke.texi: Add option to list of options.
>     * invoke.texi: Add Character conversion subsection to Extensions
>     section.
>     * lang.opt: Add new option.
>     * options.c (set_dec_flags): Add SET_BITFLAG for
>     flag_dec_char_conversions.
>     * resolve.c (resolve_ordindary_assign): Issue error if the left hand
>     side is numeric or logical and the right hand side is a character
>     variable.
>     * simplify.c (gfc_convert_constant): Assign the conversion function
>     depending on destination type.
>     * trans-const.c (gfc_constant_to_tree): Use OPT_Wsurprising in
>     gfc_warning allowing the warning to be switched off only if
>     flag_dec_char_conversions is enabled.
>
> gcc/testsuite/gfortran.dg
>
>     Jim MacArthur <[hidden email]>
>     Mark Eggleston <[hidden email]>
>
>     PR fortran/89103
>     * gfortran.dg/dec_char_conversion_in_assignment_1.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_2.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_3.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_4.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_5.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_6.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_7.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_assignment_8.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_1.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_2.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_3.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_4.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_5.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_6.f90: New test.
>     * gfortran.dg/dec_char_conversion_in_data_7.f90: New test.
>     * gfortran.dg/hollerith5.f90: Add -Wsurprising to options.
>     * gfortran.dg/hollerith_legacy.f90: Add -Wsurprising to options.
>     * gfortran.dg/no_char_to_numeric_assign.f90: New test.

[Steve Kargl]

On Fri, Nov 08, 2019 at 11:17:21AM +0100, Tobias Burnus wrote:
> Additionally, there are several recent reports on segfaults and
> regressions, looking Bugzilla (component = fortran, sort by change data).
>

Easiest way to find open gfortran bugs is through the gfortran wiki.
https://gcc.gnu.org/wiki/GFortran
Go down to the "For gfrtran developers" section.

--
steve

[Jakub Jelinek]

On Fri, Nov 08, 2019 at 11:17:21AM +0100, Tobias Burnus wrote:

> >     Jim MacArthur <[hidden email]>
> >     Mark Eggleston <[hidden email]>
> >
> >     PR fortran/89103
> >     * gfortran.dg/dec_char_conversion_in_assignment_1.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_assignment_2.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_assignment_3.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_assignment_4.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_assignment_5.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_assignment_6.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_assignment_7.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_assignment_8.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_data_1.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_data_2.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_data_3.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_data_4.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_data_5.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_data_6.f90: New test.
> >     * gfortran.dg/dec_char_conversion_in_data_7.f90: New test.
> >     * gfortran.dg/hollerith5.f90: Add -Wsurprising to options.
> >     * gfortran.dg/hollerith_legacy.f90: Add -Wsurprising to options.
> >     * gfortran.dg/no_char_to_numeric_assign.f90: New test.

After full bootstrap/regtest, I've also noticed:
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_assignment_4.f90   -O0  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_assignment_4.f90   -O1  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_assignment_4.f90   -O2  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_assignment_4.f90   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_assignment_4.f90   -O3 -g  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_assignment_4.f90   -Os  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_data_3.f90   -O0  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_data_3.f90   -O1  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_data_3.f90   -O2  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_data_3.f90   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_data_3.f90   -O3 -g  compilation failed to produce executable
+UNRESOLVED: gfortran.dg/dec_char_conversion_in_data_3.f90   -Os  compilation failed to produce executable

Tests with dg-error are expected to fail compilation, so won't produce
executable and thus can't be run.

Fixed thusly, tested on x86_64-linux, committed to trunk as obvious.

2019-11-09  Jakub Jelinek  <[hidden email]>

        * gfortran.dg/dec_char_conversion_in_assignment_4.f90: Use
        dg-do compile instead of dg-do run.
        * gfortran.dg/dec_char_conversion_in_data_3.f90: Likewise.

--- testsuite/gfortran.dg/dec_char_conversion_in_assignment_4.f90 (revision 277993)
+++ testsuite/gfortran.dg/dec_char_conversion_in_assignment_4.f90 (working copy)
@@ -1,4 +1,4 @@
-! { dg-do run }
+! { dg-do compile }
 ! { dg-options "-fdec -fno-dec-char-conversions" }
 !
 ! Modified by Mark Eggleston <[hidden email]>
@@ -17,4 +17,3 @@ include "dec_char_conversion_in_assignme
 ! { dg-error "Cannot convert" " " { target *-*-* } 47 }
 ! { dg-error "Cannot convert" " " { target *-*-* } 48 }
 ! { dg-error "Cannot convert" " " { target *-*-* } 49 }
-
--- testsuite/gfortran.dg/dec_char_conversion_in_data_3.f90 (revision 277993)
+++ testsuite/gfortran.dg/dec_char_conversion_in_data_3.f90 (working copy)
@@ -1,4 +1,4 @@
-! { dg-do run }
+! { dg-do compile }
 ! { dg-options "-fdec -fno-dec-char-conversions" }
 !
 ! Modified by Mark Eggleston <[hidden email]>
@@ -17,4 +17,3 @@ include "dec_char_conversion_in_data_1.f
 ! { dg-error "Incompatible types" " " { target *-*-* } 68 }
 ! { dg-error "Incompatible types" " " { target *-*-* } 69 }
 ! { dg-error "Incompatible types" " " { target *-*-* } 70 }
-


        Jakub