Everything

php

Home » Project » Newsgroups » php.internals » [PATCH] zend_operators DVAL_TO_LVAL fix »

[PATCH] zend_operators DVAL_TO_LVAL fix

php.internals

Joe Orton

22 years ago

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) {												\

Andi Gutmans

22 years ago

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:

Joe Orton

22 years ago

On Mon, Aug 30, 2004 at 12:32:59PM -0700, Andi Gutmans wrote:

> 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

Andi Gutmans

22 years ago

I know it's undefined but why is defining it to LONG_MAX/LONG_MIN any
better? It's not the kind of behavior which I think we should "define".
In general, PHP always keeps the values as doubles if it detects that the
value is too low. This macro is only used when you force it, and I don't
think setting arbritrary values is any better than undefined.


Andi

At 09:24 PM 8/30/2004 +0100, Joe Orton wrote:

Joe Orton

22 years ago

On Mon, Aug 30, 2004 at 02:20:42PM -0700, Andi Gutmans wrote:

> I know it's undefined but why is defining it to LONG_MAX/LONG_MIN any
> better? It's not the kind of behavior which I think we should "define".

C code which has undefined behaviour may segfault or suffer some other
run-time exception; the compiler may arrange for it to print "I'm a
fluffy pink rabbit" to stderr as a side-effect.

Andi Gutmans

22 years ago

At 11:17 PM 8/30/2004 +0100, Joe Orton wrote:

>On Mon, Aug 30, 2004 at 02:20:42PM -0700, Andi Gutmans wrote:
> > I know it's undefined but why is defining it to LONG_MAX/LONG_MIN any
> > better? It's not the kind of behavior which I think we should "define".
>
>C code which has undefined behaviour may segfault or suffer some other
>run-time exception; the compiler may arrange for it to print "I'm a
>fluffy pink rabbit" to stderr as a side-effect.

Joe,

I'm pretty sure if you read the C standard it'll say something about the
"value" being undefined, not about the behavior of the whole program being
undefined.

Andi

Joe Orton

22 years ago

On Mon, Aug 30, 2004 at 04:40:13PM -0700, Andi Gutmans wrote:

> At 11:17 PM 8/30/2004 +0100, Joe Orton wrote:
> >On Mon, Aug 30, 2004 at 02:20:42PM -0700, Andi Gutmans wrote:
> >> I know it's undefined but why is defining it to LONG_MAX/LONG_MIN any
> >> better? It's not the kind of behavior which I think we should "define".
> >
> >C code which has undefined behaviour may segfault or suffer some other
> >run-time exception; the compiler may arrange for it to print "I'm a
> >fluffy pink rabbit" to stderr as a side-effect.
>
> Joe,
>
> I'm pretty sure if you read the C standard it'll say something about the
> "value" being undefined, not about the behavior of the whole program being
> undefined.

Andi, I checked the relevant parts of the C standard before writing the
patch, I would not presume to waste your time with mere speculation or
guesswork on such matters.

C99 section 6.3.1.4 "Real floating and integer" defines conversion of
double to long, and says "If the value of the integral part cannot be
represented by the integer type, the behavior is undefined." (as I in
fact stated in my original mail).

Section 3.4.3 defines the term "undefined behaviour" in black and white.
It's quite clear.

Regards,

joe

Joe Orton

22 years ago

Also, the bug27354 test needs to be updated since it relies on
particular behaviour of integers greater than LONG_MAX on 32-bit
platforms; since it looks like it is only really checking for
not-a-division-by-zero-trap, this seems OK:

--- php-4.3.8/tests/lang/bug27354.phpt.dval2lval
+++ php-4.3.8/tests/lang/bug27354.phpt
@@ -7,8 +7,8 @@
var_dump(-2147483648 % -1);
var_dump(-2147483648 % -2);
?>
---EXPECT--
-int(0)
-int(0)
-int(0)
-int(0)
+--EXPECTF--
+int(%i)
+int(%i)
+int(%i)
+int(%i)

Derick Rethans

22 years ago

On Tue, 31 Aug 2004, Joe Orton wrote:

> Also, the bug27354 test needs to be updated since it relies on
> particular behaviour of integers greater than LONG_MAX on 32-bit
> platforms; since it looks like it is only really checking for
> not-a-division-by-zero-trap, this seems OK:

Yup, please commit this.

regards,
Derick

--
Derick Rethans
http://derickrethans.nl | http://ez.no | http://xdebug.org

Joe Orton

22 years ago

On Tue, Aug 31, 2004 at 11:47:06AM +0200, Derick Rethans wrote:

> On Tue, 31 Aug 2004, Joe Orton wrote:
>
> > Also, the bug27354 test needs to be updated since it relies on
> > particular behaviour of integers greater than LONG_MAX on 32-bit
> > platforms; since it looks like it is only really checking for
> > not-a-division-by-zero-trap, this seems OK:
>
> Yup, please commit this.

I don't have commit access, if that request is directed at me.

joe

Derick Rethans

22 years ago

On Tue, 31 Aug 2004, Joe Orton wrote:

> On Tue, Aug 31, 2004 at 11:47:06AM +0200, Derick Rethans wrote:
> > On Tue, 31 Aug 2004, Joe Orton wrote:
> >
> > > Also, the bug27354 test needs to be updated since it relies on
> > > particular behaviour of integers greater than LONG_MAX on 32-bit
> > > platforms; since it looks like it is only really checking for
> > > not-a-division-by-zero-trap, this seems OK:
> >
> > Yup, please commit this.
>
> I don't have commit access, if that request is directed at me.

It's about time you get access, don't you think? :)
Here you can request one: http://www.php.net/cvs-php.php

regards,
Derick

--
Derick Rethans
http://derickrethans.nl | http://ez.no | http://xdebug.org

Joe Orton

22 years ago

On Tue, Aug 31, 2004 at 12:02:31PM +0200, Derick Rethans wrote:

> On Tue, 31 Aug 2004, Joe Orton wrote:
>
> > On Tue, Aug 31, 2004 at 11:47:06AM +0200, Derick Rethans wrote:
> > > On Tue, 31 Aug 2004, Joe Orton wrote:
> > >
> > > > Also, the bug27354 test needs to be updated since it relies on
> > > > particular behaviour of integers greater than LONG_MAX on 32-bit
> > > > platforms; since it looks like it is only really checking for
> > > > not-a-division-by-zero-trap, this seems OK:
> > >
> > > Yup, please commit this.
> >
> > I don't have commit access, if that request is directed at me.
>
> It's about time you get access, don't you think? :)
> Here you can request one: http://www.php.net/cvs-php.php

Doh, I was wrong - It looks like I was signed up a year ago but
completely forgot about it! I've committed the test fix now.

joe

Andi Gutmans

22 years ago

Hi Joe,

It does look as if you're right. I don't quite understand why the standard
was written in such a way and not in a way which only makes the value
itself undefined.
I think we can apply the patch. Does anyone have a problem with setting
arbitrary numbers such as LONG_MAX/LONG_MIN for an undefined conversion?

Andi

At 08:54 AM 8/31/2004 +0100, Joe Orton wrote:

Joe Orton

21 years ago

On Tue, Aug 31, 2004 at 09:48:47PM -0700, Andi Gutmans wrote:

> It does look as if you're right. I don't quite understand why the standard
> was written in such a way and not in a way which only makes the value
> itself undefined.
> I think we can apply the patch. Does anyone have a problem with setting
> arbitrary numbers such as LONG_MAX/LONG_MIN for an undefined conversion?

I didn't see any objections; so could this be committed?

(the patch again below for reference)

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) {												\

Andi Gutmans

21 years ago

Commited.

At 09:32 AM 9/10/2004 +0100, Joe Orton wrote:

Ard Biesheuvel

21 years ago

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

Ard Biesheuvel

21 years ago

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

Joe Orton

21 years ago

On Sat, Sep 11, 2004 at 03:22:27PM +0200, Ard Biesheuvel wrote:

> Joe Orton wrote:
> >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.

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

Andi Gutmans

21 years ago

Also <= LONG_MIN needed?
I guess yes?

At 03:22 PM 9/11/2004 +0200, Ard Biesheuvel wrote: