Bug 12081 - Gcc can't be compiled with -mregparm=3
Summary: Gcc can't be compiled with -mregparm=3
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: other (show other bugs)
Version: 3.3.1
: P2 normal
Target Milestone: ---
Assignee: Rask Ingemann Lambertsen
URL:
Keywords: build
: 18193 (view as bug list)
Depends on:
Blocks:
 
Reported: 2003-08-27 18:28 UTC by mikulas
Modified: 2013-08-29 18:40 UTC (History)
7 users (show)

See Also:
Host: i586-pc-linux-gnu
Target: i586-pc-linux-gnu
Build: i586-pc-linux-gnu
Known to work:
Known to fail: 4.3.0
Last reconfirmed: 2005-04-30 15:55:07


Attachments
possible patch (2.21 KB, patch)
2007-10-27 23:16 UTC, Rask Ingemann Lambertsen
Details | Diff
patch v2, i386 fix added (2.78 KB, patch)
2007-10-28 11:14 UTC, Rask Ingemann Lambertsen
Details | Diff
patch v3, varargs free (10.75 KB, patch)
2007-10-30 00:14 UTC, Rask Ingemann Lambertsen
Details | Diff
Updated version of Rask Ingemann Lambertsen's patch (9.27 KB, patch)
2013-05-25 02:28 UTC, Stefan Kristiansson
Details | Diff
fix up conversion in tilepro_expand_builtin, delete unused 11-arity stuff (665 bytes, patch)
2013-05-25 08:18 UTC, Mikael Pettersson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mikulas 2003-08-27 18:28:50 UTC
When GCC is compiled with -mregparm=3 (system libraries are compiled with that flag too), it doesn't work. The problem is this line (maybe there are more buggy declarations, but this is the first one I hit):

typedef rtx (*insn_gen_fn) PARAMS ((rtx, ...));

Later, two-parameter functions are retyped to insn_gen_fn and called. The caller pushes arguments to stack, the calee expects them in registers.
Comment 1 Dara Hazeghi 2004-01-17 23:53:23 UTC
Sorry that this report seems to have slipped through the cracks. What exactly do you mean by 
"doesn't work"? Does it produce buggy code, or just crash, or what? Also, I don't suppose you 
could come up with a small, self-contained testcase? Thanks.
Comment 2 mikulas 2004-01-18 01:39:07 UTC
Gcc is code written the wrong way

Let's have these declarations:

void f(int a, int b);
void (*g)(int a, ...);

and this code:

g = (void (*)(int, ...))f;
f(1,2);

--- the code is obviously wrong in C and might not work. It works on most
architectures, but it doesn't on x86 with regparm 3 or -mrtd.

Similar code exists in gcc in insn-output.c (there is:
"(insn_gen_fn)gen_cmpqi_ext_3_insn" and many more like this).

I "fixed" it for myself by adding attribute regparm(0),cdecl to these
functions, but it is not correct fix. Correct fix would be to not use these
kind of casts.

I can't show you how to reproduce the bug, because you don't have environment
where regparm(3) is default calling convention. I do have.
Comment 3 mikulas 2004-01-18 01:42:01 UTC
In example I wrote there should be
g(1,2)
instead of
f(1,2)

f(1,2) is correct, but g(1,2) will crash with -mrtd or pass invalid arguments
with -mregparm=3.
Comment 4 Dara Hazeghi 2004-01-18 01:54:30 UTC
I don't necessarily disagree. However without a testcase, the chances of somebody looking into/
fixing this aren't too good (I certainly don't have the expertise).
Comment 5 Andrew Pinski 2004-10-28 12:09:06 UTC
*** Bug 18193 has been marked as a duplicate of this bug. ***
Comment 6 Samuel Tardieu 2007-10-27 18:32:00 UTC
Here is a small test case for this program. Compile with -mregparm=3 (or any value greater than 0 on x86).

#include <stdio.h>

typedef void (*func) (int a, ...);

void f (int a, int b)
{
   printf ("a = %d, b = %d\n", a, b);
}

int main ()
{
   func g = (void *) f;
   g (1, 2);
   return 0;
}

