public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
@ 2019-02-17 18:19 Thomas Koenig
  2019-02-17 22:46 ` Alan Modra
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Thomas Koenig @ 2019-02-17 18:19 UTC (permalink / raw)
  To: fortran, gcc-patches; +Cc: Segher Boessenkool

[-- Attachment #1: Type: text/plain, Size: 2922 bytes --]

Hello world,

the attached patch fixes a rather bad ABI violation on POWER systems.

In the absence of an explicit interface and if a procedure is not in
the same file, gfortran currently generates wrong function decls -
a longstanding problem that also creates problems with LTO, because
it (correctly) complains about mismatched declarations.

Usually, we got lucky because the actual calling sequences generated
by the compiler with the wrong info happened to match the ones
with the correct info. However, our luck ran out on POWER because
as soon as arguments were passed in memory, things did not work
any more.  The test case in question (see attachments) produced
wrong code on POWER, but merely warned with LTO on other systems.

The method for solving this problem can be seen in the patch - if
there is no backend decl for an external procedure, simply generate
a formal argument list from the arguments.

Regression tests turned up a few ICEs (now fixed), plus two
very invalid test cases, which I think are not worth saving.

I suspect that this will also fix a few LTO bugs, but we can always
check that after this has been committed.

I'd still like confirmation from one of the POWER people that
this also fixes the bug on that architecture.

Should this still go into gcc-9?  Richard has indicated in the
PR that he thinks so.  I think so too, because of the severity
of the bug(s) this fixes.  Any bugs resulting from this could
be either a) ICE-on-valid (easily fixed) or b) somehow generating
a wrong decl, but we are already doing this as of this moment,
so things can not really be made much worse, and a lot better.

So, ok for trunk and for backport (with some time in between)
once gcc-8 has re-opened?

Regards

	Thomas

2019-02-17  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/87689
         * trans-decl.c (gfc_get_extern_function_decl): Add argument
         actual_args and pass it through to gfc_get_function_type.
         * trans-expr.c (conv_function_val): Add argument actual_args
         and pass it on to gfc_get_extern_function_decl.
         (conv_procedure_call): Pass actual arguments to conv_function_val.
         * trans-types.c (get_formal_from_actual_arglist): New function.
         (gfc_get_function_type): Add argument actual_args.  Generate
         formal args from actual args if necessary.
         * trans-types.h (gfc_get_function_type): Add optional argument.
         * trans.h (gfc_get_extern_function_decl): Add optional argument.

2019-02-17  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/87689
         * gfortran.dg/lto/20091028-1_0.f90: Remove invalid test
         case.
         * gfortran.dg/lto/20091028-1_1.c: Likewise.
         * gfortran.dg/lto/20091028-2_0.f90: Likewise.
         * gfortran.dg/lto/20091028-2_1.c: Likewise.
         * gfortran.dg/lto/pr87689_0.f: New file.
         * gfortran.dg/lto/pr87689_1.f: New file.

[-- Attachment #2: p3.diff --]
[-- Type: text/x-patch, Size: 4941 bytes --]

Index: trans-decl.c
===================================================================
--- trans-decl.c	(Revision 268968)
+++ trans-decl.c	(Arbeitskopie)
@@ -1962,7 +1962,7 @@ get_proc_pointer_decl (gfc_symbol *sym)
 /* Get a basic decl for an external function.  */
 
 tree
-gfc_get_extern_function_decl (gfc_symbol * sym)
+gfc_get_extern_function_decl (gfc_symbol * sym, gfc_actual_arglist *actual_args)
 {
   tree type;
   tree fndecl;
@@ -2135,7 +2135,7 @@ module_sym:
       mangled_name = gfc_sym_mangled_function_id (sym);
     }
 
-  type = gfc_get_function_type (sym);
+  type = gfc_get_function_type (sym, actual_args);
   fndecl = build_decl (input_location,
 		       FUNCTION_DECL, name, type);
 
Index: trans-expr.c
===================================================================
--- trans-expr.c	(Revision 268968)
+++ trans-expr.c	(Arbeitskopie)
@@ -3895,7 +3895,8 @@ conv_base_obj_fcn_val (gfc_se * se, tree base_obje
 
 
 static void
-conv_function_val (gfc_se * se, gfc_symbol * sym, gfc_expr * expr)
+conv_function_val (gfc_se * se, gfc_symbol * sym, gfc_expr * expr,
+		   gfc_actual_arglist *actual_args)
 {
   tree tmp;
 
@@ -3913,7 +3914,7 @@ static void
   else
     {
       if (!sym->backend_decl)
-	sym->backend_decl = gfc_get_extern_function_decl (sym);
+	sym->backend_decl = gfc_get_extern_function_decl (sym, actual_args);
 
       TREE_USED (sym->backend_decl) = 1;
 
@@ -6580,7 +6581,7 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
 
   /* Generate the actual call.  */
   if (base_object == NULL_TREE)
-    conv_function_val (se, sym, expr);
+    conv_function_val (se, sym, expr, args);
   else
     conv_base_obj_fcn_val (se, base_object, expr);
 
Index: trans-types.c
===================================================================
--- trans-types.c	(Revision 268968)
+++ trans-types.c	(Arbeitskopie)
@@ -2970,9 +2970,54 @@ create_fn_spec (gfc_symbol *sym, tree fntype)
   return build_type_attribute_variant (fntype, tmp);
 }
 
+/* Helper function - if we do not find an interface for a procedure,
+   construct it from the actual arglist.  Luckily, this can only
+   happen for call by reference, so the information we actually need
+   to provide (and which would be impossible to guess from the call
+   itself) is not actually needed.  */
 
+static void
+get_formal_from_actual_arglist (gfc_symbol *sym, gfc_actual_arglist *actual_args)
+{
+  gfc_actual_arglist *a;
+  gfc_formal_arglist **f;
+  gfc_symbol *s;
+  char name[GFC_MAX_SYMBOL_LEN + 1];
+  static int var_num;
+
+  f = &sym->formal;
+  for (a = actual_args; a != NULL; a = a->next)
+    {
+      if (a->expr)
+	{
+	  (*f) = gfc_get_formal_arglist ();
+	  snprintf (name, GFC_MAX_SYMBOL_LEN, "_formal_%d", var_num ++);
+	  gfc_get_symbol (name, NULL, &s);
+	  if (a->expr->ts.type == BT_PROCEDURE)
+	    {
+	      s->attr.flavor = FL_PROCEDURE;
+	    }
+	  else
+	    {
+	      s->ts = a->expr->ts;
+	      s->attr.flavor = FL_VARIABLE;
+	      if (a->expr->rank > 0)
+		{
+		  s->attr.dimension = 1;
+		  s->as = gfc_get_array_spec ();
+		  s->as->type = AS_ASSUMED_SIZE;
+		}
+	    }
+	  s->attr.dummy = 1;
+	  s->attr.intent = INTENT_UNKNOWN;
+	  (*f)->sym = s;
+	}
+      f = &((*f)->next);
+    }
+}
+
 tree
-gfc_get_function_type (gfc_symbol * sym)
+gfc_get_function_type (gfc_symbol * sym, gfc_actual_arglist *actual_args)
 {
   tree type;
   vec<tree, va_gc> *typelist = NULL;
@@ -3030,6 +3075,10 @@ tree
 	    vec_safe_push (typelist, build_pointer_type(gfc_charlen_type_node));
 	}
     }
+  if (sym->backend_decl == error_mark_node && actual_args != NULL
+      && sym->formal == NULL && (sym->attr.proc == PROC_EXTERNAL
+				 || sym->attr.proc == PROC_UNKNOWN))
+    get_formal_from_actual_arglist (sym, actual_args);
 
   /* Build the argument types for the function.  */
   for (f = gfc_sym_get_dummy_args (sym); f; f = f->next)
Index: trans-types.h
===================================================================
--- trans-types.h	(Revision 268968)
+++ trans-types.h	(Arbeitskopie)
@@ -88,7 +88,7 @@ tree gfc_sym_type (gfc_symbol *);
 tree gfc_typenode_for_spec (gfc_typespec *, int c = 0);
 int gfc_copy_dt_decls_ifequal (gfc_symbol *, gfc_symbol *, bool);
 
-tree gfc_get_function_type (gfc_symbol *);
+tree gfc_get_function_type (gfc_symbol *, gfc_actual_arglist *args = NULL);
 
 tree gfc_type_for_size (unsigned, int);
 tree gfc_type_for_mode (machine_mode, int);
Index: trans.h
===================================================================
--- trans.h	(Revision 268968)
+++ trans.h	(Arbeitskopie)
@@ -580,7 +580,8 @@ void gfc_merge_block_scope (stmtblock_t * block);
 tree gfc_get_label_decl (gfc_st_label *);
 
 /* Return the decl for an external function.  */
-tree gfc_get_extern_function_decl (gfc_symbol *);
+tree gfc_get_extern_function_decl (gfc_symbol *,
+				   gfc_actual_arglist *args = NULL);
 
 /* Return the decl for a function.  */
 tree gfc_get_function_decl (gfc_symbol *);

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: pr87689_0.f --]
[-- Type: text/x-fortran; name="pr87689_0.f", Size: 471 bytes --]

! { dg-lto-run }
! { dg-lto-options {{ -Wno-lto-type-mismatch }} }
! PR 87689 - this used to fail for POWER, plus it used to
! give warnings about mismatches with LTO.
! Original test case by Judicaël Grasset.
      program main
        implicit none
        character :: c
        character(len=20) :: res, doesntwork_p8
        external doesntwork_p8
        c = 'o'
        res = doesntwork_p8(c,1,2,3,4,5,6)
        if (res /= 'foo') stop 3
      end program main

[-- Attachment #4: pr87689_1.f --]
[-- Type: text/x-fortran, Size: 364 bytes --]

      function doesntwork_p8(c,a1,a2,a3,a4,a5,a6)
        implicit none
        character(len=20) :: doesntwork_p8
        character :: c
        integer :: a1,a2,a3,a4,a5,a6
        if (a1 /= 1 .or. a2 /= 2 .or. a3 /= 3 .or. a4 /= 4 .or. a5 /= 5
     &       .or. a6 /= 6) stop 1
       if (c /= 'o ') stop 2
       doesntwork_p8 = 'foo'
       return
       end

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-17 18:19 [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER Thomas Koenig
@ 2019-02-17 22:46 ` Alan Modra
  2019-02-18  8:49 ` Janne Blomqvist
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Alan Modra @ 2019-02-17 22:46 UTC (permalink / raw)
  To: Thomas Koenig; +Cc: fortran, gcc-patches, Segher Boessenkool

On Sun, Feb 17, 2019 at 07:18:52PM +0100, Thomas Koenig wrote:
> I'd still like confirmation from one of the POWER people that
> this also fixes the bug on that architecture.

I bootstrapped and regression tested on powerpc64le-linux, and confirm
that the patch fixes the bug.  Thanks!

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-17 18:19 [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER Thomas Koenig
  2019-02-17 22:46 ` Alan Modra
