[RFT,i386] : Fix PR87928, ICE in ix86_compute_frame_layout

Message ID CAFULd4ZWvawXdSSupHC5ih=NRr+dsy_GEF_eYLbJGVE2L6qTHw@mail.gmail.com
State New
Headers show
Series
  • [RFT,i386] : Fix PR87928, ICE in ix86_compute_frame_layout
Related show

Commit Message

Uros Bizjak Nov. 9, 2018, 7:53 a.m.
Hello!

Attached patch fixes PR87928, where we ICE in ix86_compute_frame_layout in

  gcc_assert (preferred_alignment >= STACK_BOUNDARY / BITS_PER_UNIT);

when __attribute__ ((sysv_abi) is used. When the testcase is compiled,
ix86_cfun_abi () returns SYSV_ABI due to function __attribute__
override, while ix86_abi returns MS_ABI. These ABIs have different
STACK_BOUNDARY definition, so we compare preferred_alignment with
wrong STACK boundary definition.

Attached patch makes STACK_BOUNDARY dependant on function ABI attribute.

2018-11-09  Uros Bizjak  <ubizjak@gmail.com>

    PR target/87928
    * config/i386/i386.h (STACK_BOUNDARY): Use TARGET_64BIT_MS_ABI
    instead of (TARGET_64BIT && ix86_abi == MS_ABI).
    * config/i386/darwin.h (STACK_BOUNDARY): Ditto.
    * config/i386/cygming.h (STACK_BOUNDARY): Remove.

testsuite /Changelog:

2018-11-09  Uros Bizjak  <ubizjak@gmail.com>

    PR target/87928
    * gcc.target/i386/pr87928.c: New test.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

I will commit the patch to mainline SVN early next week to allow
Darwin and Ming/Cygwin maintainers some time to test the patch on
their targets.

Uros.

Comments

Iain Sandoe Nov. 9, 2018, 6:37 p.m. | #1
Hi Uros,

> On 8 Nov 2018, at 23:53, Uros Bizjak <ubizjak@gmail.com> wrote:
> 

> Attached patch fixes PR87928, where we ICE in ix86_compute_frame_layout in


> I will commit the patch to mainline SVN early next week to allow
> Darwin and Ming/Cygwin maintainers some time to test the patch on
> their targets.

Bootstrap succeeds and the new test passes for Darwin.

This does not alter that Darwin has breakage in this area (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78444)
see : https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00884.html and HJ’s reply.

Essentially, determining any case that demands anything other than 128b alignment on Darwin looks hard to reason about***

(I will try to get to more investigation there when time permits)

Iain

*** Quotes from "Mac OS X ABI Function Call Guide”

m32:

The function calling conventions used in the IA-32 environment are the same as those used in the System V IA-32 ABI, with the following exceptions: 

   …

	• The stack is 16-byte aligned at the point of function calls
    …

m64:

Calls out to the x86_64 psABI
Uros Bizjak Nov. 11, 2018, 7:34 p.m. | #2
On Fri, Nov 9, 2018 at 7:37 PM Iain Sandoe <iain@sandoe.co.uk> wrote:

> Bootstrap succeeds and the new test passes for Darwin.
>
> This does not alter that Darwin has breakage in this area (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78444)
> see : https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00884.html and HJ’s reply.

You can't use crtl->is_leaf in ix86_update_stack_boundary, since that
function gets called from cfgexpand, while crtl->is_leaf is set only
in IRA pass.

I *think* the fix should be along the lines of TARGET_64BIT_MS_ABI
fixup in ix86_compute_frame_layout (BTW: the existing fixup is strange
by itself, since TARGET_64BIT_MS_ABI declares STACK_BOUNDARY to 128,
and I can't see how leaf functions with crtl->preferred_stack_boundary
< 128 survive "gcc_assert (preferred_alignment >= STACK_BOUNDARY /
BITS_PER_UNIT);" a couple of lines below).

So, I think that fixup you proposed in the patch is in the right
direction. What happens if you add TARGET_MACHO to the existing fixup?

Uros.

Patch

Index: config/i386/cygming.h
===================================================================
--- config/i386/cygming.h	(revision 265930)
+++ config/i386/cygming.h	(working copy)
@@ -268,9 +268,6 @@ 
    bytes in one go.  */
 #define CHECK_STACK_LIMIT 4000
 
-#undef STACK_BOUNDARY
-#define STACK_BOUNDARY	(TARGET_64BIT && ix86_abi == MS_ABI ? 128 : BITS_PER_WORD)
-
 /* By default, target has a 80387, uses IEEE compatible arithmetic,
    returns float values in the 387 and needs stack probes.
    We also align doubles to 64-bits for MSVC default compatibility.  */
Index: config/i386/darwin.h
===================================================================
--- config/i386/darwin.h	(revision 265930)
+++ config/i386/darwin.h	(working copy)
@@ -113,8 +113,7 @@ 
    or dynamic loader.  */
 #undef STACK_BOUNDARY
 #define STACK_BOUNDARY \
- ((profile_flag || (TARGET_64BIT && ix86_abi == MS_ABI)) \
-  ? 128 : BITS_PER_WORD)
+  ((profile_flag || TARGET_64BIT_MS_ABI) ? 128 : BITS_PER_WORD)
 
 #undef MAIN_STACK_BOUNDARY
 #define MAIN_STACK_BOUNDARY 128
Index: config/i386/i386.h
===================================================================
--- config/i386/i386.h	(revision 265930)
+++ config/i386/i386.h	(working copy)
@@ -807,8 +807,7 @@ 
 #define PARM_BOUNDARY BITS_PER_WORD
 
 /* Boundary (in *bits*) on which stack pointer should be aligned.  */
-#define STACK_BOUNDARY \
- (TARGET_64BIT && ix86_abi == MS_ABI ? 128 : BITS_PER_WORD)
+#define STACK_BOUNDARY (TARGET_64BIT_MS_ABI ? 128 : BITS_PER_WORD)
 
 /* Stack boundary of the main function guaranteed by OS.  */
 #define MAIN_STACK_BOUNDARY (TARGET_64BIT ? 128 : 32)
Index: testsuite/gcc.target/i386/pr87928.c
===================================================================
--- testsuite/gcc.target/i386/pr87928.c	(nonexistent)
+++ testsuite/gcc.target/i386/pr87928.c	(working copy)
@@ -0,0 +1,23 @@ 
+/* { dg-do compile { target lp64 } } */
+/* { dg-options "-O1 -mstackrealign -mabi=ms" } */
+
+struct foo
+{
+  int a;
+  int b;
+  int c;
+  int d;
+};
+
+__attribute__ ((sysv_abi))
+struct foo bar (void)
+{
+  struct foo retval;
+
+  retval.a = 1;
+  retval.b = 2;
+  retval.c = 3;
+  retval.d = 4;
+
+  return retval;
+}