The execution on my machine prints:

a = 134513524, b = -1078190752

instead of the expected

a = 1, b = 2

obtained when using -mregparm=0.

The call to "f" (through variable "g") is generated as-is:

        movl    $2, 4(%esp)
        movl    $1, (%esp)
        call    f

instead of using registers.
Comment 7 Rask Ingemann Lambertsen 2007-10-27 23:16:56 UTC
Created attachment 14417 [details]
possible patch

Please give this patch a try. I need it to build GCC with OpenWatcom, which wants parameters on the stack by default.
Comment 8 pinskia@gmail.com 2007-10-27 23:22:09 UTC
Subject: Re:  Gcc can't be compiled with -mregparm=3

On 27 Oct 2007 23:16:57 -0000, rask at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
> Please give this patch a try. I need it to build GCC with OpenWatcom, which
> wants parameters on the stack by default.

I think this makes some worse code in some cases.  Var-args is very
bad for code generation on targets where argments are passed via
registers (almost all RISC and IA64).

-- Pinski
Comment 9 Rask Ingemann Lambertsen 2007-10-27 23:36:20 UTC
It happens to work because all the compilers people use to build GCC pass varargs the same way as non-varargs, at least for the number of arguments received by the gen_* functions. IOW you shouldn't see any speed difference if it works for you already.
Comment 10 Rask Ingemann Lambertsen 2007-10-28 11:14:37 UTC
Created attachment 14419 [details]
patch v2, i386 fix added
Comment 11 pinskia@gmail.com 2007-10-28 11:57:28 UTC
Subject: Re:  Gcc can't be compiled with -mregparm=3

On 28 Oct 2007 11:14:37 -0000, rask at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:

How many times do I have to say this is bad for most RISC targets (hosts)?

Really if the type being is used is wrong, they should be changed
rather than changing to use var-args.

-- Pinski
Comment 12 Rask Ingemann Lambertsen 2007-10-28 14:00:20 UTC
In reply to comment #11:
> How many times do I have to say this is bad for most RISC targets (hosts)?

I don't particularily care how many times you say it. Show some code (which
works) and/or show some timings (of code that works).

> Really if the type being is used is wrong, they should be changed
> rather than changing to use var-args.

Will you please look at the code? You have gen_movsi() with 2 arguments and
gen_addsi3() with 3 arguments. Given that they are put into a table as a
function of type insn_gen_fn and called as such through the table (see the
GEN_FCN() macro in optabs.h and the table definition in recog.h), the
function definition has to match. If it doesn't, all bets are off as to what
arguments the function receives. We have gen_* functions taking 0, 1, 2, 3, ...
arguments and with GCC being designed the way it is, they need to be
prototyped and defined with the same arguments.

You are most definitely welcome to post some non-varargs code which works.
You are most definitely also welcome to time it against my patches.
Comment 13 Rask Ingemann Lambertsen 2007-10-28 14:02:53 UTC
In reply to comment #7:

> I need it to build GCC with OpenWatcom, which wants parameters on the stack by default.

Er, that's in registers by default, of course.
Comment 14 Samuel Tardieu 2007-10-28 14:42:40 UTC
Rask,

is your patch supposed to help with testcase presented in comment #6? It looks like I still get "f" waiting (as expected) for arguments in registers when I use -mregparm=3 while the call through g still uses the stack:

main:
[...]
        subl    $8, %esp
        movl    $2, 4(%esp)
        movl    $1, (%esp)
        call    f

(GCC at SVN/trunk 129695 + your patch)
Comment 15 Rask Ingemann Lambertsen 2007-10-28 15:54:24 UTC
In reply to comment #14:
>  is your patch supposed to help with testcase presented in comment #6?

No, it's aimed at the problem from the description. That is, GCC itself doesn't work if compiled with a compiler where f(int a, int b) is called differently than f(int a, ...). I don't think your testcase is actually supposed to work. Don't you get a warning if you take out (void *) and compile with -Wall?
As pinpointed in comment #2, GCC contains the same incorrect type cast. I actually forgot to patch this part:

Index: gcc/genoutput.c
===================================================================
--- gcc/genoutput.c	(revision 129503)
+++ gcc/genoutput.c	(working copy)
@@ -386,7 +386,7 @@ output_insn_data (void)
 	}
 
       if (d->name && d->name[0] != '*')
-	printf ("    (insn_gen_fn) gen_%s,\n", d->name);
+	printf ("    gen_%s,\n", d->name);
       else
 	printf ("    0,\n");
 
Comment 16 mikulas 2007-10-28 16:09:15 UTC
Subject: Re:  Gcc can't be compiled with -mregparm=3

> arguments the function receives. We have gen_* functions taking 0, 1, 2, 3, ...
> arguments and with GCC being designed the way it is, they need to be
> prototyped and defined with the same arguments.
> 
> You are most definitely welcome to post some non-varargs code which works.
> You are most definitely also welcome to time it against my patches.

You can cast them at the time of calling and store them as void * in the 
table --- that is standard-compliant.

Or you can define
union gen_function {
	rtx (*one_arg)(rtx);
	rtx (*two_args)(rtx, rtx);
	rtx (*three_args)(rtx, rtx, rtx);
	... etc;
};

--- this will be correct C without the performance impact of varargs.

Mikulas
Comment 17 Rask Ingemann Lambertsen 2007-10-30 00:14:20 UTC
Created attachment 14438 [details]
patch v3, varargs free

In reply to comment #16:
> You can cast them at the time of calling and store them as void * in the
> table --- that is standard-compliant.

Casting is what caused the problem to go unnoticed in the first place. This version will cause complaints from the compiler if there is a mixup.
Comment 18 pinskia@gmail.com 2007-10-30 00:20:46 UTC
Subject: Re:  Gcc can't be compiled with -mregparm=3

On 30 Oct 2007 00:14:21 -0000, rask at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #17 from rask at gcc dot gnu dot org  2007-10-30 00:14 -------
> Created an attachment (id=14438)
>  --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14438&action=view)
> patch v3, varargs free

This version is much better.

Thanks,
Andrew Pinski
Comment 19 Stefan Kristiansson 2013-05-25 02:28:25 UTC
Created attachment 30186 [details]
Updated version of Rask Ingemann Lambertsen's patch

I'm bumping this bug to make people aware that it still exists and that people are affected by it.
On OpenRISC (not in main tree arch), the ABI states that var args are always passed on the stack, thus making it impossible to build a working native compiler
when this bug is present.

Attached is an updated version of the patch posted by Rask Ingemann Lambertsen,
bringing it up to date with the current svn.
Comment 20 Mikael Pettersson 2013-05-25 08:18:13 UTC
Created attachment 30188 [details]
fix up conversion in tilepro_expand_builtin, delete unused 11-arity stuff