@ 2019-02-18  8:49 ` Janne Blomqvist
  2019-02-18 17:25   ` Segher Boessenkool
  2019-02-18 12:51 ` Richard Biener
       [not found] ` <b7a9915031f14b3d9ba908109f1fcd21@ap-embx16-sp30.RES.AD.JPL>
  3 siblings, 1 reply; 14+ messages in thread
From: Janne Blomqvist @ 2019-02-18  8:49 UTC (permalink / raw)
  To: Thomas Koenig; +Cc: fortran, gcc-patches, Segher Boessenkool

On Sun, Feb 17, 2019 at 8:19 PM Thomas Koenig <tkoenig@netcologne.de> wrote:
>
> Hello world,
>
> the attached patch fixes a rather bad ABI violation on POWER systems.
>
> In the absence of an explicit interface and if a procedure is not in
> the same file, gfortran currently generates wrong function decls -
> a longstanding problem that also creates problems with LTO, because
> it (correctly) complains about mismatched declarations.
>
> Usually, we got lucky because the actual calling sequences generated
> by the compiler with the wrong info happened to match the ones
> with the correct info. However, our luck ran out on POWER because
> as soon as arguments were passed in memory, things did not work
> any more.  The test case in question (see attachments) produced
> wrong code on POWER, but merely warned with LTO on other systems.
>
> The method for solving this problem can be seen in the patch - if
> there is no backend decl for an external procedure, simply generate
> a formal argument list from the arguments.
>
> Regression tests turned up a few ICEs (now fixed), plus two
> very invalid test cases, which I think are not worth saving.
>
> I suspect that this will also fix a few LTO bugs, but we can always
> check that after this has been committed.
>
> I'd still like confirmation from one of the POWER people that
> this also fixes the bug on that architecture.
>
> Should this still go into gcc-9?  Richard has indicated in the
> PR that he thinks so.  I think so too, because of the severity
> of the bug(s) this fixes.  Any bugs resulting from this could
> be either a) ICE-on-valid (easily fixed) or b) somehow generating
> a wrong decl, but we are already doing this as of this moment,
> so things can not really be made much worse, and a lot better.
>
> So, ok for trunk and for backport (with some time in between)
> once gcc-8 has re-opened?

