RFA: Fix calculation of size of builtin setjmp buffer

RFA: Fix calculation of size of builtin setjmp buffer

[Nick Clifton]

Hi Guys,

  There is a small bug in the computation for the size of the builtin
  setjmp buffer.  The size is based upon BITS_PER_WORD / POINTER_SIZE
  which for most targets equates to 1.  But for targets where pointers
  are larger than a word, it equates to zero.  This leads to stack
  corruption and all kinds of fun things.

  The patch is obvious - see below - but since it affects generic code
  and might have consequences which I have not foreseen, I thought it
  best to ask for approval first.

  No regressions with an x86_64-pc-linux toolchain, and quite a few G++
  testsuite fixes for an rl78-elf toolchain.

  OK to apply ?

Cheers
  Nick

2014-05-06  Nick Clifton  <nickc@redhat.com>

	* except.c (init_eh): Fix computation of builtin setjmp buffer
	size.

Index: gcc/except.c
===================================================================
--- gcc/except.c	(revision 210096)
+++ gcc/except.c	(working copy)
@@ -287,7 +287,7 @@
 #endif
 #else
       /* builtin_setjmp takes a pointer to 5 words.  */
-      tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
+      tmp = size_int (5 * POINTER_SIZE / BITS_PER_WORD - 1);
 #endif
       tmp = build_index_type (tmp);
       tmp = build_array_type (ptr_type_node, tmp);

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Jakub Jelinek]

On Tue, May 06, 2014 at 01:55:18PM +0100, Nick Clifton wrote:
> 2014-05-06  Nick Clifton  <nickc@redhat.com>
> 
> 	* except.c (init_eh): Fix computation of builtin setjmp buffer
> 	size.
> 
> Index: gcc/except.c
> ===================================================================
> --- gcc/except.c	(revision 210096)
> +++ gcc/except.c	(working copy)
> @@ -287,7 +287,7 @@
>  #endif
>  #else
>        /* builtin_setjmp takes a pointer to 5 words.  */
> -      tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
> +      tmp = size_int (5 * POINTER_SIZE / BITS_PER_WORD - 1);
>  #endif
>        tmp = build_index_type (tmp);
>        tmp = build_array_type (ptr_type_node, tmp);

That doesn't look correct to me.  If the code wants to create 5 words long
array, but with pointer elements (instead of word sized elements), then
5 * BITS_PER_WORD is the desired size in bits of the buffer, POINTER_SIZE
is how many bits each void *array[...] element occupies and thus
5 * BITS_PER_WORD / POINTER_SIZE - 1 is the right last element, so I'd
say the previous expression is the right one.  Perhaps you want to round up
rather than down, but certainly not swap the division operands.

Now, if the code actually expects 5 pointers, rather than 5 words, or
something else, then you'd at least need to also fix the comment.

	Jakub

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Nicholas Clifton]

Hi Jakub,

>>         /* builtin_setjmp takes a pointer to 5 words.  */
>> -      tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
>> +      tmp = size_int (5 * POINTER_SIZE / BITS_PER_WORD - 1);

> That doesn't look correct to me.  If the code wants to create 5 words long
> array, but with pointer elements (instead of word sized elements), then
> 5 * BITS_PER_WORD is the desired size in bits of the buffer, POINTER_SIZE
> is how many bits each void *array[...] element occupies and thus
> 5 * BITS_PER_WORD / POINTER_SIZE - 1 is the right last element, so I'd
> say the previous expression is the right one.  Perhaps you want to round up
> rather than down, but certainly not swap the division operands.
>
> Now, if the code actually expects 5 pointers, rather than 5 words, or
> something else, then you'd at least need to also fix the comment.

The contents of the builtin setjmp buffer do not appear to be explicitly 
documented anywhere.   The nearest that I could come was this line in 
the description of builtin_setjmp_setup in gcc/doc/md.texi:

   Note that the buffer is five words long and that
   the first three are normally used by the generic
   mechanism.

