[RFA:] Type-punning through casts in strtod.c "miscompiled" by gcc trunk
Jeff Johnston
jjohnstn@redhat.com
Mon Jun 30 03:55:00 GMT 2008
Hans-Peter Nilsson wrote:
> See <http://gcc.gnu.org/ml/gcc-patches/2008-06/msg01473.html>,
> the message itself and the referred gcc PR. Here's the referred
> fix. Assuming of course, that the reply from the gcc judges to
> the RFC part will be "nay, that cast will not be blessed;
> gcc-4.4 and possibly gcc-4.3.2 will have its way with it".
>
> While preparing the _strtod_r fix, I noticed another
> aliasing-flawed code in mprec.h, the Storeinc implementation, so
> I included a fix for that. I haven't looked further...
>
> I tried to stay close to the existing code to avoid problems
> with future bugfix-imports, else I'd change to just use the
> double_union and its accessors (located right above "U") instead
> of "U".
>
> Alternatively, we could change to compile newlib with
> -fno-strict-aliasing. ;-) At any rate, passing
> -Wstrict-aliasing=2 could have caught this and possibly other
> similar bugs; I haven't checked if there are more. Level 3
> doesn't catch this one, arguably a gcc limitation (refer to the
> documentation).
>
> Tested cross to cris-elf, crisv32-elf and mmix-knuth-mmixware
> using the gcc testsuites.
>
> Ok to commit?
>
Yes, go ahead.
Thanks,
-- Jeff J.
> 2008-06-24 Hans-Peter Nilsson <hp@axis.com>
>
> Fix strict-aliasing issues with _strtod_r and Storeinc.
> * libc/stdlib/strtod.c (_strtod_r): Change local variables aadj,
> rv, rv0 from double to type U. Use accessor macros dval, dword0
> and dword1 for all accesses except for the ULtod call, where rv.i
> replaces the pointer cast.
> * libc/stdlib/mprec.h (U): Rename member L to i for easier re-use
> of access macros. Tweak comment.
> Remove #ifdef'd YES_ALIAS code.
> (dword0, dword1, dval): Define in terms of uncast union member
> access. Ditto for _DOUBLE_IS_32BITS variants.
> (Storeinc): Replace aliasing-flawed microoptimized definition with
> alternative suggested in comment. Remove now stale comment.
>
> Index: libc/stdlib/mprec.h
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/mprec.h,v
> retrieving revision 1.7
> diff -u -p -r1.7 mprec.h
> --- libc/stdlib/mprec.h 31 Aug 2007 21:21:27 -0000 1.7
> +++ libc/stdlib/mprec.h 24 Jun 2008 02:14:23 -0000
> @@ -78,31 +78,18 @@ union double_union
> #define word1(x) (x.i[1])
> #endif
>
> -
> -/* The following is taken from gdtoaimp.h for use with new strtod. */
> +/* The following is taken from gdtoaimp.h for use with new strtod, but
> + adjusted to avoid invalid type-punning. */
> typedef __int32_t Long;
> -typedef union { double d; __ULong L[2]; } U;
> -
> -#ifdef YES_ALIAS
> -#define dval(x) x
> -#ifdef IEEE_8087
> -#define dword0(x) ((__ULong *)&x)[1]
> -#define dword1(x) ((__ULong *)&x)[0]
> -#else
> -#define dword0(x) ((__ULong *)&x)[0]
> -#define dword1(x) ((__ULong *)&x)[1]
> -#endif
> -#else /* !YES_ALIAS */
> -#ifdef IEEE_8087
> -#define dword0(x) ((U*)&x)->L[1]
> -#define dword1(x) ((U*)&x)->L[0]
> -#else
> -#define dword0(x) ((U*)&x)->L[0]
> -#define dword1(x) ((U*)&x)->L[1]
> -#endif
> -#define dval(x) ((U*)&x)->d
> -#endif /* YES_ALIAS */
>
> +/* Unfortunately, because __ULong might be a different type than
> + __uint32_t, we can't re-use union double_union as-is without
> + further edits in strtod.c. */
> +typedef union { double d; __ULong i[2]; } U;
> +
> +#define dword0(x) word0(x)
> +#define dword1(x) word1(x)
> +#define dval(x) (x.d)
>
> #undef SI
> #ifdef Sudden_Underflow
> @@ -111,17 +98,7 @@ typedef union { double d; __ULong L[2];
> #define SI 0
> #endif
>
> -/* The following definition of Storeinc is appropriate for MIPS processors.
> - * An alternative that might be better on some machines is
> - * #define Storeinc(a,b,c) (*a++ = b << 16 | c & 0xffff)
> - */
> -#if defined (__IEEE_BYTES_LITTLE_ENDIAN) + defined (IEEE_8087) + defined (VAX)
> -#define Storeinc(a,b,c) (((unsigned short *)a)[1] = (unsigned short)b, \
> -((unsigned short *)a)[0] = (unsigned short)c, a++)
> -#else
> -#define Storeinc(a,b,c) (((unsigned short *)a)[0] = (unsigned short)b, \
> -((unsigned short *)a)[1] = (unsigned short)c, a++)
> -#endif
> +#define Storeinc(a,b,c) (*(a)++ = (b) << 16 | (c) & 0xffff)
>
> /* #define P DBL_MANT_DIG */
> /* Ten_pmax = floor(P*log(2)/log(5)) */
> @@ -167,11 +144,7 @@ typedef union { double d; __ULong L[2];
>
> #define word0(x) (x.i[0])
> #define word1(x) 0
> -#ifdef YES_ALIAS
> -#define dword0(x) ((__ULong *)&x)[0]
> -#else
> -#define dword0(x) ((U*)&x)->L[0]
> -#endif
> +#define dword0(x) word0(x)
> #define dword1(x) 0
> #else
>
> Index: libc/stdlib/strtod.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/strtod.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 strtod.c
> --- libc/stdlib/strtod.c 21 Feb 2008 17:14:14 -0000 1.10
> +++ libc/stdlib/strtod.c 24 Jun 2008 02:14:23 -0000
> @@ -218,7 +218,8 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> int bb2, bb5, bbe, bd2, bd5, bbbits, bs2, c, decpt, dsign,
> e, e1, esign, i, j, k, nd, nd0, nf, nz, nz0, sign;
> _CONST char *s, *s0, *s1;
> - double aadj, aadj1, adj, rv, rv0;
> + double aadj, adj;
> + U aadj1, rv, rv0;
> Long L;
> __ULong y, z;
> _Bigint *bb, *bb1, *bd, *bd0, *bs, *delta;
> @@ -286,7 +287,7 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> copybits(bits, fpi.nbits, bb);
> Bfree(ptr,bb);
> }
> - ULtod(((U*)&rv)->L, bits, exp, i);
> + ULtod(rv.i, bits, exp, i);
> }}
> goto ret;
> }
> @@ -469,7 +470,7 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> #ifdef Honor_FLT_ROUNDS
> /* round correctly FLT_ROUNDS = 2 or 3 */
> if (sign) {
> - rv = -rv;
> + dval(rv) = -dval(rv);
> sign = 0;
> }
> #endif
> @@ -485,7 +486,7 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> #ifdef Honor_FLT_ROUNDS
> /* round correctly FLT_ROUNDS = 2 or 3 */
> if (sign) {
> - rv = -rv;
> + dval(rv) = -dval(rv);
> sign = 0;
> }
> #endif
> @@ -513,7 +514,7 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> #ifdef Honor_FLT_ROUNDS
> /* round correctly FLT_ROUNDS = 2 or 3 */
> if (sign) {
> - rv = -rv;
> + dval(rv) = -dval(rv);
> sign = 0;
> }
> #endif
> @@ -976,14 +977,14 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> }
> if ((aadj = ratio(delta, bs)) <= 2.) {
> if (dsign)
> - aadj = aadj1 = 1.;
> + aadj = dval(aadj1) = 1.;
> else if (dword1(rv) || dword0(rv) & Bndry_mask) {
> #ifndef Sudden_Underflow
> if (dword1(rv) == Tiny1 && !dword0(rv))
> goto undfl;
> #endif
> aadj = 1.;
> - aadj1 = -1.;
> + dval(aadj1) = -1.;
> }
> else {
> /* special case -- power of FLT_RADIX to be */
> @@ -993,24 +994,24 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> aadj = 1./FLT_RADIX;
> else
> aadj *= 0.5;
> - aadj1 = -aadj;
> + dval(aadj1) = -aadj;
> }
> }
> else {
> aadj *= 0.5;
> - aadj1 = dsign ? aadj : -aadj;
> + dval(aadj1) = dsign ? aadj : -aadj;
> #ifdef Check_FLT_ROUNDS
> switch(Rounding) {
> case 2: /* towards +infinity */
> - aadj1 -= 0.5;
> + dval(aadj1) -= 0.5;
> break;
> case 0: /* towards 0 */
> case 3: /* towards -infinity */
> - aadj1 += 0.5;
> + dval(aadj1) += 0.5;
> }
> #else
> if (Flt_Rounds == 0)
> - aadj1 += 0.5;
> + dval(aadj1) += 0.5;
> #endif /*Check_FLT_ROUNDS*/
> }
> y = dword0(rv) & Exp_mask;
> @@ -1020,7 +1021,7 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> if (y == Exp_msk1*(DBL_MAX_EXP+Bias-1)) {
> dval(rv0) = dval(rv);
> dword0(rv) -= P*Exp_msk1;
> - adj = aadj1 * ulp(dval(rv));
> + adj = dval(aadj1) * ulp(dval(rv));
> dval(rv) += adj;
> if ((dword0(rv) & Exp_mask) >=
> Exp_msk1*(DBL_MAX_EXP+Bias-P)) {
> @@ -1042,18 +1043,18 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> if ((z = aadj) <= 0)
> z = 1;
> aadj = z;
> - aadj1 = dsign ? aadj : -aadj;
> + dval(aadj1) = dsign ? aadj : -aadj;
> }
> dword0(aadj1) += (2*P+1)*Exp_msk1 - y;
> }
> - adj = aadj1 * ulp(dval(rv));
> + adj = dval(aadj1) * ulp(dval(rv));
> dval(rv) += adj;
> #else
> #ifdef Sudden_Underflow
> if ((dword0(rv) & Exp_mask) <= P*Exp_msk1) {
> dval(rv0) = dval(rv);
> dword0(rv) += P*Exp_msk1;
> - adj = aadj1 * ulp(dval(rv));
> + adj = dval(aadj1) * ulp(dval(rv));
> dval(rv) += adj;
> #ifdef IBM
> if ((dword0(rv) & Exp_mask) < P*Exp_msk1)
> @@ -1076,7 +1077,7 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> dword0(rv) -= P*Exp_msk1;
> }
> else {
> - adj = aadj1 * ulp(dval(rv));
> + adj = dval(aadj1) * ulp(dval(rv));
> dval(rv) += adj;
> }
> #else /*Sudden_Underflow*/
> @@ -1088,11 +1089,11 @@ _DEFUN (_strtod_r, (ptr, s00, se),
> * example: 1.2e-307 .
> */
> if (y <= (P-1)*Exp_msk1 && aadj > 1.) {
> - aadj1 = (double)(int)(aadj + 0.5);
> + dval(aadj1) = (double)(int)(aadj + 0.5);
> if (!dsign)
> - aadj1 = -aadj1;
> + dval(aadj1) = -dval(aadj1);
> }
> - adj = aadj1 * ulp(dval(rv));
> + adj = dval(aadj1) * ulp(dval(rv));
> dval(rv) += adj;
> #endif /*Sudden_Underflow*/
> #endif /*Avoid_Underflow*/
>
>
> brgds, H-P
>
More information about the Newlib
mailing list