I agree, although we're close to the GCC-9 release, it's better to get
this in now. So Ok.

I wonder if we shouldn't exorcise all the varargs stuff, it seems to
cause more problems than benefits? But not in stage4 if we can avoid
it..

-- 
Janne Blomqvist

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-17 18:19 [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER Thomas Koenig
  2019-02-17 22:46 ` Alan Modra
  2019-02-18  8:49 ` Janne Blomqvist
@ 2019-02-18 12:51 ` Richard Biener
  2019-02-18 13:08   ` Janne Blomqvist
  2019-02-18 18:35   ` Thomas Koenig
       [not found] ` <b7a9915031f14b3d9ba908109f1fcd21@ap-embx16-sp30.RES.AD.JPL>
  3 siblings, 2 replies; 14+ messages in thread
From: Richard Biener @ 2019-02-18 12:51 UTC (permalink / raw)
  To: Thomas Koenig; +Cc: fortran, gcc-patches, Segher Boessenkool

On Sun, Feb 17, 2019 at 7:19 PM Thomas Koenig <tkoenig@netcologne.de> wrote:
>
> Hello world,
>
> the attached patch fixes a rather bad ABI violation on POWER systems.
>
> In the absence of an explicit interface and if a procedure is not in
> the same file, gfortran currently generates wrong function decls -
> a longstanding problem that also creates problems with LTO, because
> it (correctly) complains about mismatched declarations.
>
> Usually, we got lucky because the actual calling sequences generated
> by the compiler with the wrong info happened to match the ones
> with the correct info. However, our luck ran out on POWER because
> as soon as arguments were passed in memory, things did not work
> any more.  The test case in question (see attachments) produced
> wrong code on POWER, but merely warned with LTO on other systems.
>
> The method for solving this problem can be seen in the patch - if
> there is no backend decl for an external procedure, simply generate
> a formal argument list from the arguments.
>
> Regression tests turned up a few ICEs (now fixed), plus two
> very invalid test cases, which I think are not worth saving.

They were added to verify we don't ICE for such invalid testcases.
How do they fail now?

> I suspect that this will also fix a few LTO bugs, but we can always
> check that after this has been committed.
>
> I'd still like confirmation from one of the POWER people that
> this also fixes the bug on that architecture.
>
> Should this still go into gcc-9?  Richard has indicated in the
> PR that he thinks so.  I think so too, because of the severity
> of the bug(s) this fixes.  Any bugs resulting from this could
> be either a) ICE-on-valid (easily fixed) or b) somehow generating
> a wrong decl, but we are already doing this as of this moment,
> so things can not really be made much worse, and a lot better.
>
> So, ok for trunk and for backport (with some time in between)
> once gcc-8 has re-opened?

