| Message ID | a7bb3f2d-5c32-9fc3-8ece-d61794e64ab3@netcologne.de |
|---|---|
| State | New |
| Headers | show |
| Series | [fortran] Fix PR 87689, wrong decls / ABI violation on POWER | expand |
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!
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..
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.
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.
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
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
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?
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
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.
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
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
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
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.
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 *);
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.