RFA: Fix calculation of size of builtin setjmp buffer

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.

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);
>

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