i386: Update INCOMING_FRAME_SP_OFFSET for exception handler

Since there is an extra error code passed to the exception handler,
INCOMING_FRAME_SP_OFFSET is return address plus error code for the
exception handler.  This patch updates INCOMING_FRAME_SP_OFFSET to
the correct value for the exception handler.

This patch exposed a bug in DWARF stack frame CFI generation, which
assumes that INCOMING_FRAME_SP_OFFSET is the same for all functions:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81570

It sets and caches the incoming stack frame offset with the same
INCOMING_FRAME_SP_OFFSET for all functions.  When there are both
exception handler and normal function in the same input, the wrong
incoming stack frame offset is used for exception handler or normal
function, which leads to

FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 error == 0x12345670
FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 frame->ip == 0x12345671
FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 frame->cs == 0x12345672
FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 frame->flags == 0x12345673
FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 frame->sp == 0x12345674
FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 frame->ss == 0x12345675

With the patch for PR 81570:

https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01851.html

applied, there are no regressions on i686 and x86-64.

OK for trunk?

H.J.
---
gcc/

	PR target/79793
	* config/i386/i386.c (ix86_function_arg): Update arguments for
	exception handler.
	(ix86_compute_frame_layout): Set the initial stack offset to
	INCOMING_FRAME_SP_OFFSET.  Update red-zone offset with
	INCOMING_FRAME_SP_OFFSET.
	(ix86_expand_epilogue): Don't pop the 'ERROR_CODE' off the
	stack before exception handler returns.
	* config/i386/i386.h (INCOMING_FRAME_SP_OFFSET): Add the
	the 'ERROR_CODE' for exception handler.

gcc/testsuite/

	PR target/79793
	* gcc.dg/guality/pr68037-1.c: Update gdb breakpoints.
	* gcc.target/i386/interrupt-5.c (interrupt_frame): New struct.
	(foo): Check the builtin return address against the return address
	in interrupt frame.
	* gcc.target/i386/pr79793-1.c: New test.
	* gcc.target/i386/pr79793-2.c: Likewise.
---
 gcc/config/i386/i386.c                      | 36 +++++++++--------------------
 gcc/config/i386/i386.h                      |  6 +++--
 gcc/testsuite/gcc.dg/guality/pr68037-1.c    | 12 +++++-----
 gcc/testsuite/gcc.target/i386/interrupt-5.c | 13 +++++++++--
 gcc/testsuite/gcc.target/i386/pr79793-1.c   | 14 +++++++++++
 gcc/testsuite/gcc.target/i386/pr79793-2.c   | 16 +++++++++++++
 6 files changed, 62 insertions(+), 35 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr79793-1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr79793-2.c

On Sat, Jul 29, 2017 at 4:22 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> Since there is an extra error code passed to the exception handler,
> INCOMING_FRAME_SP_OFFSET is return address plus error code for the
> exception handler.  This patch updates INCOMING_FRAME_SP_OFFSET to
> the correct value for the exception handler.
>
> This patch exposed a bug in DWARF stack frame CFI generation, which
> assumes that INCOMING_FRAME_SP_OFFSET is the same for all functions:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81570
>
> It sets and caches the incoming stack frame offset with the same
> INCOMING_FRAME_SP_OFFSET for all functions.  When there are both
> exception handler and normal function in the same input, the wrong
> incoming stack frame offset is used for exception handler or normal
> function, which leads to
>
> FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 error == 0x12345670
> FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 frame->ip == 0x12345671
> FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 frame->cs == 0x12345672
> FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 frame->flags == 0x12345673
> FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 frame->sp == 0x12345674
> FAIL: gcc.dg/guality/pr68037-1.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  line 33 frame->ss == 0x12345675
>
> With the patch for PR 81570:
>
> https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01851.html
>
> applied, there are no regressions on i686 and x86-64.
>
> OK for trunk?

I have two nits below.

LGTM - you know this part better than I, so it is your call.

Uros.

