[PATCH] fix jump-misses-init warnings

[PATCH] fix jump-misses-init warnings

[Giuseppe Scrivano]

Hello!

I reported some problems compiling guile with new versions of GCC.  The
reason of these problems is that new GCC versions check for declaration
in the current scope that are missed by a goto.

This is the GCC inline code describing the jump-misses-init warning:

  /* If there are any decls in label_vars->decls_in_scope, then this
     goto has missed the declaration of the decl.  This happens for a
     case like
       int i = 1;
      lab:
       ...
       goto lab;
     Issue a warning or error.  */

It is exactly the pattern present in some files, causing the compiling
failure because warnings are treated as errors:

In file included from eval.c:4148:0:
eval.i.c: In function ‘deval’:
eval.i.c:1104:2: error: jump skips variable initialization
eval.i.c:1043:6: note: label ‘handle_a_macro’ defined here
eval.i.c:1011:8: note: ‘orig_sym’ declared here


My proposal is to move such variables causing this warning to the upper
scope as in the attached patch.

I have checked if my changes introduced regressions ("make check"), but
all tests were completed successfully.

Cheers,
Giuseppe



From 234fc0860ba35a7e5da714586bdf4b9244e8dffc Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano@gnu.org>
Date: Fri, 4 Sep 2009 16:22:07 +0200
Subject: [PATCH] Avoid the jump-misses-init compiler warning

