diff mbox

except.c: fix setjmp buffer size math

Message ID 201111030330.pA33USdS029597@greed.delorie.com
State New
Headers show

Commit Message

DJ Delorie Nov. 3, 2011, 3:30 a.m. UTC
The RL78 port has 8 bit registers and 16 bit pointers.  The existing
calculation resulted in a buffer that was too small for the five
values needed.

	* except.c (init_eh): Fix setjmp buffer size calculations for
	targets where pointers are not word-sized.

Comments

Eric Botcazou Nov. 3, 2011, 9:14 a.m. UTC | #1
> 	* except.c (init_eh): Fix setjmp buffer size calculations for
> 	targets where pointers are not word-sized.

This will shrink the buffer for most such targets though.

> Index: gcc/except.c
> ===================================================================
> --- gcc/except.c	(revision 180758)
> +++ gcc/except.c	(working copy)
> @@ -253,13 +253,13 @@ init_eh (void)
>  	 also tend to be larger than necessary for most systems, a more
>  	 optimal port will define JMP_BUF_SIZE.  */
>        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);
> +      tmp = size_int (5 * POINTER_SIZE / BITS_PER_WORD - 1);
>  #endif
>        tmp = build_index_type (tmp);
>        tmp = build_array_type (ptr_type_node, tmp);
>        f_jbuf = build_decl (BUILTINS_LOCATION,
>  			   FIELD_DECL, get_identifier ("__jbuf"), tmp);
>  #ifdef DONT_USE_BUILTIN_SETJMP

This seems hardly correct, especially if you look at the lines just below:

      tmp = build_index_type (tmp);
      tmp = build_array_type (ptr_type_node, tmp);


You probably want:

      /* 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);
Mike Stump Nov. 3, 2011, 4:24 p.m. UTC | #2
On Nov 3, 2011, at 2:14 AM, Eric Botcazou wrote:

>> 	* except.c (init_eh): Fix setjmp buffer size calculations for
>> 	targets where pointers are not word-sized.
> 
> This will shrink the buffer for most such targets though.

I prefer:

#ifndef TARGET_BUILTIN_JMP_BUF_SIZE
#define TARGET_BUILTIN_JMP_BUF_SIZE 5
#endif
      /* builtin_setjmp takes a pointer to TARGET_BUILTIN_JMP_BUF_SIZE words.  */
      tmp = build_int_cst (NULL_TREE, TARGET_BUILTIN_JMP_BUF_SIZE * BITS_PER_WORD / POINTER_SIZE - 1);

This doesn't change any existing port, which is nice, and on any port that needs a different number, they have a nice simply direct way to get it.  Because of the control afforded, I don't think we'd need to change it again.  I use this on my port, already, so it isn't theoretic.
DJ Delorie Nov. 3, 2011, 4:25 p.m. UTC | #3
If the comment is true, and it takes a pointer to five WORDS, why are
we using POINTER_SIZE at all?  Does it actually take a pointer to five
POINTERS?  If so, where did the '4' come from?

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

>      /* 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);
Eric Botcazou Nov. 3, 2011, 4:37 p.m. UTC | #4
> I prefer:
>
> #ifndef TARGET_BUILTIN_JMP_BUF_SIZE
> #define TARGET_BUILTIN_JMP_BUF_SIZE 5
> #endif
>       /* builtin_setjmp takes a pointer to TARGET_BUILTIN_JMP_BUF_SIZE
> words.  */ tmp = build_int_cst (NULL_TREE, TARGET_BUILTIN_JMP_BUF_SIZE *
> BITS_PER_WORD / POINTER_SIZE - 1);
>
> This doesn't change any existing port, which is nice, and on any port that
> needs a different number, they have a nice simply direct way to get it. 

That's a little confusing though, because JMP_BUF_SIZE just above comes with a 
different unit:

#ifdef JMP_BUF_SIZE
      tmp = size_int (JMP_BUF_SIZE - 1);
#else

so I'd do:

/* builtin_setjmp takes a pointer to 5 words by default.  */
#ifndef TARGET_BUILTIN_JMP_BUF_SIZE
#define TARGET_BUILTIN_JMP_BUF_SIZE (5 * BITS_PER_WORD / POINTER_SIZE)
#endif  TARGET_BUILTIN_JMP_BUF_SIZE

     tmp = size_int (TARGET_BUILTIN_JMP_BUF_SIZE - 1);

instead to be consistent.
Eric Botcazou Nov. 3, 2011, 4:47 p.m. UTC | #5
> If the comment is true, and it takes a pointer to five WORDS, why are
> we using POINTER_SIZE at all?

Because of the 2 lines just below:

       tmp = build_index_type (tmp);
       tmp = build_array_type (ptr_type_node, tmp);

> Does it actually take a pointer to five POINTERS?

No.

> If so, where did the '4' come from? 

5 * 1 - 1.
Joseph Myers Nov. 3, 2011, 8:46 p.m. UTC | #6
On Thu, 3 Nov 2011, Mike Stump wrote:

> On Nov 3, 2011, at 2:14 AM, Eric Botcazou wrote:
> 
> >> 	* except.c (init_eh): Fix setjmp buffer size calculations for
> >> 	targets where pointers are not word-sized.
> > 
> > This will shrink the buffer for most such targets though.
> 
> I prefer:
> 
> #ifndef TARGET_BUILTIN_JMP_BUF_SIZE
> #define TARGET_BUILTIN_JMP_BUF_SIZE 5
> #endif

Use hooks not macros for new target configuration, please.
diff mbox

Patch

Index: gcc/except.c
===================================================================
--- gcc/except.c	(revision 180758)
+++ gcc/except.c	(working copy)
@@ -253,13 +253,13 @@  init_eh (void)
 	 also tend to be larger than necessary for most systems, a more
 	 optimal port will define JMP_BUF_SIZE.  */
       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);
+      tmp = size_int (5 * POINTER_SIZE / BITS_PER_WORD - 1);
 #endif
       tmp = build_index_type (tmp);
       tmp = build_array_type (ptr_type_node, tmp);
       f_jbuf = build_decl (BUILTINS_LOCATION,
 			   FIELD_DECL, get_identifier ("__jbuf"), tmp);
 #ifdef DONT_USE_BUILTIN_SETJMP