> H.J.
> ---
> gcc/
>
>         PR target/79793
>         * config/i386/i386.c (ix86_function_arg): Update arguments for
>         exception handler.
>         (ix86_compute_frame_layout): Set the initial stack offset to
>         INCOMING_FRAME_SP_OFFSET.  Update red-zone offset with
>         INCOMING_FRAME_SP_OFFSET.
>         (ix86_expand_epilogue): Don't pop the 'ERROR_CODE' off the
>         stack before exception handler returns.
>         * config/i386/i386.h (INCOMING_FRAME_SP_OFFSET): Add the
>         the 'ERROR_CODE' for exception handler.
>
> gcc/testsuite/
>
>         PR target/79793
>         * gcc.dg/guality/pr68037-1.c: Update gdb breakpoints.
>         * gcc.target/i386/interrupt-5.c (interrupt_frame): New struct.
>         (foo): Check the builtin return address against the return address
>         in interrupt frame.
>         * gcc.target/i386/pr79793-1.c: New test.
>         * gcc.target/i386/pr79793-2.c: Likewise.
> ---
>  gcc/config/i386/i386.c                      | 36 +++++++++--------------------
>  gcc/config/i386/i386.h                      |  6 +++--
>  gcc/testsuite/gcc.dg/guality/pr68037-1.c    | 12 +++++-----
>  gcc/testsuite/gcc.target/i386/interrupt-5.c | 13 +++++++++--
>  gcc/testsuite/gcc.target/i386/pr79793-1.c   | 14 +++++++++++
>  gcc/testsuite/gcc.target/i386/pr79793-2.c   | 16 +++++++++++++
>  6 files changed, 62 insertions(+), 35 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr79793-1.c
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr79793-2.c
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index f1486ff3750..e1582f8c9a6 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -10422,25 +10422,21 @@ ix86_function_arg (cumulative_args_t cum_v, machine_mode omode,
>         {
>           /* This is the pointer argument.  */
>           gcc_assert (TYPE_MODE (type) == Pmode);
> -         if (cfun->machine->func_type == TYPE_INTERRUPT)
> -           /* -WORD(AP) in the current frame in interrupt handler.  */
> -           arg = plus_constant (Pmode, arg_pointer_rtx,
> -                                -UNITS_PER_WORD);
> -         else
> -           /* (AP) in the current frame in exception handler.  */
> -           arg = arg_pointer_rtx;
> +         /* It is at -WORD(AP) in the current frame in interrupt and
> +            exception handlers.  */
> +         arg = plus_constant (Pmode, arg_pointer_rtx, -UNITS_PER_WORD);
>         }
>        else
>         {
>           gcc_assert (cfun->machine->func_type == TYPE_EXCEPTION
>                       && TREE_CODE (type) == INTEGER_TYPE
>                       && TYPE_MODE (type) == word_mode);
> -         /* The integer argument is the error code at -WORD(AP) in
> +         /* The integer argument is the error code at -2 * WORD(AP) in
>              the current frame in exception handler.  */

The comment should probably say:

  /* The error code is the word-mode integer argument at -2 * WORD(AP)
in the current frame of the exception handler.  */

>           arg = gen_rtx_MEM (word_mode,
>                              plus_constant (Pmode,
>                                             arg_pointer_rtx,
> -                                           -UNITS_PER_WORD));
> +                                           -2 * UNITS_PER_WORD));
>         }
>        return arg;
>      }
> @@ -12915,8 +12911,8 @@ ix86_compute_frame_layout (void)
>           the registers need to be saved before allocating the frame.  */
>         && flag_stack_check != STATIC_BUILTIN_STACK_CHECK);
>
> -  /* Skip return address.  */
> -  offset = UNITS_PER_WORD;
> +  /* Skip return address and error code in exception handler.  */
> +  offset = INCOMING_FRAME_SP_OFFSET;
>
>    /* Skip pushed static chain.  */
>    if (ix86_static_chain_on_stack)
> @@ -15221,8 +15217,9 @@ ix86_expand_epilogue (int style)
>    m->fs.red_zone_offset = 0;
>    if (ix86_using_red_zone () && crtl->args.pops_args < 65536)
>      {
> -      /* The red-zone begins below the return address.  */
> -      m->fs.red_zone_offset = RED_ZONE_SIZE + UNITS_PER_WORD;
> +      /* The red-zone begins below return address and error code in
> +        exception handler.  */
> +      m->fs.red_zone_offset = RED_ZONE_SIZE + INCOMING_FRAME_SP_OFFSET;
>
>        /* When the register save area is in the aligned portion of
>           the stack, determine the maximum runtime displacement that
> @@ -15517,18 +15514,7 @@ ix86_expand_epilogue (int style)
>      }
>
>    if (cfun->machine->func_type != TYPE_NORMAL)
> -    {
> -      /* Return with the "IRET" instruction from interrupt handler.
> -        Pop the 'ERROR_CODE' off the stack before the 'IRET'
> -        instruction in exception handler.  */
> -      if (cfun->machine->func_type == TYPE_EXCEPTION)
> -       {
> -         rtx r = plus_constant (Pmode, stack_pointer_rtx,
> -                                UNITS_PER_WORD);
> -         emit_insn (gen_rtx_SET (stack_pointer_rtx, r));
> -       }
> -      emit_jump_insn (gen_interrupt_return ());
> -    }
> +    emit_jump_insn (gen_interrupt_return ());
>    else if (crtl->args.pops_args && crtl->args.size)
>      {
>        rtx popc = GEN_INT (crtl->args.pops_args);
> diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
> index 682745ae06b..ddbd6759caf 100644
> --- a/gcc/config/i386/i386.h
> +++ b/gcc/config/i386/i386.h
> @@ -2177,8 +2177,10 @@ extern int const svr4_dbx_register_map[FIRST_PSEUDO_REGISTER];
>  /* PC is dbx register 8; let's use that column for RA.  */
>  #define DWARF_FRAME_RETURN_COLUMN      (TARGET_64BIT ? 16 : 8)
>
> -/* Before the prologue, the top of the frame is at 4(%esp).  */
> -#define INCOMING_FRAME_SP_OFFSET UNITS_PER_WORD
> +/* Before the prologue, there are return address and error code for
> +   exception handler on the top of the frame.  */
> +#define INCOMING_FRAME_SP_OFFSET \
> +  (UNITS_PER_WORD * (1 + (cfun->machine->func_type == TYPE_EXCEPTION)))

The above is unnecessarily complicated way to write:

+#define INCOMING_FRAME_SP_OFFSET \
+  (cfun->machine->func_type == TYPE_EXCEPTION \
+   ? 2 * UNITS_PER_WORD : UNITS_PER_WORD)

>  /* Describe how we implement __builtin_eh_return.  */
>  #define EH_RETURN_DATA_REGNO(N)        ((N) <= DX_REG ? (N) : INVALID_REGNUM)
> diff --git a/gcc/testsuite/gcc.dg/guality/pr68037-1.c b/gcc/testsuite/gcc.dg/guality/pr68037-1.c
> index 74f61ec5f96..44cab58659f 100644
> --- a/gcc/testsuite/gcc.dg/guality/pr68037-1.c
> +++ b/gcc/testsuite/gcc.dg/guality/pr68037-1.c
> @@ -59,9 +59,9 @@ main ()
>    return 0;
>  }
>
> -/* { dg-final { gdb-test 31 "error" "0x12345670" } } */
> -/* { dg-final { gdb-test 31 "frame->ip" "0x12345671" } } */
> -/* { dg-final { gdb-test 31 "frame->cs" "0x12345672" } } */
> -/* { dg-final { gdb-test 31 "frame->flags" "0x12345673" } } */
> -/* { dg-final { gdb-test 31 "frame->sp" "0x12345674" } } */
> -/* { dg-final { gdb-test 31 "frame->ss" "0x12345675" } } */
> +/* { dg-final { gdb-test 33 "error" "0x12345670" } } */
> +/* { dg-final { gdb-test 33 "frame->ip" "0x12345671" } } */
> +/* { dg-final { gdb-test 33 "frame->cs" "0x12345672" } } */
> +/* { dg-final { gdb-test 33 "frame->flags" "0x12345673" } } */
> +/* { dg-final { gdb-test 33 "frame->sp" "0x12345674" } } */
> +/* { dg-final { gdb-test 33 "frame->ss" "0x12345675" } } */
> diff --git a/gcc/testsuite/gcc.target/i386/interrupt-5.c b/gcc/testsuite/gcc.target/i386/interrupt-5.c
> index 803c0636299..5742b6f4743 100644
> --- a/gcc/testsuite/gcc.target/i386/interrupt-5.c
> +++ b/gcc/testsuite/gcc.target/i386/interrupt-5.c
> @@ -7,12 +7,21 @@ extern void link_error (void);
>
>  typedef unsigned int uword_t __attribute__ ((mode (__word__)));
>
> +struct interrupt_frame
> +{
> +  uword_t ip;
> +  uword_t cs;
> +  uword_t flags;
> +  uword_t sp;
> +  uword_t ss;
> +};
> +
>  __attribute__ ((used, interrupt))
>  void
> -foo (void *frame, uword_t error)
> +foo (struct interrupt_frame *frame, uword_t error)
>  {
>    void *ra = __builtin_return_address (0);
> -  if ((uintptr_t) ra != (uintptr_t) error)
> +  if ((uintptr_t) ra != (uintptr_t) frame->ip)
>      link_error ();
>  }
>
> diff --git a/gcc/testsuite/gcc.target/i386/pr79793-1.c b/gcc/testsuite/gcc.target/i386/pr79793-1.c
> new file mode 100644
> index 00000000000..a382fe9c5e2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr79793-1.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile { target { *-*-linux* && { ! ia32 } } } } */
> +/* { dg-options "-O2 -mgeneral-regs-only" } */
> +
> +void
> + __attribute__ ((interrupt))
> +fn1 (void *frame)
> +{
> +  char fxsave_region [512] __attribute__((aligned(16)));
> +  __builtin_ia32_fxsave64 (fxsave_region);
> +}
> +
> +/* { dg-final { scan-assembler-times "sub\[lq\]\[\t \]*\\\$400,\[\t \]*%\[re\]sp" 1 } } */
> +/* { dg-final { scan-assembler-times "fxsave64\[\t \]*-120\\(%\[re\]sp\\)" 1 } } */
> +/* { dg-final { scan-assembler-times "add\[lq\]\[\t \]*\\\$400,\[\t \]*%\[re\]sp" 1 } } */
> diff --git a/gcc/testsuite/gcc.target/i386/pr79793-2.c b/gcc/testsuite/gcc.target/i386/pr79793-2.c
> new file mode 100644
> index 00000000000..f6ae5aed33a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr79793-2.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile { target { *-*-linux* && { ! ia32 } } } } */
> +/* { dg-options "-O2 -mgeneral-regs-only" } */
> +
> +typedef unsigned int uword_t __attribute__ ((mode (__word__)));
> +
> +void
> + __attribute__ ((interrupt))
> +fn1 (void *frame, uword_t error)
> +{
> +  char fxsave_region [512] __attribute__((aligned(16)));
> +  __builtin_ia32_fxsave64 (fxsave_region);
> +}
> +
> +/* { dg-final { scan-assembler-times "sub\[lq\]\[\t \]*\\\$392,\[\t \]*%\[re\]sp" 1 } } */
> +/* { dg-final { scan-assembler-times "fxsave64\[\t \]*-120\\(%\[re\]sp\\)" 1 } } */
> +/* { dg-final { scan-assembler-times "add\[lq\]\[\t \]*\\\$400,\[\t \]*%\[re\]sp" 1 } } */
> --
> 2.13.3
>