But a comment in gcc/except.c:expand_builtin_setjmp_setup() says that 
the first three entries in the buffer are for the frame pointer, the 
address of the return label and the stack pointer.  This appears to 
suggest that it is 3 pointers that are stored in the buffer not 3 words.

Given that pointers can be bigger than words, and that if a pointer is 2 
words long then even a 5 word buffer will be too small, I still feel 
that my patch is correct.  So here is a revised patch which updates the 
comments in all of the places that I could find them, adds a description 
of builtin_setjmp buffer to the documentation, and includes my original, 
not quite so obvious fix.

OK to apply ?

Cheers
   Nick

gcc/ChangeLog
2014-05-06  Nick Clifton  <nickc@redhat.com>

	* builtins.c: Update description of buffer used by builtin setjmp
	and longjmp.
	* doc/md.texi (builtin_setjmp_setup): Likewise.
	* except.c (init_eh): Fix computation of builtin setjmp buffer
	size.

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 210096)
+++ gcc/builtins.c	(working copy)
@@ -977,10 +977,10 @@
    emit_insn (gen_blockage ());
  }

-/* __builtin_longjmp is passed a pointer to an array of five words (not
-   all will be used on all machines).  It operates similarly to the C
-   library function of the same name, but is more efficient.  Much of
-   the code below is copied from the handling of non-local gotos.  */
+/* __builtin_longjmp is passed a pointer to an array of five Pmode sized
+   entries (not all will be used on all machines).  It operates similarly
+   to the C library function of the same name, but is more efficient.  Much
+   of the code below is copied from the handling of non-local gotos.  */

  static void
  expand_builtin_longjmp (rtx buf_addr, rtx value)
@@ -1204,10 +1204,10 @@
    return const0_rtx;
  }

-/* __builtin_update_setjmp_buf is passed a pointer to an array of five 
words
-   (not all will be used on all machines) that was passed to 
__builtin_setjmp.
-   It updates the stack pointer in that block to correspond to the current
-   stack pointer.  */
+/* __builtin_update_setjmp_buf is passed a pointer to an array of five 
Pmode
+   sized entries (not all will be used on all machines) that was passed to
+   __builtin_setjmp.  It updates the stack pointer in that block to 
correspond
+   to the current stack pointer.  */

  static void
  expand_builtin_update_setjmp_buf (rtx buf_addr)
@@ -6205,8 +6205,8 @@
        gcc_unreachable ();

      case BUILT_IN_SETJMP_SETUP:
-      /* __builtin_setjmp_setup is passed a pointer to an array of five 
words
-          and the receiver label.  */
+      /* __builtin_setjmp_setup is passed a pointer to an array of five
+	 Pmode sized entries and the receiver label.  */
        if (validate_arglist (exp, POINTER_TYPE, POINTER_TYPE, VOID_TYPE))
  	{
  	  rtx buf_addr = expand_expr (CALL_EXPR_ARG (exp, 0), subtarget,
@@ -6239,9 +6239,9 @@
  	}
        break;

-      /* __builtin_longjmp is passed a pointer to an array of five words.
-	 It's similar to the C library longjmp function but works with
-	 __builtin_setjmp above.  */
+      /* __builtin_longjmp is passed a pointer to an array of five Pmode
+	 sized entries.  It's similar to the C library longjmp function
+	 but works with __builtin_setjmp above.  */
      case BUILT_IN_LONGJMP:
        if (validate_arglist (exp, POINTER_TYPE, INTEGER_TYPE, VOID_TYPE))
  	{
Index: gcc/doc/md.texi
===================================================================
--- gcc/doc/md.texi	(revision 210096)
+++ gcc/doc/md.texi	(working copy)
@@ -6113,8 +6113,10 @@
  as a pointer to a global table, must be restored.  Though it is
  preferred that the pointer value be recalculated if possible (given the
  address of a label for instance).  The single argument is a pointer to
-the @code{jmp_buf}.  Note that the buffer is five words long and that
-the first three are normally used by the generic mechanism.
+the @code{jmp_buf}.  Note that the buffer is big enough for five
+@code{Pmode} entries.  The generic machanism just uses the first three
+entries to hold the frame pointer, return address and stack pointer
+respectively, but target specific code can use all five entries.

  @cindex @code{builtin_setjmp_receiver} instruction pattern
  @item @samp{builtin_setjmp_receiver}
Index: gcc/except.c
===================================================================
--- gcc/except.c	(revision 210096)
+++ gcc/except.c	(working copy)
@@ -286,8 +286,8 @@
        tmp = size_int (FIRST_PSEUDO_REGISTER + 2 - 1);
  #endif
  #else
-      /* builtin_setjmp takes a pointer to 5 words.  */
-      tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
+      /* builtin_setjmp uses a buffer big enough to hold 5 pointers.  */
+      tmp = size_int (5 * POINTER_SIZE / BITS_PER_WORD - 1);
  #endif
        tmp = build_index_type (tmp);
        tmp = build_array_type (ptr_type_node, tmp);

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Richard Biener]

On Tue, May 6, 2014 at 4:34 PM, Nicholas Clifton <nickc@redhat.com> wrote:
> Hi Jakub,
>
>
>>>         /* builtin_setjmp takes a pointer to 5 words.  */
>>> -      tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
>>> +      tmp = size_int (5 * POINTER_SIZE / BITS_PER_WORD - 1);
>
>
>> That doesn't look correct to me.  If the code wants to create 5 words long
>> array, but with pointer elements (instead of word sized elements), then
>> 5 * BITS_PER_WORD is the desired size in bits of the buffer, POINTER_SIZE
>> is how many bits each void *array[...] element occupies and thus
>> 5 * BITS_PER_WORD / POINTER_SIZE - 1 is the right last element, so I'd
>> say the previous expression is the right one.  Perhaps you want to round
>> up
>> rather than down, but certainly not swap the division operands.
>>
>> Now, if the code actually expects 5 pointers, rather than 5 words, or
>> something else, then you'd at least need to also fix the comment.
>
>
> The contents of the builtin setjmp buffer do not appear to be explicitly
> documented anywhere.   The nearest that I could come was this line in the
> description of builtin_setjmp_setup in gcc/doc/md.texi:
>
>   Note that the buffer is five words long and that
>   the first three are normally used by the generic
>   mechanism.
>
> But a comment in gcc/except.c:expand_builtin_setjmp_setup() says that the
> first three entries in the buffer are for the frame pointer, the address of
> the return label and the stack pointer.  This appears to suggest that it is
> 3 pointers that are stored in the buffer not 3 words.
>
> Given that pointers can be bigger than words, and that if a pointer is 2
> words long then even a 5 word buffer will be too small, I still feel that my
> patch is correct.  So here is a revised patch which updates the comments in
> all of the places that I could find them, adds a description of
> builtin_setjmp buffer to the documentation, and includes my original, not
> quite so obvious fix.

As a pointer can also be smaller than a word maybe take the maximum of
both readings?  But Eric should know what was intended here ...

Richard.

>
> OK to apply ?
>
> Cheers
>   Nick
>
> gcc/ChangeLog
> 2014-05-06  Nick Clifton  <nickc@redhat.com>
>
>         * builtins.c: Update description of buffer used by builtin setjmp
>         and longjmp.
>         * doc/md.texi (builtin_setjmp_setup): Likewise.
>
>         * except.c (init_eh): Fix computation of builtin setjmp buffer
>         size.
>
> Index: gcc/builtins.c
> ===================================================================
> --- gcc/builtins.c      (revision 210096)
> +++ gcc/builtins.c      (working copy)
> @@ -977,10 +977,10 @@
>    emit_insn (gen_blockage ());
>  }
>
> -/* __builtin_longjmp is passed a pointer to an array of five words (not
> -   all will be used on all machines).  It operates similarly to the C
> -   library function of the same name, but is more efficient.  Much of
> -   the code below is copied from the handling of non-local gotos.  */
> +/* __builtin_longjmp is passed a pointer to an array of five Pmode sized
> +   entries (not all will be used on all machines).  It operates similarly
> +   to the C library function of the same name, but is more efficient.  Much
> +   of the code below is copied from the handling of non-local gotos.  */
>
>  static void
>  expand_builtin_longjmp (rtx buf_addr, rtx value)
> @@ -1204,10 +1204,10 @@
>    return const0_rtx;
>  }
>
> -/* __builtin_update_setjmp_buf is passed a pointer to an array of five
> words
> -   (not all will be used on all machines) that was passed to
> __builtin_setjmp.
> -   It updates the stack pointer in that block to correspond to the current
> -   stack pointer.  */
> +/* __builtin_update_setjmp_buf is passed a pointer to an array of five
> Pmode
> +   sized entries (not all will be used on all machines) that was passed to
> +   __builtin_setjmp.  It updates the stack pointer in that block to
> correspond
> +   to the current stack pointer.  */
>
>  static void
>  expand_builtin_update_setjmp_buf (rtx buf_addr)
> @@ -6205,8 +6205,8 @@
>        gcc_unreachable ();
>
>      case BUILT_IN_SETJMP_SETUP:
> -      /* __builtin_setjmp_setup is passed a pointer to an array of five
> words
> -          and the receiver label.  */
> +      /* __builtin_setjmp_setup is passed a pointer to an array of five
> +        Pmode sized entries and the receiver label.  */
>        if (validate_arglist (exp, POINTER_TYPE, POINTER_TYPE, VOID_TYPE))
>         {
>           rtx buf_addr = expand_expr (CALL_EXPR_ARG (exp, 0), subtarget,
> @@ -6239,9 +6239,9 @@
>         }
>        break;
>
> -      /* __builtin_longjmp is passed a pointer to an array of five words.
> -        It's similar to the C library longjmp function but works with
> -        __builtin_setjmp above.  */
> +      /* __builtin_longjmp is passed a pointer to an array of five Pmode
> +        sized entries.  It's similar to the C library longjmp function
> +        but works with __builtin_setjmp above.  */
>      case BUILT_IN_LONGJMP:
>        if (validate_arglist (exp, POINTER_TYPE, INTEGER_TYPE, VOID_TYPE))
>         {
> Index: gcc/doc/md.texi
> ===================================================================
> --- gcc/doc/md.texi     (revision 210096)
> +++ gcc/doc/md.texi     (working copy)
> @@ -6113,8 +6113,10 @@
>  as a pointer to a global table, must be restored.  Though it is
>  preferred that the pointer value be recalculated if possible (given the
>  address of a label for instance).  The single argument is a pointer to
> -the @code{jmp_buf}.  Note that the buffer is five words long and that
> -the first three are normally used by the generic mechanism.
> +the @code{jmp_buf}.  Note that the buffer is big enough for five
> +@code{Pmode} entries.  The generic machanism just uses the first three
> +entries to hold the frame pointer, return address and stack pointer
> +respectively, but target specific code can use all five entries.
>
>  @cindex @code{builtin_setjmp_receiver} instruction pattern
>  @item @samp{builtin_setjmp_receiver}
>
> Index: gcc/except.c
> ===================================================================
> --- gcc/except.c        (revision 210096)
> +++ gcc/except.c        (working copy)
> @@ -286,8 +286,8 @@
>        tmp = size_int (FIRST_PSEUDO_REGISTER + 2 - 1);
>  #endif
>  #else
> -      /* builtin_setjmp takes a pointer to 5 words.  */
>
> -      tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
> +      /* builtin_setjmp uses a buffer big enough to hold 5 pointers.  */
>
> +      tmp = size_int (5 * POINTER_SIZE / BITS_PER_WORD - 1);
>  #endif
>        tmp = build_index_type (tmp);
>        tmp = build_array_type (ptr_type_node, tmp);
>

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Jakub Jelinek]

On Tue, May 06, 2014 at 03:34:37PM +0100, Nicholas Clifton wrote:
> --- gcc/except.c	(revision 210096)
> +++ gcc/except.c	(working copy)
> @@ -286,8 +286,8 @@
>        tmp = size_int (FIRST_PSEUDO_REGISTER + 2 - 1);
>  #endif
>  #else
> -      /* builtin_setjmp takes a pointer to 5 words.  */
> -      tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
> +      /* builtin_setjmp uses a buffer big enough to hold 5 pointers.  */
> +      tmp = size_int (5 * POINTER_SIZE / BITS_PER_WORD - 1);

But what will this do on targets where POINTER_SIZE is smaller than
BITS_PER_WORD?  E.g. I think some options on s390 or ppc.
If you want it to be always 5 pointers, then you want
  tmp = size_int (4);
and not something else, otherwise it really depends on how exactly it is
used, perhaps it can be 5 * MAX (BITS_PER_WORD / POINTER_SIZE, 1) - 1
or whatever else, but 5 * POINTER_SIZE / BITS_PER_WORD is definitely wrong.

>  #endif
>        tmp = build_index_type (tmp);
>        tmp = build_array_type (ptr_type_node, tmp);

	Jakub

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Nicholas Clifton]

Hi Jakub,

> But what will this do on targets where POINTER_SIZE is smaller than
> BITS_PER_WORD?  E.g. I think some options on s390 or ppc.
> If you want it to be always 5 pointers, then you want
>    tmp = size_int (4);
> and not something else, otherwise it really depends on how exactly it is
> used, perhaps it can be 5 * MAX (BITS_PER_WORD / POINTER_SIZE, 1) - 1
> or whatever else, but 5 * POINTER_SIZE / BITS_PER_WORD is definitely wrong.

Ah -I had not considered this.

OK - how about this revision ?  It allocates a buffer big enough to hold 
5 things, either pointers or words, whichever is bigger.  The comments 
are suitably adjusted as well.

Cheers
   Nick


Index: gcc/except.c
===================================================================
--- gcc/except.c	(revision 210096)
+++ gcc/except.c	(working copy)
@@ -286,8 +286,9 @@
        tmp = size_int (FIRST_PSEUDO_REGISTER + 2 - 1);
  #endif
  #else
-      /* builtin_setjmp takes a pointer to 5 words.  */
-      tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
+      /* builtin_setjmp uses a buffer big enough to hold
+	 5 pointers or 5 words, whichever is bigger.  */
+      tmp = size_int ((5 * MAX (POINTER_SIZE, BITS_PER_WORD)) / 
BITS_PER_WORD - 1);
  #endif
        tmp = build_index_type (tmp);
        tmp = build_array_type (ptr_type_node, tmp);
Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 210096)
+++ gcc/builtins.c	(working copy)
@@ -977,10 +977,10 @@
    emit_insn (gen_blockage ());
  }

-/* __builtin_longjmp is passed a pointer to an array of five words (not
-   all will be used on all machines).  It operates similarly to the C
-   library function of the same name, but is more efficient.  Much of
-   the code below is copied from the handling of non-local gotos.  */
+/* __builtin_longjmp is passed a pointer to an array of five word/pointer
+   sized entries, not all will be used on all machines.  It operates 
similarly
+   to the C library function of the same name, but is more efficient.  Much
+   of the code below is copied from the handling of non-local gotos.  */

  static void
  expand_builtin_longjmp (rtx buf_addr, rtx value)
@@ -1204,10 +1204,10 @@
    return const0_rtx;
  }

-/* __builtin_update_setjmp_buf is passed a pointer to an array of five 
words
-   (not all will be used on all machines) that was passed to 
__builtin_setjmp.
-   It updates the stack pointer in that block to correspond to the current
-   stack pointer.  */
+/* __builtin_update_setjmp_buf is passed a pointer to an array of five
+   entries (not all will be used on all machines) that was passed to
+   __builtin_setjmp.  It updates the stack pointer in that block to
+   correspond to the current stack pointer.  */

  static void
  expand_builtin_update_setjmp_buf (rtx buf_addr)
@@ -6205,8 +6205,8 @@
        gcc_unreachable ();

      case BUILT_IN_SETJMP_SETUP:
-      /* __builtin_setjmp_setup is passed a pointer to an array of five 
words
-          and the receiver label.  */
+      /* __builtin_setjmp_setup is passed a pointer to an array of five
+	 entries and the receiver label.  */
        if (validate_arglist (exp, POINTER_TYPE, POINTER_TYPE, VOID_TYPE))
  	{
  	  rtx buf_addr = expand_expr (CALL_EXPR_ARG (exp, 0), subtarget,
@@ -6239,9 +6239,9 @@
  	}
        break;

-      /* __builtin_longjmp is passed a pointer to an array of five words.
-	 It's similar to the C library longjmp function but works with
-	 __builtin_setjmp above.  */
+      /* __builtin_longjmp is passed a pointer to an array of five
+	 entries.  It's similar to the C library longjmp function
+	 but works with __builtin_setjmp above.  */
      case BUILT_IN_LONGJMP:
        if (validate_arglist (exp, POINTER_TYPE, INTEGER_TYPE, VOID_TYPE))
  	{
Index: gcc/doc/md.texi
===================================================================
--- gcc/doc/md.texi	(revision 210096)
+++ gcc/doc/md.texi	(working copy)
@@ -6112,10 +6112,15 @@
  A typical reason why you might need this pattern is if some value, such
  as a pointer to a global table, must be restored.  Though it is
  preferred that the pointer value be recalculated if possible (given the
-address of a label for instance).  The single argument is a pointer to
-the @code{jmp_buf}.  Note that the buffer is five words long and that
-the first three are normally used by the generic mechanism.
+address of a label for instance).

+The single argument is a pointer to the @code{jmp_buf}.  This buffer
+is big enough to hold five @code{word_mode} or @code{Pmode} sized
+entries, whichever is bigger.  The generic machanism uses just the
+first three entries, (to hold the frame pointer, return address and
+stack pointer respectively), but target specific code can use all five
+of them.
+
  @cindex @code{builtin_setjmp_receiver} instruction pattern
  @item @samp{builtin_setjmp_receiver}
  This pattern, if defined, contains code needed at the site of a

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Mike Stump]

On May 6, 2014, at 8:07 AM, Nicholas Clifton <nickc@redhat.com> wrote:
> +      tmp = size_int ((5 * MAX (POINTER_SIZE, BITS_PER_WORD)) / BITS_PER_WORD - 1);

This is not right.  The denominator should either be GET_MODE_SIZE (Pmode) or POINTER_SIZE.  See get_nl_goto_field for additional fun.

Also, the save area for the machine is exactly 2 * GET_MODE_SIZE (Pmode) in and the save area is exactly GET_MODE_SIZE (STACK_SAVEAREA_MODE (SAVE_NONLOCAL)) bits.  builtin_setjmp_setup is the only unaccounted for component.

mips is alone in having slop not accounted for in GET_MODE_SIZE (STACK_SAVEAREA_MODE (SAVE_NONLOCAL)).  Personally, I’d rather make the size calculation exact, rather than even more sloppy and hard to comprehend.

How about GET_MODE_SIZE (STACK_SAVEAREA_MODE (SAVE_NONLOCAL)) / GET_MODE_SIZE (Pmode) + 2 + /* slop for mips, see builtin_setjmp_setup */ 1 - 1.  This retains the slop for mips, and fixes ports like ia64 and s390 (see STACK_SAVEAREA_MODE on those ports, it is larger one might expect)?