* libguile/conv-uinteger.i.c (SCM_TO_TYPE_PROTO): Move the declaration
of `n' to the upper scope and rename other variables with the same name.

* libguile/eval.i.c (dispatch): Do the same with `orig_sym'.
(SCM_GPROC1): Do the same with `xx'.
(scm_max): Likewise.
(scm_min): Likewise.
(scm_i_divide): Likewise.

* srfi/srfi-1.c (scm_srfi1_filter_map): Do the same with `proc_tramp'.
(scm_srfi1_list_index): Do the same with `pred_tramp'.
---
 libguile/conv-uinteger.i.c |   27 ++++++++++---------
 libguile/eval.i.c          |    3 +-
 libguile/numbers.c         |   60 +++++++++++++++++++++++--------------------
 srfi/srfi-1.c              |   20 ++++++++------
 4 files changed, 59 insertions(+), 51 deletions(-)

diff --git a/libguile/conv-uinteger.i.c b/libguile/conv-uinteger.i.c
index 52f49f7..b08fdab 100644
--- a/libguile/conv-uinteger.i.c
+++ b/libguile/conv-uinteger.i.c
@@ -26,9 +26,10 @@
 TYPE
 SCM_TO_TYPE_PROTO (SCM val)
 {
+  scm_t_signed_bits n;
   if (SCM_I_INUMP (val))
     {
-      scm_t_signed_bits n = SCM_I_INUM (val);
+      n = SCM_I_INUM (val);
       if (n >= 0
 	  && ((scm_t_uintmax)n) >= TYPE_MIN && ((scm_t_uintmax)n) <= TYPE_MAX)
 	return n;
@@ -49,17 +50,17 @@ SCM_TO_TYPE_PROTO (SCM val)
 	{
 	  if (mpz_fits_ulong_p (SCM_I_BIG_MPZ (val)))
 	    {
-	      unsigned long n = mpz_get_ui (SCM_I_BIG_MPZ (val));
+	      unsigned long nn = mpz_get_ui (SCM_I_BIG_MPZ (val));
 #if SIZEOF_TYPE != 0 && SIZEOF_TYPE > SCM_SIZEOF_LONG
-	      return n;
+	      return nn;
 #else
 
 #if TYPE_MIN == 0 
-              if (n <= TYPE_MAX)
-                return n;
+              if (nn <= TYPE_MAX)
+                return nn;
 #else /* TYPE_MIN != 0 */
-              if (n >= TYPE_MIN && n <= TYPE_MAX)
-                return n;
+              if (nn >= TYPE_MIN && nn <= TYPE_MAX)
+                return nn;
 #endif /* TYPE_MIN != 0 */
               else
                 goto out_of_range;
@@ -71,7 +72,7 @@ SCM_TO_TYPE_PROTO (SCM val)
 	}
       else
 	{
-	  scm_t_uintmax n;
+	  scm_t_uintmax nn;
 	  size_t count;
 
 	  if (mpz_sgn (SCM_I_BIG_MPZ (val)) < 0)
@@ -81,14 +82,14 @@ SCM_TO_TYPE_PROTO (SCM val)
 	      > CHAR_BIT*sizeof (TYPE))
 	    goto out_of_range;
 	  
-	  mpz_export (&n, &count, 1, sizeof (TYPE), 0, 0, SCM_I_BIG_MPZ (val));
+	  mpz_export (&nn, &count, 1, sizeof (TYPE), 0, 0, SCM_I_BIG_MPZ (val));
 
 #if TYPE_MIN == 0
-	  if (n <= TYPE_MAX)
-	    return n;
+	  if (nn <= TYPE_MAX)
+	    return nn;
 #else /* TYPE_MIN != 0 */
-	  if (n >= TYPE_MIN && n <= TYPE_MAX)
-	    return n;
+	  if (nn >= TYPE_MIN && nn <= TYPE_MAX)
+	    return nn;
 #endif /* TYPE_MIN != 0 */
           else
             goto out_of_range;
diff --git a/libguile/eval.i.c b/libguile/eval.i.c
index 25abf6c..d0f669d 100644
--- a/libguile/eval.i.c
+++ b/libguile/eval.i.c
@@ -1000,6 +1000,7 @@ dispatch:
     }
   else
     {
+      SCM orig_sym;
       if (SCM_VARIABLEP (SCM_CAR (x)))
         proc = SCM_VARIABLE_REF (SCM_CAR (x));
       else if (SCM_ILOCP (SCM_CAR (x)))
@@ -1008,7 +1009,7 @@ dispatch:
 	proc = CEVAL (SCM_CAR (x), env);
       else if (scm_is_symbol (SCM_CAR (x)))
 	{
-	  SCM orig_sym = SCM_CAR (x);
+	  orig_sym = SCM_CAR (x);
 	  {
 	    SCM *location = scm_lookupcar1 (x, env, 1);
 	    if (location == NULL)
diff --git a/libguile/numbers.c b/libguile/numbers.c
index 5812576..bd3d2c0 100644
--- a/libguile/numbers.c
+++ b/libguile/numbers.c
@@ -3460,10 +3460,11 @@ SCM_GPROC1 (s_less_p, "<", scm_tc7_rpsubr, scm_less_p, g_less_p);
 SCM
 scm_less_p (SCM x, SCM y)
 {
+  long xx;
  again:
   if (SCM_I_INUMP (x))
     {
-      long xx = SCM_I_INUM (x);
+      xx = SCM_I_INUM (x);
       if (SCM_I_INUMP (y))
 	{
 	  long yy = SCM_I_INUM (y);
@@ -3723,6 +3724,7 @@ SCM_GPROC1 (s_max, "max", scm_tc7_asubr, scm_max, g_max);
 SCM
 scm_max (SCM x, SCM y)
 {
+  long xx;
   if (SCM_UNBNDP (y))
     {
       if (SCM_UNBNDP (x))
@@ -3735,7 +3737,7 @@ scm_max (SCM x, SCM y)
   
   if (SCM_I_INUMP (x))
     {
-      long xx = SCM_I_INUM (x);
+      xx = SCM_I_INUM (x);
       if (SCM_I_INUMP (y))
 	{
 	  long yy = SCM_I_INUM (y);
@@ -3777,12 +3779,12 @@ scm_max (SCM x, SCM y)
 	}
       else if (SCM_REALP (y))
 	{
-          /* if y==NaN then xx>yy is false, so we return the NaN y */
-          double xx, yy;
+          /* if y==NaN then xxd>yyd is false, so we return the NaN y */
+          double xxd, yyd;
         big_real:
-          xx = scm_i_big2dbl (x);
-          yy = SCM_REAL_VALUE (y);
-	  return (xx > yy ? scm_from_double (xx) : y);
+          xxd = scm_i_big2dbl (x);
+          yyd = SCM_REAL_VALUE (y);
+	  return (xxd > yyd ? scm_from_double (xxd) : y);
 	}
       else if (SCM_FRACTIONP (y))
 	{
@@ -3810,14 +3812,14 @@ scm_max (SCM x, SCM y)
 	     if y==NaN then ">" is false and we return NaN
 	     calling isnan is unavoidable, since it's the only way to know
 	     which of x or y causes any compares to be false */
-	  double xx = SCM_REAL_VALUE (x);
-	  return (xisnan (xx) || xx > SCM_REAL_VALUE (y)) ? x : y;
+	  double xxd = SCM_REAL_VALUE (x);
+	  return (xisnan (xxd) || xxd > SCM_REAL_VALUE (y)) ? x : y;
 	}
       else if (SCM_FRACTIONP (y))
 	{
-	  double yy = scm_i_fraction2double (y);
-	  double xx = SCM_REAL_VALUE (x);
-	  return (xx < yy) ? scm_from_double (yy) : x;
+	  double yyd = scm_i_fraction2double (y);
+	  double xxd = SCM_REAL_VALUE (x);
+	  return (xxd < yyd) ? scm_from_double (yyd) : x;
 	}
       else
 	SCM_WTA_DISPATCH_2 (g_max, x, y, SCM_ARGn, s_max);
@@ -3834,8 +3836,8 @@ scm_max (SCM x, SCM y)
 	}
       else if (SCM_REALP (y))
 	{
-	  double xx = scm_i_fraction2double (x);
-	  return (xx < SCM_REAL_VALUE (y)) ? y : scm_from_double (xx);
+	  double xxd = scm_i_fraction2double (x);
+	  return (xxd < SCM_REAL_VALUE (y)) ? y : scm_from_double (xxd);
 	}
       else if (SCM_FRACTIONP (y))
 	{
@@ -3855,6 +3857,7 @@ SCM_GPROC1 (s_min, "min", scm_tc7_asubr, scm_min, g_min);
 SCM
 scm_min (SCM x, SCM y)
 {
+  long xx;
   if (SCM_UNBNDP (y))
     {
       if (SCM_UNBNDP (x))
@@ -3867,7 +3870,7 @@ scm_min (SCM x, SCM y)
   
   if (SCM_I_INUMP (x))
     {
-      long xx = SCM_I_INUM (x);
+      xx = SCM_I_INUM (x);
       if (SCM_I_INUMP (y))
 	{
 	  long yy = SCM_I_INUM (y);
@@ -3909,12 +3912,12 @@ scm_min (SCM x, SCM y)
 	}
       else if (SCM_REALP (y))
 	{
-          /* if y==NaN then xx<yy is false, so we return the NaN y */
-          double xx, yy;
+          /* if y==NaN then xxd<yyd is false, so we return the NaN y */
+          double xxd, yyd;
         big_real:
-          xx = scm_i_big2dbl (x);
-          yy = SCM_REAL_VALUE (y);
-	  return (xx < yy ? scm_from_double (xx) : y);
+          xxd = scm_i_big2dbl (x);
+          yyd = SCM_REAL_VALUE (y);
+	  return (xxd < yyd ? scm_from_double (xxd) : y);
 	}
       else if (SCM_FRACTIONP (y))
 	{
@@ -3942,14 +3945,14 @@ scm_min (SCM x, SCM y)
 	     if y==NaN then "<" is false and we return NaN
 	     calling isnan is unavoidable, since it's the only way to know
 	     which of x or y causes any compares to be false */
-	  double xx = SCM_REAL_VALUE (x);
-	  return (xisnan (xx) || xx < SCM_REAL_VALUE (y)) ? x : y;
+	  double xxd = SCM_REAL_VALUE (x);
+	  return (xisnan (xxd) || xxd < SCM_REAL_VALUE (y)) ? x : y;
 	}
       else if (SCM_FRACTIONP (y))
 	{
-	  double yy = scm_i_fraction2double (y);
-	  double xx = SCM_REAL_VALUE (x);
-	  return (yy < xx) ? scm_from_double (yy) : x;
+	  double yyd = scm_i_fraction2double (y);
+	  double xxd = SCM_REAL_VALUE (x);
+	  return (yyd < xxd) ? scm_from_double (yyd) : x;
 	}
       else
 	SCM_WTA_DISPATCH_2 (g_min, x, y, SCM_ARGn, s_min);
@@ -3966,8 +3969,8 @@ scm_min (SCM x, SCM y)
 	}
       else if (SCM_REALP (y))
 	{
-	  double xx = scm_i_fraction2double (x);
-	  return (SCM_REAL_VALUE (y) < xx) ? y : scm_from_double (xx);
+	  double xxd = scm_i_fraction2double (x);
+	  return (SCM_REAL_VALUE (y) < xxd) ? y : scm_from_double (xxd);
 	}
       else if (SCM_FRACTIONP (y))
 	{
@@ -4648,6 +4651,7 @@ static SCM
 scm_i_divide (SCM x, SCM y, int inexact)
 {
   double a;
+  long xx;
 
   if (SCM_UNLIKELY (SCM_UNBNDP (y)))
     {
@@ -4711,7 +4715,7 @@ scm_i_divide (SCM x, SCM y, int inexact)
 
   if (SCM_LIKELY (SCM_I_INUMP (x)))
     {
-      long xx = SCM_I_INUM (x);
+      xx = SCM_I_INUM (x);
       if (SCM_LIKELY (SCM_I_INUMP (y)))
 	{
 	  long yy = SCM_I_INUM (y);
diff --git a/srfi/srfi-1.c b/srfi/srfi-1.c
index 02f46fc..5ef35f0 100644
--- a/srfi/srfi-1.c
+++ b/srfi/srfi-1.c
@@ -809,6 +809,7 @@ SCM_DEFINE (scm_srfi1_filter_map, "filter-map", 2, 0, 1,
 {
   SCM  ret, *loc, elem, newcell, lst;
   int  argnum;
+  scm_t_trampoline_1 proc_tramp;
 
   SCM_VALIDATE_REST_ARGUMENT (rest);
 
@@ -818,7 +819,7 @@ SCM_DEFINE (scm_srfi1_filter_map, "filter-map", 2, 0, 1,
   if (scm_is_null (rest))
     {
       /* one list */
-      scm_t_trampoline_1 proc_tramp = scm_trampoline_1 (proc);
+      proc_tramp = scm_trampoline_1 (proc);
       SCM_ASSERT (proc_tramp, proc, SCM_ARG1, FUNC_NAME);
 
       for ( ; scm_is_pair (list1); list1 = SCM_CDR (list1))
@@ -840,9 +841,9 @@ SCM_DEFINE (scm_srfi1_filter_map, "filter-map", 2, 0, 1,
   else if (scm_is_null (SCM_CDR (rest)))
     {
       /* two lists */
-      scm_t_trampoline_2 proc_tramp = scm_trampoline_2 (proc);
+      scm_t_trampoline_2 proc_tramp2 = scm_trampoline_2 (proc);
       SCM list2 = SCM_CAR (rest);
-      SCM_ASSERT (proc_tramp, proc, SCM_ARG1, FUNC_NAME);
+      SCM_ASSERT (proc_tramp2, proc, SCM_ARG1, FUNC_NAME);
 
       for (;;)
         {
@@ -854,7 +855,7 @@ SCM_DEFINE (scm_srfi1_filter_map, "filter-map", 2, 0, 1,
               argnum = 3;
               goto check_lst_and_done;
             }
-          elem = proc_tramp (proc, SCM_CAR (list1), SCM_CAR (list2));
+          elem = proc_tramp2 (proc, SCM_CAR (list1), SCM_CAR (list2));
           if (scm_is_true (elem))
             {
               newcell = scm_cons (elem, SCM_EOL);
@@ -1112,12 +1113,13 @@ SCM_DEFINE (scm_srfi1_list_index, "list-index", 2, 0, 1,
   long  n = 0;
   SCM   lst;
   int   argnum;
+  scm_t_trampoline_1 pred_tramp;
   SCM_VALIDATE_REST_ARGUMENT (rest);
 
   if (scm_is_null (rest))
     {
       /* one list */
-      scm_t_trampoline_1 pred_tramp = scm_trampoline_1 (pred);
+      pred_tramp = scm_trampoline_1 (pred);
       SCM_ASSERT (pred_tramp, pred, SCM_ARG1, FUNC_NAME);
 
       for ( ; scm_is_pair (list1); n++, list1 = SCM_CDR (list1))
@@ -1133,8 +1135,8 @@ SCM_DEFINE (scm_srfi1_list_index, "list-index", 2, 0, 1,
     {
       /* two lists */
       SCM list2 = SCM_CAR (rest);
-      scm_t_trampoline_2 pred_tramp = scm_trampoline_2 (pred);
-      SCM_ASSERT (pred_tramp, pred, SCM_ARG1, FUNC_NAME);
+      scm_t_trampoline_2 pred_tramp2 = scm_trampoline_2 (pred);
+      SCM_ASSERT (pred_tramp2, pred, SCM_ARG1, FUNC_NAME);
 
       for ( ; ; n++)
         {
@@ -1146,8 +1148,8 @@ SCM_DEFINE (scm_srfi1_list_index, "list-index", 2, 0, 1,
               argnum = 3;
               break;
             }
-          if (scm_is_true (pred_tramp (pred,
-                                       SCM_CAR (list1), SCM_CAR (list2))))
+          if (scm_is_true (pred_tramp2 (pred,
+					SCM_CAR (list1), SCM_CAR (list2))))
             return SCM_I_MAKINUM (n);
 
           list1 = SCM_CDR (list1);
-- 
1.6.3.3

Re: [PATCH] fix jump-misses-init warnings

[Ludovic Courtès]

Hi Giuseppe,

Sorry for the late reply.

Giuseppe Scrivano <gscrivano@gnu.org> writes:

> I reported some problems compiling guile with new versions of GCC.  The
> reason of these problems is that new GCC versions check for declaration
> in the current scope that are missed by a goto.

Which version is that?  Is the warning enabled by ‘-Wall’?  I’m using
4.4.1, which is the latest released version AIUI, without having this
problem.

> --- a/libguile/conv-uinteger.i.c
> +++ b/libguile/conv-uinteger.i.c
> @@ -26,9 +26,10 @@
>  TYPE
>  SCM_TO_TYPE_PROTO (SCM val)
>  {
> +  scm_t_signed_bits n;
>    if (SCM_I_INUMP (val))
>      {
> -      scm_t_signed_bits n = SCM_I_INUM (val);
> +      n = SCM_I_INUM (val);
>        if (n >= 0
>  	  && ((scm_t_uintmax)n) >= TYPE_MIN && ((scm_t_uintmax)n) <= TYPE_MAX)
>  	return n;

Why does it help to move locals to the enclosing scope?  I’d expect the
opposite.

Thanks for the report!

Ludo’.

Re: [PATCH] fix jump-misses-init warnings

[Ken Raeburn]

On Sep 14, 2009, at 03:37, Ludovic Courtès wrote:
> Why does it help to move locals to the enclosing scope?  I’d expect  
> the
> opposite.

Jumping over the initializer is a simpler analysis than used-before- 
set.  It may also yield different results, if the "goto" that branches  
into the block and bypasses the initializer can now be reached through  
a path that retains a value of the variable in question stored by a  
previous pass through that block entering at the top.  You could still  
warn "possibly used before set", unless that's the only way to get to  
that goto statement.

If the logic of the function requires recalculating the value on any  
entry to the block, moving the declaration to an outer block is just  
masking the bug, not fixing it.

In this case, though, nothing after the label uses the value of the  
variable for which the initialization is bypassed, so it seems okay.   
Though, I'd think the declaration could be left in the same scope, and  
just split into a declaration without initializer and a separate  
assignment statement; that would probably suppress the warning without  
triggering a used-before-set warning, and wouldn't require renaming  
all the other "n" variables.  (If it doesn't, I think it might be a  
smaller change to rename the variable for which the declaration is  
being moved, rather than all the others.)

Ken

Re: [PATCH] fix jump-misses-init warnings

[Ludovic Courtès]

Hi,

Ken Raeburn <raeburn@raeburn.org> writes:

> In this case, though, nothing after the label uses the value of the
> variable for which the initialization is bypassed, so it seems okay.
> Though, I'd think the declaration could be left in the same scope, and
> just split into a declaration without initializer and a separate
> assignment statement; that would probably suppress the warning without
> triggering a used-before-set warning, and wouldn't require renaming
> all the other "n" variables.  (If it doesn't, I think it might be a
> smaller change to rename the variable for which the declaration is
> being moved, rather than all the others.)

I would prefer it.  Giuseppe: can you check whether it works for you?

Thanks,
Ludo’.