On Sat, Jul 29, 2017 at 4:22 PM, H.J. Lu <hjl.to...@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
>
