This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fortran patches


On Wed, Dec 5, 2018 at 7:03 PM Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
>
> On Wed, Dec 05, 2018 at 04:48:28PM -0500, Fritz Reese wrote:
[...]
> > RE:
> > >         PR fortran/88228
> > >         * expr.c (check_null, check_elemental): Work around -fdec and
> > >         initialization with logical operators operating on integers.
> >
> > I plan to review this section of the patch later today -- though the
> > patch hides the segfault from the PR, I need more time to determine
> > whether it is correct and complete.
>
> By the time the gfc_expr is given to check_check and check_elemental,
> it has been reduced to a EXPR_CONSTANT, which neither routine expected.
> I simply return early in that case.

It appears the correct solution is simply the following patch:

diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index b2090218d48..775a5c52c65 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -4004,7 +4004,7 @@ resolve_operator (gfc_expr *e)
          if (op2->ts.type != e->ts.type || op2->ts.kind != e->ts.kind)
            gfc_convert_type (op2, &e->ts, 1);
          e = logical_to_bitwise (e);
-         return resolve_function (e);
+         break;
        }

       sprintf (msg, _("Operands of logical operator %%<%s%%> at %%L
are %s/%s"),
@@ -4020,7 +4020,7 @@ resolve_operator (gfc_expr *e)
          e->ts.type = BT_INTEGER;
          e->ts.kind = op1->ts.kind;
          e = logical_to_bitwise (e);
-         return resolve_function (e);
+         break;
        }

       if (op1->ts.type == BT_LOGICAL)

Returning immediately short-circuits various checks and
simplifications which are done in the remainder of resolve_operator,
including gfc_simplify_expr which handles the EXPR_CONSTANT case. The
comments on gfc_reduce_init_expr indicate that check_null and
check_elemental should never get EXPR_CONSTANT anyway if
gfc_resolve_expr is correct. Regression tests verify this patch is
correct. Please use this patch instead for PR 88228, or if you prefer
I can submit/commit the patch myself.

>
> > RE:
> > >        PR fortran/88139
> > >        * dump-parse-tree.c (write_proc): Alternate return.
> > I dissent with this patch. The introduced error is meaningless and, as
> > mentioned by comment #3 in the PR, avoiding the ICE in dump-parse-tree
> > is not directly the issue. The code should be rejected in parsing. In
> > gcc-8.1 the invalid code is accepted (without an ICE) even without the
> > -fc-prototypes flag: I haven't finished building the compiler with
> > your changes yet to see whether that is still true afterwards, but at
> > least the test case doesn't try this, so I strongly suspect the patch
> > is incomplete to fix the PR.
>
> Comment #3 does not contain a patch to fix the problem elsewhere.
>
> In F2003, 15.2.6 "Interoperability of procedures and procedure interfaces",
> I cannot find a prohibition on an alternate return in a subroutine
> interface with BIND(C).
>
> I'm disinclined to let a patch fester in bugzilla to only attain
> the same fate as my patch to PR68544.

According to F2008 §15.3.7.2(5):

> any dummy argument without the VALUE attribute [...] is interoperable with an entity of the
> referenced type (ISO/IEC 9899:1999, 6.2.5, 7.17, and 7.18.1) of the formal parameter

Regardless of whether or not we accept alternate returns in BIND(C)
procedures, the compiler must be at least consistent: if we accept
them (which gfortran currently does), then we should be able to dump
the C prototype (with -fc-prototypes), providing a formal parameter
interoperable with the type of the alternate return dummy argument; if
we reject them, then we should issue the error in parsing (before
handling by -fc-prototypes). In either case, the error message should
not be obscure or meaningless. Even so, the patch here is inconsistent
since we accept the code, but issue an error when attempting to dump
the C prototype.

However, gfortran does not implement alternate return dummy arguments
as actual arguments, but rather using an integer return code
(regardless of the number of alternate return parameters in the
interface). One interpretation of the consequences of this are that
BIND(C) should be rejected, since there is no interoperable formal
parameter which can be used to mirror the dummy argument (required by
15.3.7.2.5 above). An alternate interpretation is that we can continue
to accept BIND(C) with alternate return dummy arguments, but just
ignore the alternate return arguments. The former is perhaps more
"correct"; the latter is perhaps more useful albeit potentially
error-prone.

To patch support for the latter case, rather than issuing an error in
write_proc for procedures with alternate return arguments, we should
output the actual interoperable prototype: in this case we would
output 'int' as the return type (rather than void, as usual for
subroutines) and alternate return dummy arguments would be ignored
(not output). So the output for the example in the PR should really be
'int f()'. Something like this should do it:

diff --git a/gcc/fortran/dump-parse-tree.c b/gcc/fortran/dump-parse-tree.c
index af64588786a..9d6c3945cc5 100644
--- a/gcc/fortran/dump-parse-tree.c
+++ b/gcc/fortran/dump-parse-tree.c
@@ -3239,19 +3239,41 @@ write_proc (gfc_symbol *sym)
   gfc_formal_arglist *f;
   const char *sym_name;
   const char *intent_in;
+  bool has_alternate_returns;

   if (sym->binding_label)
     sym_name = sym->binding_label;
   else
     sym_name = sym->name;

-  if (sym->ts.type == BT_UNKNOWN)
+  /* Look for alternate return placeholders.  */
+  for (f = gfc_sym_get_dummy_args (sym); f; f = f->next)
+    {
+      if (f->sym == NULL)
+       {
+         has_alternate_returns = true;
+         break;
+       }
+    }
+
+  gfc_typespec ts = sym->ts;
+  gfc_array_spec *as = sym->as;
+  if (has_alternate_returns)
+    {
+      /* Alternate returns are implemented as an integer return code from
+         an otherwise void subroutine; override this here.  */
+      ts.type = BT_INTEGER;
+      ts.kind = gfc_c_int_kind;
+      as = NULL;
+    }
+
+  if (!has_alternate_returns && sym->ts.type == BT_UNKNOWN)
     {
       fprintf (dumpfile, "void ");
       fputs (sym_name, dumpfile);
     }
   else
-    write_decl (&(sym->ts), sym->as, sym_name, true, &sym->declared_at);
+    write_decl (&ts, as, sym_name, true, &sym->declared_at);

   fputs (" (", dumpfile);

@@ -3259,6 +3281,12 @@ write_proc (gfc_symbol *sym)
     {
       gfc_symbol *s;
       s = f->sym;
+
+      /* Ignore alternate return dummy arguments, since they are handled
+         as an integer return value.  */
+      if (!s)
+       continue;
+
       rok = get_c_type_name (&(s->ts), NULL, &pre, &type_name, &asterisk,
                             &post, false);
       if (rok == T_ERROR)


EDIT:

It appears Thomas beat me to a reply - I believe he's suggested
something like what the above diff should provide. Perhaps this will
be a useful starting point for him.


> > RE:
> > >        PR fortran/88205
> > >        * io.c (gfc_match_open): STATUS must be CHARACTER type.
[...]
> If I used e->where one gets
>
> a.f90:2:32:
>
>     2 |    character(3), parameter :: a(0) = [character(3)::]
>       |                               1
> Error: FORMAT tag at (1) cannot be a zero-sized array
>
> Now, imagine a few hundred lines separating the two statements.
> I think the latter error locus is preferable.

Yes, I agree.

Swapping gfc_current_locus definitely works, but is possibly less
readable(+maintainable) than my other suggestion of passing loc down
as an argument... But that suggestion touches more code, so there are
merits to either approach. In either case I have no real issue with
this part of the patch regardless of implementation of the locus
workaround.


Fritz


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]