Looking through the patch I noticed that the conversion in tilepro_expand_builtin was broken (icode was lost).  Grepping around I also noticed that nothing used GEN_FCN11 (or is that used by the out-of-tree OpenRISC port?)  This add-on fixes those two issues.
Comment 21 Mikael Pettersson 2013-05-25 09:03:04 UTC
(In reply to Mikael Pettersson from comment #20)
> Grepping around I also
> noticed that nothing used GEN_FCN11 (or is that used by the out-of-tree
> OpenRISC port?)  This add-on fixes those two issues.

i386/sync.md appears to generate code that needs the 11-arity version; consider that part of the fixup patch revoked.
Comment 22 Mikael Pettersson 2013-06-03 08:30:08 UTC
FWIW, the updated patch for gcc 4.9 bootstraps and regtests cleanly on several hosts (x86_64, sparc64, powerpc64, armv5tel, m68k).
Comment 23 Uroš Bizjak 2013-06-05 18:39:41 UTC
(In reply to Mikael Pettersson from comment #22)
> FWIW, the updated patch for gcc 4.9 bootstraps and regtests cleanly on
> several hosts (x86_64, sparc64, powerpc64, armv5tel, m68k).

Please post the patch to gcc-patches@ mailing list for a review.
Comment 24 Stefan Kristiansson 2013-07-10 02:11:54 UTC
(In reply to Uroš Bizjak from comment #23)
> (In reply to Mikael Pettersson from comment #22)
> > FWIW, the updated patch for gcc 4.9 bootstraps and regtests cleanly on
> > several hosts (x86_64, sparc64, powerpc64, armv5tel, m68k).
> 
> Please post the patch to gcc-patches@ mailing list for a review.

Done: http://gcc.gnu.org/ml/gcc-patches/2013-07/msg00400.html
Comment 25 Oleg Endo 2013-07-27 12:55:29 UTC
I've proposed a simpler patch which doesn't require fixing up backend code etc.

http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01315.html
Comment 26 Oleg Endo 2013-08-05 22:15:28 UTC
I don't think this is a 'target' issue, changed to 'other'.

I've just committed the following, which should fix the issue for 4.9.

http://gcc.gnu.org/viewcvs/gcc?view=revision&revision=201513

	PR other/12081
	* recog.h (rtx (*insn_gen_fn) (rtx, ...)): Replace typedef with	new
	class insn_gen_fn.
	* expr.c (move_by_pieces_1, store_by_pieces_2): Replace argument
	rtx (*) (rtx, ...) with insn_gen_fn.
	* genoutput.c (output_insn_data): Cast gen_? function pointers to
	insn_gen_fn::stored_funcptr.  Add initializer braces.


Since 4.8 also uses C++ it could be backported.
Comment 27 Michael Meissner 2013-08-07 18:39:25 UTC
The patch from Oleg Endo breaks the PowerPC build.

.../gcc/config/rs6000/rs6000.c: In function ‘void rs6000_emit_swdiv(rtx_def*, rtx_def*, rtx_def*, bool)’:
.../gcc/config/rs6000/rs6000.c:28142: error: invalid cast from type ‘const insn_gen_fn’ to type ‘rtx_def* (*)(rtx_def*, rtx_def*, rtx_def*)’
.../gcc/config/rs6000/rs6000.c: In function ‘void rs6000_emit_swrsqrt(rtx_def*, rtx_def*)’:
.../gcc/config/rs6000/rs6000.c:28220: error: invalid cast from type ‘const insn_gen_fn’ to type ‘rtx_def* (*)(rtx_def*, rtx_def*, rtx_def*)’
Comment 28 Uroš Bizjak 2013-08-07 18:57:40 UTC
(In reply to Michael Meissner from comment #27)
> The patch from Oleg Endo breaks the PowerPC build.
> 
> .../gcc/config/rs6000/rs6000.c: In function ‘void
> rs6000_emit_swdiv(rtx_def*, rtx_def*, rtx_def*, bool)’:
> .../gcc/config/rs6000/rs6000.c:28142: error: invalid cast from type ‘const
> insn_gen_fn’ to type ‘rtx_def* (*)(rtx_def*, rtx_def*, rtx_def*)’
> .../gcc/config/rs6000/rs6000.c: In function ‘void
> rs6000_emit_swrsqrt(rtx_def*, rtx_def*)’:
> .../gcc/config/rs6000/rs6000.c:28220: error: invalid cast from type ‘const
> insn_gen_fn’ to type ‘rtx_def* (*)(rtx_def*, rtx_def*, rtx_def*)’

Patch at [1].

[1] http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00323.html
Comment 29 Oleg Endo 2013-08-07 19:22:32 UTC
(In reply to Uroš Bizjak from comment #28)
> (In reply to Michael Meissner from comment #27)
> > The patch from Oleg Endo breaks the PowerPC build.
> > 
> > .../gcc/config/rs6000/rs6000.c: In function ‘void
> > rs6000_emit_swdiv(rtx_def*, rtx_def*, rtx_def*, bool)’:
> > .../gcc/config/rs6000/rs6000.c:28142: error: invalid cast from type ‘const
> > insn_gen_fn’ to type ‘rtx_def* (*)(rtx_def*, rtx_def*, rtx_def*)’
> > .../gcc/config/rs6000/rs6000.c: In function ‘void
> > rs6000_emit_swrsqrt(rtx_def*, rtx_def*)’:
> > .../gcc/config/rs6000/rs6000.c:28220: error: invalid cast from type ‘const
> > insn_gen_fn’ to type ‘rtx_def* (*)(rtx_def*, rtx_def*, rtx_def*)’
> 
> Patch at [1].
> 
> [1] http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00323.html

Fixed on trunk.

http://gcc.gnu.org/viewcvs/gcc?view=revision&revision=201580

	PR other/12081
	config/rs6000/rs6000.c (gen_2arg_fn_t): Remove typedef.
	(rs6000_emit_swdiv, rs6000_emit_swrsqrt): Don't cast result of GEN_FCN
	to gen_2arg_fn_t.
Comment 30 Michael Meissner 2013-08-07 19:26:52 UTC
On Wed, Aug 07, 2013 at 07:22:32PM +0000, olegendo at gcc dot gnu.org wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12081
> 
> --- Comment #29 from Oleg Endo <olegendo at gcc dot gnu.org> ---
> (In reply to Uroš Bizjak from comment #28)
> > (In reply to Michael Meissner from comment #27)
> > > The patch from Oleg Endo breaks the PowerPC build.
> > > 
> > > .../gcc/config/rs6000/rs6000.c: In function ‘void
> > > rs6000_emit_swdiv(rtx_def*, rtx_def*, rtx_def*, bool)’:
> > > .../gcc/config/rs6000/rs6000.c:28142: error: invalid cast from type ‘const
> > > insn_gen_fn’ to type ‘rtx_def* (*)(rtx_def*, rtx_def*, rtx_def*)’
> > > .../gcc/config/rs6000/rs6000.c: In function ‘void
> > > rs6000_emit_swrsqrt(rtx_def*, rtx_def*)’:
> > > .../gcc/config/rs6000/rs6000.c:28220: error: invalid cast from type ‘const
> > > insn_gen_fn’ to type ‘rtx_def* (*)(rtx_def*, rtx_def*, rtx_def*)’
> > 
> > Patch at [1].
> > 
> > [1] http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00323.html
> 
> Fixed on trunk.
> 
> http://gcc.gnu.org/viewcvs/gcc?view=revision&revision=201580
> 
>     PR other/12081
>     config/rs6000/rs6000.c (gen_2arg_fn_t): Remove typedef.
>     (rs6000_emit_swdiv, rs6000_emit_swrsqrt): Don't cast result of GEN_FCN
>     to gen_2arg_fn_t.

Great, thanks.  I was just starting the build process to verify it.
Comment 31 Oleg Endo 2013-08-29 18:37:48 UTC
Author: olegendo
Date: Thu Aug 29 18:37:46 2013
New Revision: 202083

URL: http://gcc.gnu.org/viewcvs?rev=202083&root=gcc&view=rev
Log:
	Backport from mainline
	2013-08-05  Oleg Endo  <olegendo@gcc.gnu.org>

	PR other/12081
	* recog.h (rtx (*insn_gen_fn) (rtx, ...)): Replace typedef with	new
	class insn_gen_fn.
	* expr.c (move_by_pieces_1, store_by_pieces_2): Replace argument
	rtx (*) (rtx, ...) with insn_gen_fn.
	* genoutput.c (output_insn_data): Cast gen_? function pointers to
	insn_gen_fn::stored_funcptr.  Add initializer braces.

	Backport from mainline
	2013-08-07  Oleg Endo  <olegendo@gcc.gnu.org>

	PR other/12081
	* config/rs6000/rs6000.c (gen_2arg_fn_t): Remove typedef.
	(rs6000_emit_swdiv_high_precision, rs6000_emit_swdiv_low_precision,
	rs6000_emit_swrsqrt): Don't cast result of GEN_FCN to gen_2arg_fn_t.


Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/config/rs6000/rs6000.c
    branches/gcc-4_8-branch/gcc/expr.c
    branches/gcc-4_8-branch/gcc/genoutput.c
    branches/gcc-4_8-branch/gcc/recog.h
Comment 32 Oleg Endo 2013-08-29 18:40:37 UTC
Fixed on 4.8 and 4.9.  Won't backport to 4.7 since it doesn't use C++.