This is the mail archive of the newlib@sourceware.org mailing list for the newlib 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: [PATCH] Fix undefined behaviour in lround,llround.


On 16/07/2010 19:14, Howland Craig D (Craig) wrote:

> (which would be nothing).  So I think that just one SAFE_SHIFT per file
> is needed and appropriate.

  Fair enough.  I also got tired of doing it in my head, so I went and
annotated the whole thing as well, in case anyone wants to check my working.

  (Another approach would be to replace "unsigned int tmp" with an unsigned
long long, or cast it to that when it gets shifted, but we could only do that
in llround, as newlib has to run on non-C99 systems so we can't assume the
availability of long long in any function that doesn't require them as
parameters.  So I took this approach because it works identically for both
files.  Also, allocating another DImode pseudo in the middle of all this
wouldn't do the register pressure any favours, particularly on x86.)

  Also, I could omit SAFE_LEFT_SHIFT from the header file since it's unused
now, but I didn't do so in this iteration; I'll wait and do it before
committing if this version gets approved.  Likewise if the annotations aren't
desired, I could remove them on commit.

  Same changelog as before.  OK now/with/without above-mentioned minor
alterations?

    cheers,
      DaveK


Index: newlib/libm/common/fdlibm.h
===================================================================
RCS file: /cvs/src/src/newlib/libm/common/fdlibm.h,v
retrieving revision 1.7
diff -p -u -r1.7 fdlibm.h
--- newlib/libm/common/fdlibm.h	17 Nov 2009 22:35:46 -0000	1.7
+++ newlib/libm/common/fdlibm.h	17 Jul 2010 16:42:35 -0000
@@ -361,3 +361,13 @@ do {								\
   sf_u.word = (i);						\
   (d) = sf_u.value;						\
 } while (0)
+
+/* Macros to avoid undefined behaviour that can arise if the amount
+   of a shift is exactly equal to the size of the shifted operand.  */
+
+#define SAFE_LEFT_SHIFT(op,amt)					\
+  (((amt) < 8 * sizeof(op)) ? ((op) << (amt)) : 0)
+
+#define SAFE_RIGHT_SHIFT(op,amt)				\
+  (((amt) < 8 * sizeof(op)) ? ((op) >> (amt)) : 0)
+
Index: newlib/libm/common/s_llround.c
===================================================================
RCS file: /cvs/src/src/newlib/libm/common/s_llround.c,v
retrieving revision 1.1
diff -p -u -r1.1 s_llround.c
--- newlib/libm/common/s_llround.c	25 Mar 2009 19:13:01 -0000	1.1
+++ newlib/libm/common/s_llround.c	17 Jul 2010 16:42:35 -0000
@@ -31,8 +31,10 @@ llround(double x)
   msw &= 0x000fffff;
   msw |= 0x00100000;
 
+  /* exponent_less_1023 in [-1024,1023] */
   if (exponent_less_1023 < 20)
     {
+      /* exponent_less_1023 in [-1024,19] */
       if (exponent_less_1023 < 0)
         {
           if (exponent_less_1023 < -1)
@@ -42,20 +44,34 @@ llround(double x)
         }
       else
         {
+          /* exponent_less_1023 in [0,19] */
+	  /* shift amt in [0,19] */
           msw += 0x80000 >> exponent_less_1023;
+	  /* shift amt in [20,1] */
           result = msw >> (20 - exponent_less_1023);
         }
     }
   else if (exponent_less_1023 < (8 * sizeof (long long int)) - 1)
     {
+      /* 64bit longlong: exponent_less_1023 in [20,62] */
       if (exponent_less_1023 >= 52)
-        result = ((long long int) msw << (exponent_less_1023 - 20)) | (lsw << (exponent_less_1023 - 52));
+	/* 64bit longlong: exponent_less_1023 in [52,62] */
+	/* 64bit longlong: shift amt in [32,42] */
+        result = ((long long int) msw << (exponent_less_1023 - 20))
+		    /* 64bit longlong: shift amt in [0,10] */
+                    | (lsw << (exponent_less_1023 - 52));
       else
         {
-          unsigned int tmp = lsw + (0x80000000 >> (exponent_less_1023 - 20));
+	  /* 64bit longlong: exponent_less_1023 in [20,51] */
+          unsigned int tmp = lsw
+		    /* 64bit longlong: shift amt in [0,31] */
+                    + (0x80000000 >> (exponent_less_1023 - 20));
           if (tmp < lsw)
             ++msw;
-          result = ((long long int) msw << (exponent_less_1023 - 20)) | (tmp >> (52 - exponent_less_1023));
+	  /* 64bit longlong: shift amt in [0,31] */
+          result = ((long long int) msw << (exponent_less_1023 - 20))
+		    /* ***64bit longlong: shift amt in [32,1] */
+                    | SAFE_RIGHT_SHIFT (tmp, (52 - exponent_less_1023));
         }
     }
   else
Index: newlib/libm/common/s_lround.c
===================================================================
RCS file: /cvs/src/src/newlib/libm/common/s_lround.c,v
retrieving revision 1.2
diff -p -u -r1.2 s_lround.c
--- newlib/libm/common/s_lround.c	25 Mar 2009 19:13:01 -0000	1.2
+++ newlib/libm/common/s_lround.c	17 Jul 2010 16:42:35 -0000
@@ -71,9 +71,10 @@ ANSI C, POSIX
   exponent_less_1023 = ((msw & 0x7ff00000) >> 20) - 1023;
   msw &= 0x000fffff;
   msw |= 0x00100000;
-
+  /* exponent_less_1023 in [-1024,1023] */
   if (exponent_less_1023 < 20)
     {
+      /* exponent_less_1023 in [-1024,19] */
       if (exponent_less_1023 < 0)
         {
           if (exponent_less_1023 < -1)
@@ -83,20 +84,39 @@ ANSI C, POSIX
         }
       else
         {
+          /* exponent_less_1023 in [0,19] */
+	  /* shift amt in [0,19] */
           msw += 0x80000 >> exponent_less_1023;
+	  /* shift amt in [20,1] */
           result = msw >> (20 - exponent_less_1023);
         }
     }
   else if (exponent_less_1023 < (8 * sizeof (long int)) - 1)
     {
+      /* 32bit long: exponent_less_1023 in [20,30] */
+      /* 64bit long: exponent_less_1023 in [20,62] */
       if (exponent_less_1023 >= 52)
-        result = ((long int) msw << (exponent_less_1023 - 20)) | (lsw << (exponent_less_1023 - 52));
+	/* 64bit long: exponent_less_1023 in [52,62] */
+	/* 64bit long: shift amt in [32,42] */
+        result = ((long int) msw << (exponent_less_1023 - 20))
+		/* 64bit long: shift amt in [0,10] */
+                | (lsw << (exponent_less_1023 - 52));
       else
         {
-          unsigned int tmp = lsw + (0x80000000 >> (exponent_less_1023 - 20));
+	  /* 32bit long: exponent_less_1023 in [20,30] */
+	  /* 64bit long: exponent_less_1023 in [20,51] */
+          unsigned int tmp = lsw
+		    /* 32bit long: shift amt in [0,10] */
+		    /* 64bit long: shift amt in [0,31] */
+                    + (0x80000000 >> (exponent_less_1023 - 20));
           if (tmp < lsw)
             ++msw;
-          result = ((long int) msw << (exponent_less_1023 - 20)) | (tmp >> (52 - exponent_less_1023));
+	  /* 32bit long: shift amt in [0,10] */
+	  /* 64bit long: shift amt in [0,31] */
+          result = ((long int) msw << (exponent_less_1023 - 20))
+		    /* ***32bit long: shift amt in [32,22] */
+		    /* ***64bit long: shift amt in [32,1] */
+                    | SAFE_RIGHT_SHIFT (tmp, (52 - exponent_less_1023));
         }
     }
   else

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