Skip to content

fix double cast to int on 32-bit#3476

Closed
remicollet wants to merge 1 commit into
php:PHP-7.1from
remicollet:issue-gcc82
Closed

fix double cast to int on 32-bit#3476
remicollet wants to merge 1 commit into
php:PHP-7.1from
remicollet:issue-gcc82

Conversation

@remicollet
Copy link
Copy Markdown
Member

2 tests are failing with GCC 8.2 on 32-bit build

zend_dval_to_lval preserves low bits  (32 bit long) [Zend/tests/dval_to_lval_32.phpt]
testing integer underflow (32bit) [Zend/tests/int_underflow_32bit.phpt]

The comment was added in 716da71 by @dstogov

@remicollet remicollet changed the base branch from master to PHP-7.1 August 29, 2018 07:18
@cmb69
Copy link
Copy Markdown
Member

cmb69 commented Aug 29, 2018

This fix looks good to me. Without it we would cast negative doubles to unsigned ints, which is undefined behavior.

However, this should be carefully tested on Windows which never defines ZEND_DVAL_TO_LVAL_CAST_OK. A quick check on VC15/x86 succeeds, but probably more checking should be done. cc @weltling

BTW: the language specification states:

If the remainder is less than zero, it is rounded towards infinity and X is added.

However, the current implementation rounds towards zero in this case. Is the implementation or the spec in error?

@dstogov
Copy link
Copy Markdown
Member

dstogov commented Aug 29, 2018

The fix is right.
I guess, GCC-8.2 started to use SSE floating point math instruction in 32-bit mode by default.
Older versions used x87 FPU instruction, with more precise internal representation, that didn't need this addition.

Please, merge this.

@remicollet
Copy link
Copy Markdown
Member Author

@dstogov thanks for the check.

Merged

@remicollet remicollet closed this Aug 30, 2018
@remicollet remicollet deleted the issue-gcc82 branch August 30, 2018 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants