# [PATCH] zend_operators DVAL_TO_LVAL fix - Thread: https://externals.io/message/12420 - Original archive: https://news-web.php.net/php.internals/12420 - Started by Joe Orton on 2004-08-27T12:25:50+00:00 - Messages: 7 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #12420 — Joe Orton — 2004-08-27T12:25:50+00:00 URL: https://externals.io/message/12420#12420 The DVAL_TO_LVAL macro is quite weird, I'm not sure exactly what it's supposed to be doing but it probably isn't doing it. If the integral part of d is outside the range of a long, the conversion has undefined behaviour by the C99 standard; an explicit cast makes no difference AFAICT. GCC on IA64 does wierd things with this macro, though I think there's a GCC bug involved there too. This fixes the macro to have well-defined behaviour for all values of 'd', and avoids triggering the GCC bug on IA64 to boot (both PHP users on that platform will be happy): Index: Zend/zend_operators.c =================================================================== RCS file: /repository/ZendEngine2/zend_operators.c,v retrieving revision 1.194 diff -u -r1.194 zend_operators.c --- Zend/zend_operators.c 19 Jul 2004 07:19:02 -0000 1.194 +++ Zend/zend_operators.c 27 Aug 2004 12:15:12 -0000 @@ -183,7 +183,15 @@ } -#define DVAL_TO_LVAL(d, l) (l) = (d) > LONG_MAX ? (unsigned long) (d) : (long) (d) +#define DVAL_TO_LVAL(d, l) do { \ + if ((d) > LONG_MAX) { \ + l = LONG_MAX; \ + } else if ((d) < LONG_MIN) { \ + l = LONG_MIN; \ + } else { \ + l = (d); \ + } \ +} while (0) #define zendi_convert_to_long(op, holder, result) \ if (op==result) { \ --- ## #12482 — Andi Gutmans — 2004-08-30T19:32:59+00:00 URL: https://externals.io/message/12420#12482 · In reply to: #12420 Hi Joe, It seems like your patch doesn't really fix anything. How is rounding to LONG_MAX/LONG_MIN any better? Maybe you can explain in more detail what this gcc bug you are hitting is? Thanks, Andi At 01:25 PM 8/27/2004 +0100, Joe Orton wrote: >The DVAL_TO_LVAL macro is quite weird, I'm not sure exactly what it's >supposed to be doing but it probably isn't doing it. If the integral >part of d is outside the range of a long, the conversion has undefined >behaviour by the C99 standard; an explicit cast makes no difference >AFAICT. > >GCC on IA64 does wierd things with this macro, though I think there's a >GCC bug involved there too. This fixes the macro to have well-defined >behaviour for all values of 'd', and avoids triggering the GCC bug on >IA64 to boot (both PHP users on that platform will be happy): > >Index: Zend/zend_operators.c >=================================================================== >RCS file: /repository/ZendEngine2/zend_operators.c,v >retrieving revision 1.194 >diff -u -r1.194 zend_operators.c >--- Zend/zend_operators.c 19 Jul 2004 07:19:02 -0000 1.194 >+++ Zend/zend_operators.c 27 Aug 2004 12:15:12 -0000 >@@ -183,7 +183,15 @@ > } > > >-#define DVAL_TO_LVAL(d, l) (l) = (d) > LONG_MAX ? (unsigned long) (d) : >(long) (d) >+#define DVAL_TO_LVAL(d, l) do { \ >+ if ((d) > LONG_MAX) { \ >+ l = LONG_MAX; \ >+ } else if ((d) < LONG_MIN) { \ >+ l = LONG_MIN; \ >+ } else { \ >+ l = (d); \ >+ } \ >+} while (0) > > #define zendi_convert_to_long(op, holder, > result) \ > if (op==result) > { > \ --- ## #12483 — Joe Orton — 2004-08-30T20:24:40+00:00 URL: https://externals.io/message/12420#12483 · In reply to: #12482 > Hi Joe, > > It seems like your patch doesn't really fix anything. How is rounding to > LONG_MAX/LONG_MIN any better? The C standard says that when converting a double to a long, if the integral part of the double is outside the range which can be represented by a long, the conversion has undefined behaviour. #define DVAL_TO_LVAL(d, l) (l) = (d) > LONG_MAX ? (unsigned long) (d) : (long) (d) where d is a double and l is a long: so this has undefined behaviour for values of d greater than ULONG_MAX or smaller than LONG_MIN. #define DVAL_TO_LVAL(d, l) do { \ if ((d) > LONG_MAX) { \ l = LONG_MAX; \ } else if ((d) < LONG_MIN) { \ l = LONG_MIN; \ } else { \ l = (d); \ } \ } while (0) has well-defined behaviour for all values of 'd', since it will never attempt to assign a value to 'l' which cannot be represented by a long. > Maybe you can explain in more detail what this gcc bug you are hitting is? It seems to be an IA64-specific optimisation bug, it's really incidental to the fact that the current code is dubious. GCC evaluates the expression as either 0 or LONG_MIN if d is negative, depending on -O level. (Which is fairly nasty) joe --- ## #12735 — Ard Biesheuvel — 2004-09-11T13:22:06+00:00 URL: https://externals.io/message/12420#12735 · In reply to: #12420 Joe Orton wrote: > The DVAL_TO_LVAL macro is quite weird, I'm not sure exactly what it's > supposed to be doing but it probably isn't doing it. If the integral > part of d is outside the range of a long, the conversion has undefined > behaviour by the C99 standard; an explicit cast makes no difference > AFAICT. > > GCC on IA64 does wierd things with this macro, though I think there's a > GCC bug involved there too. This fixes the macro to have well-defined > behaviour for all values of 'd', and avoids triggering the GCC bug on > IA64 to boot (both PHP users on that platform will be happy): > This probably has to do with the fact that on 64-bit systems, doubles lack the accuracy to distinguish LONG_MAX from LONG_MAX +1. To be on the safe side here, you might want to use >= LONG_MAX instead of > LONG_MAX, or cast the other way around. -- Ard --- ## #12736 — Ard Biesheuvel — 2004-09-11T13:22:27+00:00 URL: https://externals.io/message/12420#12736 · In reply to: #12420 Joe Orton wrote: > The DVAL_TO_LVAL macro is quite weird, I'm not sure exactly what it's > supposed to be doing but it probably isn't doing it. If the integral > part of d is outside the range of a long, the conversion has undefined > behaviour by the C99 standard; an explicit cast makes no difference > AFAICT. > > GCC on IA64 does wierd things with this macro, though I think there's a > GCC bug involved there too. This fixes the macro to have well-defined > behaviour for all values of 'd', and avoids triggering the GCC bug on > IA64 to boot (both PHP users on that platform will be happy): > This probably has to do with the fact that on 64-bit systems, doubles lack the accuracy to distinguish LONG_MAX from LONG_MAX +1. To be on the safe side here, you might want to use >= LONG_MAX instead of > LONG_MAX, or cast the other way around. -- Ard --- ## #12748 — Joe Orton — 2004-09-12T16:50:09+00:00 URL: https://externals.io/message/12420#12748 · In reply to: #12736 > Joe Orton wrote: > > […] > > This probably has to do with the fact that on 64-bit systems, doubles > lack the accuracy to distinguish LONG_MAX from LONG_MAX +1. To be on the > safe side here, you might want to use >= LONG_MAX instead of > LONG_MAX, > or cast the other way around. It actually was a GCC bug, in fact, one of our GCC developers tracked it down: http://gcc.gnu.org/ml/gcc-patches/2004-08/msg02654.html joe --- ## #12806 — Andi Gutmans — 2004-09-14T17:26:10+00:00 URL: https://externals.io/message/12420#12806 · In reply to: #12420 Also <= LONG_MIN needed? I guess yes? At 03:22 PM 9/11/2004 +0200, Ard Biesheuvel wrote: >Joe Orton wrote: > > […] > >This probably has to do with the fact that on 64-bit systems, doubles lack >the accuracy to distinguish LONG_MAX from LONG_MAX +1. To be on the safe >side here, you might want to use >= LONG_MAX instead of > LONG_MAX, or >cast the other way around. > >-- >Ard