This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: 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.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]