The patch looks good to me.  I wonder how the frontend handles
the 2nd call to doesntwork_p8 for

      program main
        implicit none
        character :: c
        character(len=20) :: res, doesntwork_p8
        external doesntwork_p8
        c = 'o'
        res = doesntwork_p8(c,1,2,3,4,5,6)
        res = doesntwork_p8(1,2)
        if (res /= 'foo') stop 3
      end program main

at least I get no diagnostic and the patch suggests whatever call
gets there first determines the backend-decl and its type?  At least
when I omit res = from the second call I get

t.f:4:47:

    4 |         character(len=20) :: res, doesntwork_p8
      |                                               1
......
    8 |         call doesntwork_p8(1,2)
      |
Error: ‘doesntwork_p8’ at (1) has a type, which is not consistent with
the CALL at (2)

Does the Fortran standard say anything in how the above case should be handled?

Richard.

>
> Regards
>
>         Thomas
>
> 2019-02-17  Thomas Koenig  <tkoenig@gcc.gnu.org>
>
>          PR fortran/87689
>          * trans-decl.c (gfc_get_extern_function_decl): Add argument
>          actual_args and pass it through to gfc_get_function_type.
>          * trans-expr.c (conv_function_val): Add argument actual_args
>          and pass it on to gfc_get_extern_function_decl.
>          (conv_procedure_call): Pass actual arguments to conv_function_val.
>          * trans-types.c (get_formal_from_actual_arglist): New function.
>          (gfc_get_function_type): Add argument actual_args.  Generate
>          formal args from actual args if necessary.
>          * trans-types.h (gfc_get_function_type): Add optional argument.
>          * trans.h (gfc_get_extern_function_decl): Add optional argument.
>
> 2019-02-17  Thomas Koenig  <tkoenig@gcc.gnu.org>
>
>          PR fortran/87689
>          * gfortran.dg/lto/20091028-1_0.f90: Remove invalid test
>          case.
>          * gfortran.dg/lto/20091028-1_1.c: Likewise.
>          * gfortran.dg/lto/20091028-2_0.f90: Likewise.
>          * gfortran.dg/lto/20091028-2_1.c: Likewise.
>          * gfortran.dg/lto/pr87689_0.f: New file.
>          * gfortran.dg/lto/pr87689_1.f: New file.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-18 12:51 ` Richard Biener
@ 2019-02-18 13:08   ` Janne Blomqvist
  2019-02-18 18:35   ` Thomas Koenig
  1 sibling, 0 replies; 14+ messages in thread
From: Janne Blomqvist @ 2019-02-18 13:08 UTC (permalink / raw)
  To: Richard Biener; +Cc: Thomas Koenig, fortran, gcc-patches, Segher Boessenkool

On Mon, Feb 18, 2019 at 2:51 PM Richard Biener
<richard.guenther@gmail.com> wrote:
> The patch looks good to me.  I wonder how the frontend handles
> the 2nd call to doesntwork_p8 for
>
>       program main
>         implicit none
>         character :: c
>         character(len=20) :: res, doesntwork_p8
>         external doesntwork_p8
>         c = 'o'
>         res = doesntwork_p8(c,1,2,3,4,5,6)
>         res = doesntwork_p8(1,2)
>         if (res /= 'foo') stop 3
>       end program main
>
> at least I get no diagnostic and the patch suggests whatever call
> gets there first determines the backend-decl and its type?  At least
> when I omit res = from the second call I get
>
> t.f:4:47:
>
>     4 |         character(len=20) :: res, doesntwork_p8
>       |                                               1
> ......
>     8 |         call doesntwork_p8(1,2)
>       |
> Error: ‘doesntwork_p8’ at (1) has a type, which is not consistent with
> the CALL at (2)
>
> Does the Fortran standard say anything in how the above case should be handled?

Without actually checking, I'm pretty sure a procedure is either a
function (something that returns a value) or a subroutine (does NOT
return a value, like a void function in C), but not both (some of the
intrinsics in gfortran can be called both as a function or a
subroutine, but that's a gfortran-specific extension (inherited from
g77, AFAIK), not part of the language, and only for some intrinsics
that the compiler handles in a special way, not a generic user-defined
procedure). So the example you show above is invalid.


-- 
Janne Blomqvist

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-18  8:49 ` Janne Blomqvist
@ 2019-02-18 17:25   ` Segher Boessenkool
  2019-02-18 20:16     ` Janne Blomqvist
  0 siblings, 1 reply; 14+ messages in thread
From: Segher Boessenkool @ 2019-02-18 17:25 UTC (permalink / raw)
  To: Janne Blomqvist; +Cc: Thomas Koenig, fortran, gcc-patches

On Mon, Feb 18, 2019 at 10:48:35AM +0200, Janne Blomqvist wrote:
> I wonder if we shouldn't exorcise all the varargs stuff, it seems to
> cause more problems than benefits? But not in stage4 if we can avoid
> it..

On the Power ABIs at least, unprototyped functions (a K&R thing for C) are
handled the same as varargs (with zero fixed arguments).  How does this
tie in with Fortran requirements?


Segher

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-18 12:51 ` Richard Biener
  2019-02-18 13:08   ` Janne Blomqvist
@ 2019-02-18 18:35   ` Thomas Koenig
  1 sibling, 0 replies; 14+ messages in thread
From: Thomas Koenig @ 2019-02-18 18:35 UTC (permalink / raw)
  To: Richard Biener; +Cc: fortran, gcc-patches, Segher Boessenkool

I have now committed the patch as r268992.  Janne and Richard, thanks
for the review and the comments.

Am 18.02.19 um 13:50 schrieb Richard Biener:
> On Sun, Feb 17, 2019 at 7:19 PM Thomas Koenig <tkoenig@netcologne.de> wrote:
>> Regression tests turned up a few ICEs (now fixed), plus two
>> very invalid test cases, which I think are not worth saving.
> 
> They were added to verify we don't ICE for such invalid testcases.
> How do they fail now?

They failed with an LTO type mistmatch. Instead of deleting them,
I have now added -Wno-lto-type-mismatch to the options in the
committed version.

> I wonder how the frontend handles
> the 2nd call to doesntwork_p8 for
> 
>        program main
>          implicit none
>          character :: c
>          character(len=20) :: res, doesntwork_p8
>          external doesntwork_p8
>          c = 'o'
>          res = doesntwork_p8(c,1,2,3,4,5,6)
>          res = doesntwork_p8(1,2)
>          if (res /= 'foo') stop 3
>        end program main

This is invalid Fortran.  I think we should be able to diagnose this,
but the comitted version does not check it.

Regards

	Thomas

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-18 17:25   ` Segher Boessenkool
@ 2019-02-18 20:16     ` Janne Blomqvist
  2019-02-18 20:30       ` Thomas Koenig
  0 siblings, 1 reply; 14+ messages in thread
From: Janne Blomqvist @ 2019-02-18 20:16 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: Thomas Koenig, fortran, gcc-patches

On Mon, Feb 18, 2019 at 7:25 PM Segher Boessenkool <
segher@kernel.crashing.org> wrote:

> On Mon, Feb 18, 2019 at 10:48:35AM +0200, Janne Blomqvist wrote:
> > I wonder if we shouldn't exorcise all the varargs stuff, it seems to
> > cause more problems than benefits? But not in stage4 if we can avoid
> > it..
>
> On the Power ABIs at least, unprototyped functions (a K&R thing for C) are
> handled the same as varargs (with zero fixed arguments).  How does this
> tie in with Fortran requirements?
>

Varargs don't exist in Fortran.  But we need some kind of support for
so-called "implicit interfaces" (which was the only thing available before
Fortran 90), which I guess are pretty similar to the K&R unprototyped
functions. E.g. something like

subroutine foo
    call bar(1, 2, 3.0)
end subroutine foo

is perfectly valid code, even though discouraged by modern programming
practice. Here the compiler can only deduce from the syntax that bar must
be a subroutine that takes (int, int, float) arguments. And bar can be in
another translation unit, so we have no idea what it's actual interface is,
the onus is on the programmer that they match. Similarly, from

subroutine foo
    f = bar(1, 2)
    print *, f
end subroutine foo

the compiler can deduce that bar is a function that takes (int, int)
arguments, and returns a float (due to implicit typing rules). However, as
previously mentioned in this thread

subroutine foo
    call bar(1, 2)
    f = bar(1, 2)
    print *, f
end subroutine foo

is invalid since bar cannot be both a subroutine and a function. Also,
getting back to my first statement

subroutine foo
    call bar(1, 2)
    call bar(1, 2, 3)
end subroutine foo

is invalid since Fortran doesn't have vararg functions (well, with the
newer "explicit interfaces", optional arguments are possible, but that's
still not the same thing as varargs).

I'm not really sure if there is any good reason why GFortran occasionally
generates these varargs declarations, hence my suggestion to get rid of
them. Unless the middle-end is planning to get rid of untyped function
decls?

-- 
Janne Blomqvist

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-18 20:16     ` Janne Blomqvist
@ 2019-02-18 20:30       ` Thomas Koenig
  2019-02-18 20:37         ` Janne Blomqvist
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Koenig @ 2019-02-18 20:30 UTC (permalink / raw)
  To: Janne Blomqvist, Segher Boessenkool; +Cc: fortran, gcc-patches

Hi Janne,

> I'm not really sure if there is any good reason why GFortran occasionally
> generates these varargs declarations, hence my suggestion to get rid of
> them. Unless the middle-end is planning to get rid of untyped function
> decls?

Are they still being generated after the patch went in?  I'm not sure,
but because I wanted to change as little as possible, I did not try
to change that aspect of the code.

Regards

	Thomas

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-18 20:30       ` Thomas Koenig
@ 2019-02-18 20:37         ` Janne Blomqvist
  0 siblings, 0 replies; 14+ messages in thread
From: Janne Blomqvist @ 2019-02-18 20:37 UTC (permalink / raw)
  To: Thomas Koenig; +Cc: Segher Boessenkool, fortran, gcc-patches

On Mon, Feb 18, 2019 at 10:30 PM Thomas Koenig <tkoenig@netcologne.de>
wrote:

> Hi Janne,
>
> > I'm not really sure if there is any good reason why GFortran occasionally
> > generates these varargs declarations, hence my suggestion to get rid of
> > them. Unless the middle-end is planning to get rid of untyped function
> > decls?
>
> Are they still being generated after the patch went in?


I haven't checked whether your patch fixes all such cases. How do we even
conclusively prove it, except  by just getting rid of that code path? :)


>   I'm not sure,
> but because I wanted to change as little as possible, I did not try
> to change that aspect of the code.
>

I fully agree, this close to the release we shouldn't do any surgery which
isn't absolutely necessary.


-- 
Janne Blomqvist

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
       [not found] ` <b7a9915031f14b3d9ba908109f1fcd21@ap-embx16-sp30.RES.AD.JPL>
@ 2019-02-19 18:06   ` Bob Deen via gcc-patches
  2019-02-19 22:44     ` Thomas Koenig
       [not found]     ` <27d54f8088674492addb78b1dc3067db@ap-embx16-sp30.RES.AD.JPL>
  0 siblings, 2 replies; 14+ messages in thread
From: Bob Deen via gcc-patches @ 2019-02-19 18:06 UTC (permalink / raw)
  To: Janne Blomqvist, Thomas Koenig; +Cc: fortran, gcc-patches, Segher Boessenkool

On 2/18/19 12:48 AM, Janne Blomqvist wrote:
> On Sun, Feb 17, 2019 at 8:19 PM Thomas Koenig <tkoenig@netcologne.de> wrote:
>>
>> Hello world,
>>
>> the attached patch fixes a rather bad ABI violation on POWER systems.
>> ...

> I agree, although we're close to the GCC-9 release, it's better to get
> this in now. So Ok.
> 
> I wonder if we shouldn't exorcise all the varargs stuff, it seems to
> cause more problems than benefits? But not in stage4 if we can avoid
> it..
> 

Please don't.  Some of us still use varargs interfaces (in my case, 
Fortran calling C stdarg subroutines).

Thanks...

-Bob Deen  @  NASA-JPL Multimission Image Processing Lab
Bob.Deen@jpl.nasa.gov

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-19 18:06   ` Bob Deen via gcc-patches
@ 2019-02-19 22:44     ` Thomas Koenig
       [not found]     ` <27d54f8088674492addb78b1dc3067db@ap-embx16-sp30.RES.AD.JPL>
  1 sibling, 0 replies; 14+ messages in thread
From: Thomas Koenig @ 2019-02-19 22:44 UTC (permalink / raw)
  To: Bob Deen, Janne Blomqvist; +Cc: fortran, gcc-patches, Segher Boessenkool

Bob,

> Some of us still use varargs interfaces (in my case, Fortran calling C 
> stdarg subroutines).

The problem for us is that that sometimes using varargs made standard-
conforming Fortran code like, in file a.f

       subroutine foo(a)
       print *,a
       end

and in file main.f

       programme main
       call foo(1.0)
       end

depend ABI details: The call to foo used to be called using
the varargs convention, and the subroutine foo was compiled
as a non-varargs function.

This "worked" until PR 87689 showed that this breaks
standard-conforming Fortran code on a primary gcc platform.

I don't know if that makes a difference for the platform you work
on.  For the System V AMD64 ABI, I suspect it actually might not
matter (at least from glancing at the corresponding Wikipedia
article), but I am _not_ an expert in this field, so please take this
with a chunk of rock salt of appropriate size.

So, we cannot really keep this as a feature (note that varargs
are also not C interoperable).

Regards

	Thomas

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
       [not found]     ` <27d54f8088674492addb78b1dc3067db@ap-embx16-sp30.RES.AD.JPL>
@ 2019-02-26  2:36       ` Bob Deen via gcc-patches
  2019-02-26  7:19         ` Steve Kargl
  0 siblings, 1 reply; 14+ messages in thread
From: Bob Deen via gcc-patches @ 2019-02-26  2:36 UTC (permalink / raw)
  To: Thomas Koenig, Janne Blomqvist; +Cc: fortran, gcc-patches, Segher Boessenkool

On 2/19/19 2:44 PM, Thomas Koenig wrote:
> Bob,
> 
>> Some of us still use varargs interfaces (in my case, Fortran calling C
>> stdarg subroutines).
> 
> The problem for us is that that sometimes using varargs made standard-
> conforming Fortran code like, in file a.f
> 
>         subroutine foo(a)
>         print *,a
>         end
> 
> and in file main.f
> 
>         programme main
>         call foo(1.0)
>         end
> 
> depend ABI details: The call to foo used to be called using
> the varargs convention, and the subroutine foo was compiled
> as a non-varargs function.
> 
> This "worked" until PR 87689 showed that this breaks
> standard-conforming Fortran code on a primary gcc platform.
> 
> I don't know if that makes a difference for the platform you work
> on.  For the System V AMD64 ABI, I suspect it actually might not
> matter (at least from glancing at the corresponding Wikipedia
> article), but I am _not_ an expert in this field, so please take this
> with a chunk of rock salt of appropriate size.
> 
> So, we cannot really keep this as a feature (note that varargs
> are also not C interoperable).

Okay.  But I hope you don't let the perfect be the enemy of the good. 
That particular platform is not a concern for us, so if you break it 
there I'm not happy, but it's not the end of the world either.  But 
please don't break it other places just because it doesn't work on that 
one platform.  I know it's not good design practice to have that kind of 
platform-dependent behavior, but sometimes practicalities force less 
than ideal choices.

Thanks...

-Bob

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER
  2019-02-26  2:36       ` Bob Deen via gcc-patches
@ 2019-02-26  7:19         ` Steve Kargl
  0 siblings, 0 replies; 14+ messages in thread
From: Steve Kargl @ 2019-02-26  7:19 UTC (permalink / raw)
  To: Bob Deen via fortran
  Cc: Thomas Koenig, Janne Blomqvist, gcc-patches, Segher Boessenkool

On Mon, Feb 25, 2019 at 05:54:44PM -0800, Bob Deen via fortran wrote:
> On 2/19/19 2:44 PM, Thomas Koenig wrote:
> > Bob,
> > 
> >> Some of us still use varargs interfaces (in my case, Fortran calling C
> >> stdarg subroutines).
> > 
> > The problem for us is that that sometimes using varargs made standard-
> > conforming Fortran code like, in file a.f
> > 
> >         subroutine foo(a)
> >         print *,a
> >         end
> > 
> > and in file main.f
> > 
> >         programme main
> >         call foo(1.0)
> >         end
> > 
> > depend ABI details: The call to foo used to be called using
> > the varargs convention, and the subroutine foo was compiled
> > as a non-varargs function.
> > 
> > This "worked" until PR 87689 showed that this breaks
> > standard-conforming Fortran code on a primary gcc platform.
> > 
> > I don't know if that makes a difference for the platform you work
> > on.  For the System V AMD64 ABI, I suspect it actually might not
> > matter (at least from glancing at the corresponding Wikipedia
> > article), but I am _not_ an expert in this field, so please take this
> > with a chunk of rock salt of appropriate size.
> > 
> > So, we cannot really keep this as a feature (note that varargs
> > are also not C interoperable).
> 
> Okay.  But I hope you don't let the perfect be the enemy of the good. 
> That particular platform is not a concern for us, so if you break it 
> there I'm not happy, but it's not the end of the world either.  But 
> please don't break it other places just because it doesn't work on that 
> one platform.  I know it's not good design practice to have that kind of 
> platform-dependent behavior, but sometimes practicalities force less 
> than ideal choices.
> 

Instead of fixing a bug in the compiler and conforming
to the Fortran standard, we should just let this one go
as an "enhancement".  How many more of these "enhancements"
should be allowed?  If an "enhancement" works on one
architecture but not on another, we'll need to start adding
pre-processing directives into otherwise machine-independent
code.  It becomes a maintainence nightmare.  We have these
maintainence issues in libgfortran, where we do have to
take the architecture into consider.  Keeping things working 
for everyone can be (ahem) fun.

If you need a vararg-like interface, it seems you should
consider encapulating the specific routines in a module
and build a generic interface with optional arguments.

-- 
Steve

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-02-26  2:16 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-17 18:19 [patch, fortran] Fix PR 87689, wrong decls / ABI violation on POWER Thomas Koenig
2019-02-17 22:46 ` Alan Modra
2019-02-18  8:49 ` Janne Blomqvist
2019-02-18 17:25   ` Segher Boessenkool
2019-02-18 20:16     ` Janne Blomqvist
2019-02-18 20:30       ` Thomas Koenig
2019-02-18 20:37         ` Janne Blomqvist
2019-02-18 12:51 ` Richard Biener
2019-02-18 13:08   ` Janne Blomqvist
2019-02-18 18:35   ` Thomas Koenig
     [not found] ` <b7a9915031f14b3d9ba908109f1fcd21@ap-embx16-sp30.RES.AD.JPL>
2019-02-19 18:06   ` Bob Deen via gcc-patches
2019-02-19 22:44     ` Thomas Koenig
     [not found]     ` <27d54f8088674492addb78b1dc3067db@ap-embx16-sp30.RES.AD.JPL>
2019-02-26  2:36       ` Bob Deen via gcc-patches
2019-02-26  7:19         ` Steve Kargl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).