[Bug fortran/103828] New: Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[Bug fortran/103828] New: Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103828

            Bug ID: 103828
           Summary: Type generated for CHARACTER(C_CHAR), VALUE arguments
                    is wrong
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: fxcoudert at gcc dot gnu.org
  Target Milestone: ---

On the aarch64-apple-darwin port (Apple Silicon), currently in late-stage
development, the runtime test gfortran.dg/c_kind_params.f90 fails. After
reducing it (https://github.com/iains/gcc-darwin-arm64/issues/73) we have
determined that the front-end is emitting wrong trees. Take this function:

subroutine param_test (a, b, c, d, e, f, g, h, v, w) bind(c)
  use, intrinsic :: iso_c_binding
  implicit none
  integer(c_int), value :: a, b, c, d, e, f, g, h
  character(c_char), value :: v
  integer(c_signed_char), value :: w
  if (w /= 1) call foo
end subroutine

where argument V is a scalar, pass-by-value, C char (an unsigned char, although
it does not matter for interoperability, because that's how the Fortran
front-end does things). The equivalent C function is therefore:

void param_test(int a, int b, int c, int d, int e, int f, int g, int h,
                unsigned char v, signed char w)
{
  if (w != 1)
    foo_();
}

But gcc can emit different arguments for the Fortran and C versions. The reason
is because of the tree type for the V argument, which is invalid as emitted by
the Fortran front-end.

Inserting a call to debug_tree() in gfc_get_function_type() shows that the type
for the V argument is:

 <array_type 0x1059e9260
    type <integer_type 0x105844348 character(kind=1) public unsigned QI
        size <integer_cst 0x105804db0 constant 8>
        unit-size <integer_cst 0x105804dc8 constant 1>
        align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x105844348 precision:8 min <integer_cst 0x105804de0 0> max <integer_cst
0x105804d80 255>
        pointer_to_this <pointer_type 0x105847f00>>
    string-flag QI size <integer_cst 0x105804db0 8> unit-size <integer_cst
0x105804dc8 1>
    align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x1059e9260
    domain <integer_type 0x1059e9110
        type <integer_type 0x105844738 integer(kind=8) public DI
            size <integer_cst 0x105804cc0 constant 64>
            unit-size <integer_cst 0x105804cd8 constant 8>
            align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x105844738 precision:64 min <integer_cst 0x105804f48 -9223372036854775808> max
<integer_cst 0x105804f60 9223372036854775807>
            pointer_to_this <pointer_type 0x1058575d0>>
        DI size <integer_cst 0x105804cc0 64> unit-size <integer_cst 0x105804cd8
8>
        align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x1059e9110 precision:64 min <integer_cst 0x105889c80 1> max <integer_cst
0x105889c80 1>>>


See how this is actually not a scalar character(kind=1), but as an array type.
On aarch64-apple-darwin, this leads to a different passing of the argument, and
therefore to the testcase failure.

Where is this type tree generated? It comes from gfc_sym_type(), where the is a
special treatment for BT_CHARACTER. Weirdly, we don't follow that (not sure why
the conditions don't cover this case). We go instead into
gfc_typenode_for_spec(), which calls gfc_get_character_type(), which returns an
array, which is wrong.

[Bug fortran/103828] Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103828

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
   Last reconfirmed|                            |2021-12-25
                 CC|                            |iains at gcc dot gnu.org
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

[Bug fortran/103828] Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103828

--- Comment #1 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
The condition for being treated as a special CHARACTER case in gfc_sym_type()
is:

  if (sym->ts.type == BT_CHARACTER
      && ((sym->attr.function && sym->attr.is_bind_c)
          || (sym->attr.result
              && sym->ns->proc_name
              && sym->ns->proc_name->attr.is_bind_c)
          || (sym->ts.deferred && (!sym->ts.u.cl
                                   || !sym->ts.u.cl->backend_decl))))

Here we're not a function, not a result variable, we don't have
sym->ts.deferred.

I am thinking we may want to do this:

--- a/gcc/fortran/trans-types.c
+++ b/gcc/fortran/trans-types.c
@@ -2262,14 +2262,14 @@ gfc_sym_type (gfc_symbol * sym, bool is_bind_c)

   if (sym->ts.type == BT_CHARACTER
       && ((sym->attr.function && sym->attr.is_bind_c)
-         || (sym->attr.result
+         || ((sym->attr.result || sym->attr.value)
              && sym->ns->proc_name
              && sym->ns->proc_name->attr.is_bind_c)
          || (sym->ts.deferred && (!sym->ts.u.cl
                                   || !sym->ts.u.cl->backend_decl))))

But while it does give the propre tree-type, it does not fix the global issue.

[Bug fortran/103828] Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103828

--- Comment #2 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
So, even modifying gfc_sym_type() in trans-types.c to emit the proper type does
not fix the issue. Why? Because the rug is pulled under our feet later. Turns
out there is a function that deals with this, much later in the front-end:
gfc_conv_scalar_char_value()

But that function is really wrong. It's called in generate_local_decl(), which
says:

       /* Modify the tree type for scalar character dummy arguments of bind(c)
         procedures if they are passed by value.  The tree type for them will
         be promoted to INTEGER_TYPE for the middle end, which appears to be
         what C would do with characters passed by-value.  The value attribute
         implies the dummy is a scalar.  */

and gfc_conv_scalar_char_value() does this:

       /* This becomes the nominal_type in
         function.c:assign_parm_find_data_types.  */
       TREE_TYPE (sym->backend_decl) = unsigned_char_type_node;
       /* This becomes the passed_type in
         function.c:assign_parm_find_data_types.  C promotes char to
         integer for argument passing.  */
       DECL_ARG_TYPE (sym->backend_decl) = unsigned_type_node;

But this is completely wrong. In C, `char` arguments are only promoted to `int`
when the destination type is unknown, i.e., in unprototyped functions (K&R
style) or variadic arguments. C interoperability only interoperates with
prototyped C functions, so this promotion should not happen, and `char` should
be passed as `char`!

I am attaching the patch under testing.

[Bug fortran/103828] Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103828

--- Comment #3 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Created attachment 52062
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52062&action=edit
Patch, v1

[Bug fortran/103828] Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103828

--- Comment #4 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Wait, there is more, lower in gfc_conv_scalar_char_value():

      /* If we have a constant character expression, make it into an
         integer.  */
      if ((*expr)->expr_type == EXPR_CONSTANT)
        {
          gfc_typespec ts;
          gfc_clear_ts (&ts);

          *expr = gfc_get_int_expr (gfc_default_character_kind, NULL,
                                    (*expr)->value.character.string[0]);
          if ((*expr)->ts.kind != gfc_c_int_kind)
            {
              /* The expr needs to be compatible with a C int.  If the
                 conversion fails, then the 2 causes an ICE.  */
              ts.type = BT_INTEGER;
              ts.kind = gfc_c_int_kind;
              gfc_convert_type (*expr, &ts, 2);
            }
        }

And… we're forcing it into a C int. Again. The commit that introduced this
function is:

+2007-08-06  Christopher D. Rickett  <crickett@lanl.gov>
+
+       PR fortran/32732
+       * trans-expr.c (gfc_conv_scalar_char_value): Convert the tree and
+       actual arg expressions for scalar characters passed by-value to
+       bind(c) routines.
+       (gfc_conv_function_call): Call gfc_conv_scalar_char_value.
+       * trans.h: Add prototype for gfc_conv_scalar_char_value.
+       * trans-decl.c (generate_local_decl): Convert by-value character
+       dummy args of bind(c) procedures using
+       gfc_conv_scalar_char_value.

It's interesting to note that, at the time, the testcase was failing on IA64
HP-UX, although the bug was still marked as fixed.

[Bug fortran/103828] Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103828

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch
                URL|                            |https://gcc.gnu.org/piperma
                   |                            |il/fortran/2021-December/05
                   |                            |7226.html

--- Comment #5 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Patch posted: https://gcc.gnu.org/pipermail/fortran/2021-December/057226.html

[Bug fortran/103828] Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103828

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED
   Target Milestone|---                         |12.0

--- Comment #6 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Fixed in
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=906b4e15ce84790c7657405238d61358e0893676

[Bug fortran/103828] Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103828

--- Comment #7 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to Francois-Xavier Coudert from comment #6)
> Fixed in
> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=906b4e15ce84790c7657405238d61358e0893676

should this get a note in the release notes due to being an ABI change?

[Bug fortran/103828] Type generated for CHARACTER(C_CHAR), VALUE arguments is wrong

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103828

--- Comment #8 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
I'm not sure if it really counts as an ABI change, given that I know no
existing target where this resulted in an actual change in the argument passing
convention. (i.e., where that test actually failed).