Last patch that `broke’ this:

  http://gcc.gnu.org/ml/gcc-patches/2004-01/msg02000.html

That entire thread is interesting.

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Nicholas Clifton]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

Hi Mike,

> How about GET_MODE_SIZE (STACK_SAVEAREA_MODE (SAVE_NONLOCAL)) / GET_MODE_SIZE (Pmode) + 2 + /* slop for mips, see builtin_setjmp_setup */ 1 - 1.  This retains the slop for mips, and fixes ports like ia64 and s390 (see STACK_SAVEAREA_MODE on those ports, it is larger one might expect)?


OK - revised patch attached.  I have added a comment before the 
computation to explain each of the numbers, and adjusted the comments in 
the other files to match the new size of the jump buffer.

What do you think of this version ?

Cheers
   Nick




[-- Attachment #2: builtin-setjmp.patch.2 --]
[-- Type: application/x-troff-man, Size: 4656 bytes --]

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Mike Stump]

On May 8, 2014, at 7:24 AM, Nicholas Clifton <nickc@redhat.com> wrote:
> 
> What do you think of this version ?

Now we just need a __builtin_setjmp style of maintainer to review…

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Eric Botcazou]

> 2014-05-06  Nick Clifton  <nickc@redhat.com>
> 
> 	* except.c (init_eh): Fix computation of builtin setjmp buffer
> 	size.

That's the same patch as
  https://gcc.gnu.org/ml/gcc-patches/2011-11/msg00272.html
and is still incorrect.

-- 
Eric Botcazou

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Eric Botcazou]

> Now we just need a __builtin_setjmp style of maintainer to review…

Let's just do what I suggested in
  https://gcc.gnu.org/ml/gcc-patches/2011-11/msg00286.html

-- 
Eric Botcazou

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Nicholas Clifton]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

Hi Eric,

>> 2014-05-06  Nick Clifton  <nickc@redhat.com>
>>
>> 	* except.c (init_eh): Fix computation of builtin setjmp buffer
>> 	size.
>
> That's the same patch as
>    https://gcc.gnu.org/ml/gcc-patches/2011-11/msg00272.html
> and is still incorrect.

Ah - you are worried about the case where STACK_SAVEAREA_MODE() is 
smaller than Pmode, yes ?

OK then, how about this revised version of the patch where the size 
computation is now:

      tmp = size_int (MAX (GET_MODE_SIZE (STACK_SAVEAREA_MODE 
(SAVE_NONLOCAL))
			   / GET_MODE_SIZE (Pmode), 1)
		      + 2 /* Stack pointer and frame pointer.  */
		      + 1 /* Slop for mips.  */
		      - 1);

OK to apply ?

Cheers
   Nick


[-- Attachment #2: except.c.patch.4 --]
[-- Type: application/x-troff-man, Size: 4663 bytes --]

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Eric Botcazou]

> Ah - you are worried about the case where STACK_SAVEAREA_MODE() is
> smaller than Pmode, yes ?

No, simply that the modified formula is plain wrong.  The code does:

  tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
  tmp = build_index_type (tmp);
  tmp = build_array_type (ptr_type_node, tmp);

so, in the end, the size of the buffer is:

  [(5 * BITS_PER_WORD / POINTER_SIZE - 1) + 1] * POINTER_SIZE

which boilds down to:

  5 * BITS_PER_WORD

provided that POINTER_SIZE <= BITS_PER_WORD.


So we have a problem if POINTER_SIZE > BITS_PER_WORD, in which case it's 
sufficient to use 5 * POINTER_SIZE instead.

> OK then, how about this revised version of the patch where the size
> computation is now:
> 
>       tmp = size_int (MAX (GET_MODE_SIZE (STACK_SAVEAREA_MODE
> (SAVE_NONLOCAL))
> 			   / GET_MODE_SIZE (Pmode), 1)
> 		      + 2 /* Stack pointer and frame pointer.  */
> 		      + 1 /* Slop for mips.  */
> 		      - 1);
> 
> OK to apply ?

No, that's too complicated and risky, just do the following:

      /* builtin_setjmp takes a pointer to 5 words or pointers.  */
      if (POINTER_SIZE > BITS_PER_WORD)
	tmp = size_int (4);
      else
	tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);

which is simple and safe.

-- 
Eric Botcazou

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Mike Stump]

On May 15, 2014, at 12:55 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> No, that's too complicated and risky, just do the following:
> 
>      /* builtin_setjmp takes a pointer to 5 words or pointers.  */
>      if (POINTER_SIZE > BITS_PER_WORD)
> 	tmp = size_int (4);
>      else
> 	tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
> 
> which is simple and safe.

But, fails whenever the size of the mode of the save area is bigger than a certain amount…  On my port, the size taken up by the  save area is large enough to cause this to fail.  :-(  The correct bug fix would have my port not fail.

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Nicholas Clifton]

Hi Eric,

   OK - here is your version of the patch, extended with a comment which 
I think is helpful for other people reading the code, and with the 
changes to builtins.c and md.texi removed, since the size of the buffer 
is not changing.

   Is this version OK to apply ?

Cheers
   Nick

gcc/ChangeLog
2014-05-16  Nick Clifton  <nickc@redhat.com>

	* except.c (init_eh): Correct computation of the size of a builtin
	setjmp buffer for when pointers are bigger than words.


Index: gcc/except.c
===================================================================
--- gcc/except.c	(revision 210490)
+++ gcc/except.c	(working copy)
@@ -286,9 +286,22 @@
        tmp = size_int (FIRST_PSEUDO_REGISTER + 2 - 1);
  #endif
  #else
-      /* builtin_setjmp takes a pointer to 5 words.  */
-      tmp = size_int (5 * BITS_PER_WORD / POINTER_SIZE - 1);
+      /* Compute a minimally sized jump buffer.  We need room to store at
+	 least 3 pointers - stack pointer, frame pointer and return address.
+	 Plus for some targets we need room for an extra pointer - in the
+	 case of MIPS this is the global pointer.  This makes a total of four
+	 pointers, but to be safe we actually allocate room for 5.
+
+	 If pointers are smaller than words then we allocate enough room for
+	 5 words, just in case the backend needs this much room.  For more
+	 discussion on this issue see:
+	 http://gcc.gnu.org/ml/gcc-patches/2014-05/msg00313.html.  */
+      if (POINTER_SIZE > BITS_PER_WORD)
+	tmp = size_int (5 - 1);
+      else
+	tmp = size_int ((5 * BITS_PER_WORD / POINTER_SIZE) - 1);
  #endif
        tmp = build_index_type (tmp);
        tmp = build_array_type (ptr_type_node, tmp);

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Eric Botcazou]

> But, fails whenever the size of the mode of the save area is bigger than a
> certain amount…  On my port, the size taken up by the  save area is large
> enough to cause this to fail.  :-(

That's a bit unexpected, why do you need so big a save area exactly?  The only 
architecture for which this doesn't work is the IA-64, which is a very special 
beast...  In this case, the way out is to define DONT_USE_BUILTIN_SETJMP and 
JMP_BUF_SIZE to the needed size.

-- 
Eric Botcazou

Re: RFA: Fix calculation of size of builtin setjmp buffer

[Eric Botcazou]

>    OK - here is your version of the patch, extended with a comment which
> I think is helpful for other people reading the code, and with the
> changes to builtins.c and md.texi removed, since the size of the buffer
> is not changing.
> 
>    Is this version OK to apply ?

Yes, IMO that's fine, thanks.

-- 
Eric Botcazou