Re: [PATCH] Use ptr_mode to save/restore pointers in builtin jmpbuf

[Uros Bizjak <ubizjak at gmail dot com>]

On Thu, Feb 1, 2018 at 1:16 AM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>
> We currently read and write beyond the builtin jmpbuf on ILP32 targets
> where Pmode == DImode and ptr_mode == SImode.  Since the builtin jmpbuf
> is an array of 5 pointers, ptr_mode should be used to save and restore
> frame and program pointers.  Since x86 only saves stack pointer in
> stack save area, STACK_SAVEAREA_MODE should be ptr_mode, not Pmode.
>
> Note: If ptr_mode != Pmode, builtin setjmp/longjmp tests may fail.  When
> it happens, please check if there are correct save_stack_nonlocal and
> restore_stack_nonlocal expand patterns.
>
> Tested on i686 and x86-64.  OK for trunk?
>
> H.J.
> ----
> gcc/
>
>         PR middle-end/84150
>         * builtins.c (expand_builtin_setjmp_setup): Use ptr_mode to
>         save frame and program pointers.
>         (expand_builtin_longjmp): Use ptr_mode to restore frame and
>         program pointers.
>         * config/i386/i386.md (save_stack_nonlocal): New expand pattern.
>         (restore_stack_nonlocal): Likewise.
>         * config/i386/i386.h (STACK_SAVEAREA_MODE): New.
>
> gcc/testsuite/
>
>         PR middle-end/84150
>         * gcc.dg/pr84150.c: New test.
>         * gcc.target/i386/pr84150.c: Likewisde.
> ---
>  gcc/builtins.c                          | 36 +++++++++++++++++---------
>  gcc/config/i386/i386.h                  |  4 +++
>  gcc/config/i386/i386.md                 | 36 ++++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/pr84150.c          | 45 +++++++++++++++++++++++++++++++++
>  gcc/testsuite/gcc.target/i386/pr84150.c | 44 ++++++++++++++++++++++++++++++++
>  5 files changed, 153 insertions(+), 12 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/pr84150.c
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr84150.c
>
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index 683c6ec6e5b..c3d45d5e3fa 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -840,6 +840,7 @@ expand_builtin_setjmp_setup (rtx buf_addr, rtx receiver_label)
>    machine_mode sa_mode = STACK_SAVEAREA_MODE (SAVE_NONLOCAL);
>    rtx stack_save;
>    rtx mem;
> +  rtx tmp;
>
>    if (setjmp_alias_set == -1)
>      setjmp_alias_set = new_alias_set ();
> @@ -852,20 +853,25 @@ expand_builtin_setjmp_setup (rtx buf_addr, rtx receiver_label)
>       the buffer and use the rest of it for the stack save area, which
>       is machine-dependent.  */
>
> -  mem = gen_rtx_MEM (Pmode, buf_addr);
> +  mem = gen_rtx_MEM (ptr_mode, buf_addr);
>    set_mem_alias_set (mem, setjmp_alias_set);
> -  emit_move_insn (mem, targetm.builtin_setjmp_frame_value ());
> +  tmp = targetm.builtin_setjmp_frame_value ();
> +  if (GET_MODE (tmp) != ptr_mode)
> +    tmp = gen_lowpart (ptr_mode, tmp);
> +  emit_move_insn (mem, tmp);
>
> -  mem = gen_rtx_MEM (Pmode, plus_constant (Pmode, buf_addr,
> -                                          GET_MODE_SIZE (Pmode))),
> +  mem = gen_rtx_MEM (ptr_mode, plus_constant (Pmode, buf_addr,
> +                                             GET_MODE_SIZE (ptr_mode))),
>    set_mem_alias_set (mem, setjmp_alias_set);
>
> -  emit_move_insn (validize_mem (mem),
> -                 force_reg (Pmode, gen_rtx_LABEL_REF (Pmode, receiver_label)));
> +  tmp = force_reg (Pmode, gen_rtx_LABEL_REF (Pmode, receiver_label));
> +  if (Pmode != ptr_mode)
> +    tmp = gen_lowpart (ptr_mode, tmp);
> +  emit_move_insn (validize_mem (mem), tmp);
>
>    stack_save = gen_rtx_MEM (sa_mode,
>                             plus_constant (Pmode, buf_addr,
> -                                          2 * GET_MODE_SIZE (Pmode)));
> +                                          2 * GET_MODE_SIZE (ptr_mode)));
>    set_mem_alias_set (stack_save, setjmp_alias_set);
>    emit_stack_save (SAVE_NONLOCAL, &stack_save);
>
> @@ -991,12 +997,14 @@ expand_builtin_longjmp (rtx buf_addr, rtx value)
>      emit_insn (targetm.gen_builtin_longjmp (buf_addr));
>    else
>      {
> -      fp = gen_rtx_MEM (Pmode, buf_addr);
> -      lab = gen_rtx_MEM (Pmode, plus_constant (Pmode, buf_addr,
> -                                              GET_MODE_SIZE (Pmode)));
> +      fp = gen_rtx_MEM (ptr_mode, buf_addr);
> +      lab = gen_rtx_MEM (ptr_mode,
> +                        plus_constant (Pmode, buf_addr,
> +                                       GET_MODE_SIZE (ptr_mode)));
>
> -      stack = gen_rtx_MEM (sa_mode, plus_constant (Pmode, buf_addr,
> -                                                  2 * GET_MODE_SIZE (Pmode)));
> +      stack = gen_rtx_MEM (sa_mode,
> +                          plus_constant (Pmode, buf_addr,
> +                                         2 * GET_MODE_SIZE (ptr_mode)));
>        set_mem_alias_set (fp, setjmp_alias_set);
>        set_mem_alias_set (lab, setjmp_alias_set);
>        set_mem_alias_set (stack, setjmp_alias_set);
> @@ -1015,6 +1023,10 @@ expand_builtin_longjmp (rtx buf_addr, rtx value)
>           emit_clobber (gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode)));
>           emit_clobber (gen_rtx_MEM (BLKmode, hard_frame_pointer_rtx));
>
> +#ifdef POINTERS_EXTEND_UNSIGNED
> +         if (GET_MODE (fp) != Pmode)
> +           fp = convert_to_mode (Pmode, fp, POINTERS_EXTEND_UNSIGNED);
> +#endif
>           emit_move_insn (hard_frame_pointer_rtx, fp);
>           emit_stack_restore (SAVE_NONLOCAL, stack);
>
> diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
> index 59522ccba02..16aace86e48 100644
> --- a/gcc/config/i386/i386.h
> +++ b/gcc/config/i386/i386.h
> @@ -1937,6 +1937,10 @@ do {                                                     \
>     between pointers and any other objects of this machine mode.  */
>  #define Pmode (ix86_pmode == PMODE_DI ? DImode : SImode)
>
> +/* Supply a definition of STACK_SAVEAREA_MODE for emit_stack_save.  We
> +   only need save and restore stack pointer in ptr_mode.  */
> +#define STACK_SAVEAREA_MODE(LEVEL) ptr_mode
> +
>  /* Specify the machine mode that bounds have.  */
>  #define BNDmode (ix86_pmode == PMODE_DI ? BND64mode : BND32mode)
>
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index c08e4f55cff..c563a26cd60 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -18375,6 +18375,42 @@
>    "* return output_probe_stack_range (operands[0], operands[2]);"
>    [(set_attr "type" "multi")])
>
> +;; Non-local goto support.
> +
> +(define_expand "save_stack_nonlocal"
> +  [(use (match_operand 0 "memory_operand" ""))
> +   (use (match_operand 1 "register_operand" ""))]
> +  ""
> +{
> +  /* Stack is saved in ptr_mode and stack register is in Pmode.  */
> +  if (GET_MODE (operands[0]) != GET_MODE (operands[1]))
> +    {
> +      if (GET_MODE (operands[0]) != ptr_mode
> +         || GET_MODE (operands[1]) != Pmode)
> +       gcc_unreachable ();

gcc_assert instead of if (...) gcc_unreachable.

> +      operands[1] = gen_rtx_SUBREG (ptr_mode, operands[1], 0);

gen_lowpart

> +    }
> +  emit_move_insn (operands[0], operands[1]);
> +  DONE;
> +})
> +
> +(define_expand "restore_stack_nonlocal"
> +  [(use (match_operand 0 "register_operand" ""))
> +   (use (match_operand 1 "memory_operand" ""))]
> +  ""
> +{
> +  /* Stack is saved in ptr_mode and stack register is in Pmode.  */
> +  if (GET_MODE (operands[0]) != GET_MODE (operands[1]))
> +    {
> +      if (GET_MODE (operands[0]) != Pmode
> +         || GET_MODE (operands[1]) != ptr_mode)
> +       gcc_unreachable ();

Also here.

> +      operands[1] = gen_rtx_ZERO_EXTEND (Pmode, operands[1]);
> +    }
> +  emit_move_insn (operands[0], operands[1]);
> +  DONE;
> +})
> +
>  /* Additional processing for builtin_setjmp.  Store the shadow stack pointer
>     as a forth element in jmpbuf.  */
>  (define_expand "builtin_setjmp_setup"
> diff --git a/gcc/testsuite/gcc.dg/pr84150.c b/gcc/testsuite/gcc.dg/pr84150.c
> new file mode 100644
> index 00000000000..1e7a04c1e9c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr84150.c
> @@ -0,0 +1,45 @@
> +/* { dg-do run } */
> +/* { dg-options "-O" } */
> +/* { dg-require-effective-target indirect_jumps } */
> +
> +void *buf[6] = {
> +  (void *) -1,
> +  (void *) -1,
> +  (void *) -1,
> +  (void *) -1,
> +  (void *) -1,
> +  (void *) -1
> +};
> +
> +void raise0(void)
> +{
> +  __builtin_longjmp (buf, 1);
> +}
> +
> +int execute(int cmd)
> +{
> +  int last = 0;
> +
> +  if (__builtin_setjmp (buf) == 0)
> +    while (1)
> +      {
> +        last = 1;
> +        raise0 ();
> +      }
> +
> +  if (last == 0)
> +    return 0;
> +  else
> +    return cmd;
> +}
> +
> +int main(void)
> +{
> +  if (execute (1) == 0)
> +    __builtin_abort ();
> +
> +  if (buf[5] != (void *) -1)
> +    __builtin_abort ();
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/i386/pr84150.c b/gcc/testsuite/gcc.target/i386/pr84150.c
> new file mode 100644
> index 00000000000..11d3d361487
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr84150.c
> @@ -0,0 +1,44 @@
> +/* { dg-do run { target x32 } } */
> +/* { dg-options "-O -maddress-mode=long" } */
> +
> +void *buf[6] = {
> +  (void *) -1,
> +  (void *) -1,
> +  (void *) -1,
> +  (void *) -1,
> +  (void *) -1,
> +  (void *) -1
> +};
> +
> +void raise0(void)
> +{
> +  __builtin_longjmp (buf, 1);
> +}
> +
> +int execute(int cmd)
> +{
> +  int last = 0;
> +
> +  if (__builtin_setjmp (buf) == 0)
> +    while (1)
> +      {
> +        last = 1;
> +        raise0 ();
> +      }
> +
> +  if (last == 0)
> +    return 0;
> +  else
> +    return cmd;
> +}
> +
> +int main(void)
> +{
> +  if (execute (1) == 0)
> +    __builtin_abort ();
> +
> +  if (buf[5] != (void *) -1)
> +    __builtin_abort ();
> +
> +  return 0;
> +}
> --
> 2.14.3
>