Message ID | 5368F2FD.9080604@redhat.com |
---|---|
State | New |
Headers | show |
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
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);