Bug 61300 - powerpc64le miscompile with K&R-style function definition at -O0
Summary: powerpc64le miscompile with K&R-style function definition at -O0
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.9.1
: P3 normal
Target Milestone: ---
Assignee: Alan Modra
URL: https://gcc.gnu.org/ml/gcc-patches/20...
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-24 02:33 UTC by Brooks Moses
Modified: 2014-06-11 23:57 UTC (History)
6 users (show)

See Also:
Host:
Target: powerpc64le-*-*
Build:
Known to work:
Known to fail:
Last reconfirmed: 2014-05-24 00:00:00


Attachments
quick and dirty fix (1.19 KB, patch)
2014-05-26 02:37 UTC, Alan Modra
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brooks Moses 2014-05-24 02:33:44 UTC
We've run into a GCC miscompile problem with K&R-style function definitions that's causing some various old GNU software to fail in some cases.

A simple test case:

  $ cat bb.c
  struct builtin { char *name; };

  int compare_kr (sbp1, sbp2)
      struct builtin *sbp1, *sbp2;
  {
    return  sbp1->name[0] - sbp2->name[0];
  }

  int compare_ansi (struct builtin *sbp1, struct builtin *sbp2)
  {
    return  sbp1->name[0] - sbp2->name[0];
  }

On either our Google branch of GCC 4.9 or the Ubuntu system GCC 4.8 on our test machine, we get something like this (annotated to show the difference):

  $ powerpc64le-unknown-linux-gnu-gcc -S -o- bb.c
       .file   "bb.c"
       .abiversion 2
       .section        ".toc","aw"
       .section        ".text"
       .align 2
       .globl compare_kr
       .type   compare_kr, @function
  compare_kr:
       std 31,-8(1)
       stdu 1,-48(1)
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ it allocates 48 bytes for the frame
       mr 31,1
       std 3,80(31)
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ and then writes way past this
       std 4,88(31)
       ld 9,80(31)
       ...
       .globl compare_ansi
       .type   compare_ansi, @function
  compare_ansi:
       std 31,-8(1)
       stdu 1,-64(1)
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ allocates 64 bytes
       mr 31,1
       std 3,40(31)
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ and saves within the confines of the frame
       std 4,32(31)
       ld 9,40(31)
       ...

In this testcase, this seems to go away if we turn on -O2.  I'm not sure if that's always the case or not.

I've also confirmed that this does _not_ happen with the x86_64 compiler built from the same source branch.
Comment 1 Alan Modra 2014-05-26 00:13:57 UTC
So, the "writes way past this" is writing into the parameter save area.  

compare_kr is assuming that it was called with a parameter save area because it isn't prototyped, but that is quite wrong because when compiling the function body we don't know how the function was called.  A call may well have a prototype in scope, and thus not set up a parameter save area.

It's a bug in rs6000_function_parms_need_stack.  This function can't blindly test !prototype_p (fun).
Comment 2 Alan Modra 2014-05-26 02:37:24 UTC
Created attachment 32854 [details]
quick and dirty fix

