[WIP] refresh appveyor #293

stesie · 2017-03-05T19:43:08Z

I'd like to refresh v8js' appveyor to some more recent assets, ... and then try adding x64 build ... and last but not least try to reproduce #292 over there.

Yet work in progress, will rewrite commits here until appveyor says green :-)

Jan-E · 2017-03-06T00:01:53Z

The failing test is 'Check long integer handling from PHP to JS', which exactly reflects #292

In other words: the AppVeyor setup is correct at the moment. It should fail on this test and it really does. Also note that only the x64 builds produce 'possible loss of data' warnings, like those here:
https://ci.appveyor.com/project/stesie/v8js/build/1.0.133/job/06xwsoo6m6e7h4fm#L239

You are on the right track! I was looking into those errors as well and noticed that a lot of them had to do with the V8Js string and integer macro definitions. See for instance the error message for this line:

jsValue = V8JS_ZSTR(Z_STR_P(value));

The relevant parts of the code block:

v8::Handle<v8::Value> zval_to_v8js(zval *value, v8::Isolate *isolate TSRMLS_DC) /* {{{ */
{
	v8::Handle<v8::Value> jsValue;
	long v;
	zend_class_entry *ce;

	switch (Z_TYPE_P(value))
	{
		case IS_STRING:
			jsValue = V8JS_ZSTR(Z_STR_P(value));
			break;

		case IS_LONG:
			v = Z_LVAL_P(value);

There is a warning as well on the last line of the quote:

v = Z_LVAL_P(value);

For someone like @weltling it should be easy to tell what is wrong in those two lines, I guess, because he is the author of the 64-bits changes:
https://github.com/php/php-src/blob/master/Zend/zend_long.h#L27
I hope he has some spare time to look at this.

Jan-E · 2017-03-06T01:27:13Z

Another observation. If you change this in https://github.com/phpv8/v8js/blob/php7/v8js_convert.cc#L104

- long v;
+ zend_long v:

the warning for https://github.com/phpv8/v8js/blob/php7/v8js_convert.cc#L140 disappears. Instead a warning is issued on https://github.com/phpv8/v8js/blob/php7/v8js_convert.cc#L148:

jsValue = V8JS_INT(v);

ext\v8js\v8js_convert.cc(148): warning C4244: 'argument': conversion from 'zend_long' to 'int32_t', possible loss of data

Interpretation: v is zend_long, V8JS_INT(v) is defined as 'v8::Integer::New(isolate, v)', jsValue is just a (typeless?) v8::Handlev8::Value. Something tells me that V8JS_INT(v) should have been int64_t and not int32_t.

Jan-E · 2017-03-06T01:37:32Z

I tried to fix the warnings in v8js_array_access.cc with this diff:

diff --git a/v8js_array_access.cc b/v8js_array_access.cc
index 3419172..5cdcbfa 100644
--- a/v8js_array_access.cc
+++ b/v8js_array_access.cc
@@ -110,7 +110,7 @@ void v8js_array_access_setter(uint32_t index, v8::Local<v8::Value> value,
 /* }}} */
 
 
-static int v8js_array_access_get_count_result(zend_object *object TSRMLS_DC) /* {{{ */
+static zend_long v8js_array_access_get_count_result(zend_object *object TSRMLS_DC) /* {{{ */
 {
 	zval zvalue;
 	ZVAL_UNDEF(&zvalue);
@@ -123,7 +123,7 @@ static int v8js_array_access_get_count_result(zend_object *object TSRMLS_DC) /*
 		return 0;
 	}
 
-	int result = Z_LVAL(php_value);
+	zend_long result = Z_LVAL(php_value);
 	return result;
 }
 /* }}} */
@@ -155,7 +155,7 @@ static void v8js_array_access_length(v8::Local<v8::String> property, const v8::P
 
 	zend_object *object = reinterpret_cast<zend_object *>(self->GetAlignedPointerFromInternalField(1));
 
-	int length = v8js_array_access_get_count_result(object TSRMLS_CC);
+	zend_long length = v8js_array_access_get_count_result(object TSRMLS_CC);
 	info.GetReturnValue().Set(V8JS_INT(length));
 }
 /* }}} */
@@ -206,8 +206,8 @@ void v8js_array_access_enumerator(const v8::PropertyCallbackInfo<v8::Array>& inf
 
 	zend_object *object = reinterpret_cast<zend_object *>(self->GetAlignedPointerFromInternalField(1));
 
-	int length = v8js_array_access_get_count_result(object TSRMLS_CC);
-	v8::Local<v8::Array> result = v8::Array::New(isolate, length);
+	zend_long length = v8js_array_access_get_count_result(object TSRMLS_CC);
+	v8::Local<v8::Array> result = v8::Array::New(isolate, (int)length);
 
 	int i = 0;

Result:

ext\v8js\v8js_array_access.cc(159): warning C4244: 'argument': conversion from 'zend_long' to 'int32_t', possible loss of data

https://github.com/phpv8/v8js/blob/php7/v8js_array_access.cc#L159:

info.GetReturnValue().Set(V8JS_INT(length));

The same warning as in the previous comment. And the same macro V8JS_INT ...

Jan-E · 2017-03-06T01:48:13Z

For the length of an array there seems to be no realistic scenario that the array-length is more than 32-bits, so maybe a explicit cast to int is allowed here:

diff --git a/v8js_array_access.cc b/v8js_array_access.cc
index 3419172..3eb37cc 100644
--- a/v8js_array_access.cc
+++ b/v8js_array_access.cc
@@ -123,8 +123,8 @@ static int v8js_array_access_get_count_result(zend_object *object TSRMLS_DC) /*
 		return 0;
 	}
 
-	int result = Z_LVAL(php_value);
-	return result;
+	zend_long result = Z_LVAL(php_value);
+	return (int)result;
 }
 /* }}} */

or shorter:

diff --git a/v8js_array_access.cc b/v8js_array_access.cc
index 3419172..91861f5 100644
--- a/v8js_array_access.cc
+++ b/v8js_array_access.cc
@@ -123,7 +123,7 @@ static int v8js_array_access_get_count_result(zend_object *object TSRMLS_DC) /*
 		return 0;
 	}
 
-	int result = Z_LVAL(php_value);
+	int result = (int)Z_LVAL(php_value);
 	return result;
 }
 /* }}} */

This also suppresses the warning in v8js_array_access.cc

Jan-E · 2017-03-06T03:04:18Z

I took a closer look at long.phpt and see now that you do not check for 64-bit integers, but for

pow(2, 31)
pow(2, 31) + 1
-pow(2, 31)
-pow(2, 31) - 1

Result when I ran this test myself:

---- EXPECTED OUTPUT
2147483648
2147483649
-2147483648
-2147483649
===EOF===
---- ACTUAL OUTPUT
-2147483648
-2147483647
-2147483648
2147483647
===EOF===
---- FAILED

The first 2 actual results are in line with #292 (comment)

>= pow(2,31) => negative

@flexarts even did a check for pow(2,45): #287 (comment)
The result on *nix was 35184372088832, on Windows 0.

You might consider to add things like pow(2,45) to the long.phpt test or create a new long64bits.phpt.

Jan-E · 2017-03-06T03:13:16Z

You might consider to add things like pow(2,45) to the long.phpt test or create a new long64bits.phpt.

Best option: create a new long64bits.phpt with pow(2,45) and skip that test for x86 builds.

stesie · 2017-03-06T06:35:03Z

Hey @Jan-E, thanks for your comments. And yes this now all guides to the direction you outlined.
Unfortunately I didn't have more time to spent yesterday night.

... and I don't yet know whether I'll find enough time in the course of the week, let's see. Yet I'm definitely going to continue working on that :)

stesie · 2017-03-06T07:33:35Z

Some interesting read on V8 and 64-bit integers: https://bugs.chromium.org/p/v8/issues/detail?id=973

So v8::Integer::New intentionally takes just a int32_t ... as it doesn't (want to) support 64-bit integers at all. After all the EcmaScript standard doesn't know of integers at all and numbers are generally represented by double precision numbers. The v8::Integer is just an implementation aid in V8.

If we let V8Js convert 64-bit integers to JS numbers, then there might be some loss in precision. Generally I think this is fine, yet should be pointed out in the README. We might show PHP warnings, yet I think this is more feature creep then helpful.

hence a roadmap could be

fix use of long in PHP -> JS export of longs > 31bit
make sure all compiler warnings on Win64 are eliminated
update README so it tells about possible precission loss
add test cases for > 32bit integers (skip on 32bit PHP), PHP to JS
add test case for 31/32/48 bit integer JS back to PHP (should possibly end up as float)

Jan-E · 2017-03-06T11:50:56Z

Interesting stuff indeed.

Strange thing is that the x86 builds generate the correct output for the tests with pow(2,31). On x64 there must be a mismatch between signed and unsigned ints somewhere.

flexarts · 2017-03-06T13:02:01Z

Hi @stesie & @Jan-E,

Thank you both for you efforts to provide a functioning 64-bit V8 PHP extension also for Windows.

I initally made the 64-bit int tests for verification of our main reason to use 64-bit integers namely datetime milliseconds. Our customers are railway infrastructure companies which maintain assets (e.g. bridges/tunnels...) with construction datetime values starting from 1800 :-) We ran into problems using PHP5* 32-bit builds related to datetime functions when using integers to represent unixtime.

Cheers to you!

Jan-E · 2017-03-08T09:57:46Z

PHP 7.0.17RC1 nts x64:

=====================================================================
Number of tests :  164               161
Tests skipped   :    3 (  1.8%) --------
Tests warned    :    0 (  0.0%) (  0.0%)
Tests failed    :    0 (  0.0%) (  0.0%)
Expected fail   :    0 (  0.0%) (  0.0%)
Tests passed    :  161 ( 98.2%) (100.0%)
---------------------------------------------------------------------
Time taken      :  575 seconds
=====================================================================

Slow machine, because it was busy compiling te other versions. But even more tests passed than in your appveyor setup! Congrats.

Jan-E · 2017-03-08T10:26:02Z

Additional tests passing in my setup with all extensions:

PASS Test V8Js::setModuleLoader : CommonJS modules [/php-sdk/php70dev/ext/v8js/tests/commonjs_modules.phpt]
PASS Test V8::executeString() : DOM object passed from PHP [/php-sdk/php70dev/ext/v8js/tests/object_dom.phpt]

stesie · 2017-03-08T11:13:56Z

@Jan-E yes, those two tests require json and libxml extensions, which aren't built on appveyor.
So far I haven't tested whether it'd be feasible to build those there

Jan-E · 2017-03-08T11:58:31Z

Json should be easy as it does not need external dependencies and compiles as static extension by default. Just change the configure line into:

configure --disable-all --enable-cli --enable-json --with-v8js %CONFIGURE_EXTRA%

Not tested, but worth the try.

stesie added 4 commits March 5, 2017 20:29

(appveyor) Use PHP 7.0.16

31f5871

(appveyor) update build assets to 5.8.301.0

e3a5b91

(appveyor) supply v8 assets .zip name via env

e631f25

(appveyor) add x64 builds

a9bdc32

stesie force-pushed the refresh-appveyor branch from 547ae25 to a9bdc32 Compare March 5, 2017 22:06

stesie added 3 commits March 8, 2017 07:38

fix long vs. zend_long issue on Windows, refs phpv8#292

e86c957

pick up int64 export test from issue phpv8#287

b45bef4

skip long_int64 test on 32bit PHP versions

346c47b

stesie merged commit 9a1f42c into phpv8:php7 Mar 8, 2017

stesie mentioned this pull request Mar 8, 2017

Fix precission-loss warnings on Windows x64 build #295

Closed

2 tasks

Jan-E mentioned this pull request Mar 8, 2017

Building v8 on windows #287

Closed

stesie mentioned this pull request Mar 10, 2017

Win64 precission loss fixes #296

Merged

stesie deleted the refresh-appveyor branch April 14, 2017 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] refresh appveyor #293

[WIP] refresh appveyor #293

Uh oh!

stesie commented Mar 5, 2017

Uh oh!

Jan-E commented Mar 6, 2017 •

edited

Loading

Uh oh!

Jan-E commented Mar 6, 2017 •

edited

Loading

Uh oh!

Jan-E commented Mar 6, 2017

Uh oh!

Jan-E commented Mar 6, 2017 •

edited

Loading

Uh oh!

Jan-E commented Mar 6, 2017

Uh oh!

Jan-E commented Mar 6, 2017

Uh oh!

stesie commented Mar 6, 2017

Uh oh!

stesie commented Mar 6, 2017 •

edited

Loading

Uh oh!

Jan-E commented Mar 6, 2017

Uh oh!

flexarts commented Mar 6, 2017

Uh oh!

Jan-E commented Mar 8, 2017

Uh oh!

Jan-E commented Mar 8, 2017

Uh oh!

stesie commented Mar 8, 2017

Uh oh!

Jan-E commented Mar 8, 2017

Uh oh!

Uh oh!

[WIP] refresh appveyor #293

[WIP] refresh appveyor #293

Uh oh!

Conversation

stesie commented Mar 5, 2017

Uh oh!

Jan-E commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jan-E commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jan-E commented Mar 6, 2017

Uh oh!

Jan-E commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jan-E commented Mar 6, 2017

Uh oh!

Jan-E commented Mar 6, 2017

Uh oh!

stesie commented Mar 6, 2017

Uh oh!

stesie commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jan-E commented Mar 6, 2017

Uh oh!

flexarts commented Mar 6, 2017

Uh oh!

Jan-E commented Mar 8, 2017

Uh oh!

Jan-E commented Mar 8, 2017

Uh oh!

stesie commented Mar 8, 2017

Uh oh!

Jan-E commented Mar 8, 2017

Uh oh!

Uh oh!

Jan-E commented Mar 6, 2017 •

edited

Loading

Jan-E commented Mar 6, 2017 •

edited

Loading

Jan-E commented Mar 6, 2017 •

edited

Loading

stesie commented Mar 6, 2017 •

edited

Loading