On Tue, May 6, 2014 at 4:34 PM, Nicholas Clifton <ni...@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 <ni...@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);
>