This fixes the problem in a fairly obvious way, but I think we can use a more refined approach that doesn't need a new INCOMING_REG_PARM_STACK_SPACE..
Comment 3 Richard Biener 2014-05-28 11:25:47 UTC
(In reply to Alan Modra from comment #1)
> So, the "writes way past this" is writing into the parameter save area.  
> 
> compare_kr is assuming that it was called with a parameter save area because
> it isn't prototyped, but that is quite wrong because when compiling the
> function body we don't know how the function was called.  A call may well
> have a prototype in scope, and thus not set up a parameter save area.
> 
> It's a bug in rs6000_function_parms_need_stack.  This function can't blindly
> test !prototype_p (fun).

So you can simply drop that check instead as it seems to be an optimization
only?  Thus

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c  (revision 211011)
+++ gcc/config/rs6000/rs6000.c  (working copy)
@@ -10492,7 +10492,7 @@ rs6000_function_parms_need_stack (tree f
     fun = TREE_TYPE (fun);
 
   /* Varargs functions need the parameter save area.  */
-  if (!prototype_p (fun) || stdarg_p (fun))
+  if (stdarg_p (fun))
     return true;
 
   INIT_CUMULATIVE_INCOMING_ARGS (args_so_far_v, fun, NULL_RTX);

?
Comment 4 Alan Modra 2014-05-28 12:30:34 UTC
You could do that, but smaller stack frames is one of the nice features of the ELFv2 ABI!  What I called the "quick and dirty" fix seems to be the way to go, as the scheme I had in mind to avoid a new INCOMING_REG_PARM_STACK_SPACE target macro only works with very old versions of gcc..  See
https://gcc.gnu.org/ml/gcc/2014-05/msg00302.html
Comment 5 Alan Modra 2014-05-28 12:52:11 UTC
Actually, to work the patch in comment #3 would need to be

-  if (!prototype_p (fun) || stdarg_p (fun))
+  if (1)
     return true;
Comment 6 Ulrich Weigand 2014-06-02 14:42:17 UTC
Note that either of the proposed changes (comment #3 or comment #5) would result in an incompatible ABI change, since other compilers already implement the parameter save area rules as defined by the ELFv2 ABI.

The basic rule is: A function body may expect the parameter save area to have been provided on its callers stack *iff* the function has either some argument that is passed in memory according to calling convention rules, or the function has a variable argument list.

This is the only correct rule to be used when generating code for a function body, no matter whether the function definition uses K&R style or not, or whether there is a prototype in scope at the definition site or not.


Now, when generating a function *call*, we of course may follow the same rule, which we can do if we have a prototype in scope, or else we may opt to always provide the parameter save area (which is the only safe option if there is *no* prototype in scope).


The problem is that the same macro REG_PARM_STACK_SPACE is currently invoked for both function calls and function definitions, so the test for prototype_p is wrong if we're currently compiling a function definition.  Is there a way to inspect the REG_PARM_STACK_SPACE argument to distinguish those cases?
Comment 7 Alan Modra 2014-06-03 00:56:47 UTC
No, I don't believe there is a convenient way we can look at the REG_PARM_STACK_SPACE argument to determine whether it was used for a call or for a function body.  (I did think it might be possible, hence my comment #2, but that was remembering the situation from way back before TYPE_ACTUAL_ARG_TYPES became private to C lang tree nodes.)

function.c (function body case) always passes the function decl as the argument.  calls.c (call case) sometimes passes a function decl, and sometimes a function type.  If calls.c always passed a type it would be easy..  A cursory glance over all target defines of REG_PARM_STACK_SPACE says that it should be possible to change calls.c to do this.  Is that a better design change than defining INCOMING_REG_PARM_STACK_SPACE?  I don't think so..
Comment 8 Alan Modra 2014-06-06 01:04:56 UTC
Author: amodra
Date: Fri Jun  6 01:04:22 2014
New Revision: 211296

URL: http://gcc.gnu.org/viewcvs?rev=211296&root=gcc&view=rev
Log:
	PR target/61300
	* doc/tm.texi.in (INCOMING_REG_PARM_STACK_SPACE): Document.
	* doc/tm.texi: Regenerate.
	* function.c (INCOMING_REG_PARM_STACK_SPACE): Provide default.
	Use throughout in place of REG_PARM_STACK_SPACE.
	* config/rs6000/rs6000.c (rs6000_reg_parm_stack_space): Add
	"incoming" param.  Pass to rs6000_function_parms_need_stack.
	(rs6000_function_parms_need_stack): Add "incoming" param, ignore
	prototype_p when incoming.  Use function decl when incoming
	to handle K&R style functions.
	* config/rs6000/rs6000.h (REG_PARM_STACK_SPACE): Adjust.
	(INCOMING_REG_PARM_STACK_SPACE): Define.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/rs6000/rs6000-protos.h
    trunk/gcc/config/rs6000/rs6000.c
    trunk/gcc/config/rs6000/rs6000.h
    trunk/gcc/doc/tm.texi
    trunk/gcc/doc/tm.texi.in
    trunk/gcc/function.c
Comment 9 Alan Modra 2014-06-11 23:50:22 UTC
Author: amodra
Date: Wed Jun 11 23:49:49 2014
New Revision: 211482

URL: http://gcc.gnu.org/viewcvs?rev=211482&root=gcc&view=rev
Log:
	PR target/61300
	* doc/tm.texi.in (INCOMING_REG_PARM_STACK_SPACE): Document.
	* doc/tm.texi: Regenerate.
	* function.c (INCOMING_REG_PARM_STACK_SPACE): Provide default.
	Use throughout in place of REG_PARM_STACK_SPACE.
	* config/rs6000/rs6000.c (rs6000_reg_parm_stack_space): Add
	"incoming" param.  Pass to rs6000_function_parms_need_stack.
	(rs6000_function_parms_need_stack): Add "incoming" param, ignore
	prototype_p when incoming.  Use function decl when incoming
	to handle K&R style functions.
	* config/rs6000/rs6000.h (REG_PARM_STACK_SPACE): Adjust.
	(INCOMING_REG_PARM_STACK_SPACE): Define.


Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/config/rs6000/rs6000-protos.h
    branches/gcc-4_9-branch/gcc/config/rs6000/rs6000.c
    branches/gcc-4_9-branch/gcc/config/rs6000/rs6000.h
    branches/gcc-4_9-branch/gcc/doc/tm.texi
    branches/gcc-4_9-branch/gcc/doc/tm.texi.in
    branches/gcc-4_9-branch/gcc/function.c
Comment 10 Alan Modra 2014-06-11 23:50:48 UTC
Author: amodra
Date: Wed Jun 11 23:50:16 2014
New Revision: 211483

URL: http://gcc.gnu.org/viewcvs?rev=211483&root=gcc&view=rev
Log:
	PR target/61300
	* doc/tm.texi.in (INCOMING_REG_PARM_STACK_SPACE): Document.
	* doc/tm.texi: Regenerate.
	* function.c (INCOMING_REG_PARM_STACK_SPACE): Provide default.
	Use throughout in place of REG_PARM_STACK_SPACE.
	* config/rs6000/rs6000.c (rs6000_reg_parm_stack_space): Add
	"incoming" param.  Pass to rs6000_function_parms_need_stack.
	(rs6000_function_parms_need_stack): Add "incoming" param, ignore
	prototype_p when incoming.  Use function decl when incoming
	to handle K&R style functions.
	* config/rs6000/rs6000.h (REG_PARM_STACK_SPACE): Adjust.
	(INCOMING_REG_PARM_STACK_SPACE): Define.


Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/config/rs6000/rs6000-protos.h
    branches/gcc-4_8-branch/gcc/config/rs6000/rs6000.c
    branches/gcc-4_8-branch/gcc/config/rs6000/rs6000.h
    branches/gcc-4_8-branch/gcc/doc/tm.texi
    branches/gcc-4_8-branch/gcc/doc/tm.texi.in
    branches/gcc-4_8-branch/gcc/function.c
Comment 11 Alan Modra 2014-06-11 23:57:45 UTC
Fixed