Skip to content

[RFC] Path to Saner Increment/Decrement operators #10358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 23 commits into from
Jul 17, 2023

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jan 17, 2023

RFC: https://wiki.php.net/rfc/saner-inc-dec-operators

The goal of this RFC is to normalize the behaviour of $v++ and $v-- to be the same as $v += 1 and $v -= 1, respectively.

@fisharebest
Copy link
Contributor

Alphabetic increment is very useful for generating spreadsheet column names, e.g. using PHPOffice/PhpSpreadsheet.

$col = 'A';
$row = '1';
foreach ($data as $datum) {
  $sheet->setCellValue($col++ . $row, $datum);
}

@Girgias
Copy link
Member Author

Girgias commented Jan 18, 2023

Alphabetic increment is very useful for generating spreadsheet column names, e.g. using PHPOffice/PhpSpreadsheet.

$col = 'A';
$row = '1';
foreach ($data as $datum) {
  $sheet->setCellValue($col++ . $row, $datum);
}

This is not the place to discuss RFCs the internal mailing list is, and this point was already brought up: https://externals.io/message/119290

@fisharebest
Copy link
Contributor

Apologies for breaching etiquette. Followed links from the RFC announcement on phpc.social...

@Girgias Girgias force-pushed the de-in-crement-rfc-sane branch from 69b6745 to 8eacb42 Compare January 20, 2023 14:44
@Girgias Girgias force-pushed the de-in-crement-rfc-sane branch from c55a0f2 to f6c0a98 Compare June 29, 2023 12:40
@Girgias Girgias force-pushed the de-in-crement-rfc-sane branch from f6c0a98 to 25c0e2e Compare June 29, 2023 12:57
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should update type inference accordingly. PRE/POST_INC/DEC won't return IS_NULL, IS_FALSE, IS_TRUE any more. e.g. see:

tmp |= MAY_BE_NULL;

tmp |= t1 & (MAY_BE_FALSE | MAY_BE_TRUE | MAY_BE_OBJECT);

tmp |= MAY_BE_NULL;

tmp |= t1 & (MAY_BE_FALSE | MAY_BE_TRUE | MAY_BE_RESOURCE | MAY_BE_ARRAY | MAY_BE_OBJECT | MAY_BE_ARRAY_OF_ANY | MAY_BE_ARRAY_OF_REF | MAY_BE_ARRAY_KEY_ANY);

@Girgias
Copy link
Member Author

Girgias commented Jul 4, 2023

You should update type inference accordingly. PRE/POST_INC/DEC won't return IS_NULL, IS_FALSE, IS_TRUE any more. e.g. see:

tmp |= MAY_BE_NULL;

tmp |= t1 & (MAY_BE_FALSE | MAY_BE_TRUE | MAY_BE_OBJECT);

tmp |= MAY_BE_NULL;

tmp |= t1 & (MAY_BE_FALSE | MAY_BE_TRUE | MAY_BE_RESOURCE | MAY_BE_ARRAY | MAY_BE_OBJECT | MAY_BE_ARRAY_OF_ANY | MAY_BE_ARRAY_OF_REF | MAY_BE_ARRAY_KEY_ANY);

But updating it now would make the type inference incorrect, no? As the RFC doesn't change the output of these operators yet, just that they start emitting E_WARNINGs and E_DEPRECATED diagnostics.

Or, because, those operations emits diagnostics the type inference bails out and thus I should already update it to not forget?

@Girgias
Copy link
Member Author

Girgias commented Jul 4, 2023

@dstogov do I need to do anything in particular for ZEND_PRE_INC_OBJ, ZEND_PRE_DEC_OBJ, ZEND_POST_INC_OBJ and ZEND_POST_DEC_OBJ opcodes in the VM/zend_inference.c?

@Girgias Girgias force-pushed the de-in-crement-rfc-sane branch from 25c0e2e to 497f687 Compare July 4, 2023 20:40
@dstogov
Copy link
Member

dstogov commented Jul 5, 2023

But updating it now would make the type inference incorrect, no? As the RFC doesn't change the output of these operators yet, just that they start emitting E_WARNINGs and E_DEPRECATED diagnostics.

Oh, I stopped reading RFC at "Summary of behavioural differences". Then you shouldn't update inference code now.

@dstogov dstogov closed this Jul 5, 2023
@dstogov dstogov reopened this Jul 5, 2023
@dstogov
Copy link
Member

dstogov commented Jul 5, 2023

Sorry, I closed this unintentionally. Re-opened.

@Girgias Girgias force-pushed the de-in-crement-rfc-sane branch from 497f687 to a28b1f3 Compare July 13, 2023 15:20
@Girgias Girgias force-pushed the de-in-crement-rfc-sane branch from 283ed45 to 12d84fe Compare July 16, 2023 17:43
@Girgias Girgias merged commit d8696f9 into php:master Jul 17, 2023
@Girgias Girgias deleted the de-in-crement-rfc-sane branch July 17, 2023 14:51
@dstogov
Copy link
Member

dstogov commented Jul 24, 2023

This broke something (OSS Fuzz #60709). The following script produces incorrect output and leads to assertion in tracing JIT (PHP-8.2 works fine).

<?php
set_error_handler(function($_, $m) {
    echo "$m\n";
    unset($GLOBALS['x']);
});
var_dump($x--);
unset($x);
var_dump($x++);
unset($x);
var_dump(--$x);
unset($x);
var_dump(++$x);
?>
Undefined variable $x
Decrement on type null has no effect, this will change in the next major version of PHP
NULL
Undefined variable $x
NULL
Undefined variable $x
Decrement on type null has no effect, this will change in the next major version of PHP
UNKNOWN:0
Undefined variable $x
int(1)
php: /home/dmitry/php/php-master/ext/opcache/jit/zend_jit_trace.c:381: zend_jit_trace_type_to_info_ex: Assertion `info & (1 << type)' failed.
Aborted (core dumped)

@dstogov
Copy link
Member

dstogov commented Jul 24, 2023

Another problem (OSS Fuzz #60734) - use-after-free visible in ASAN build.

<?php
class Foo{
}
$test = new Foo;
$y = ++$test;
?>
$ USE_ZEND_ALLOC=0 sapi/cli/php -d opcache.jit=0 ~/tmp/fuzz-60734.php 
PHP Fatal error:  Uncaught TypeError: Cannot increment Foo in /home/dmitry/tmp/fuzz-60734.php:5
Stack trace:
#0 {main}
  thrown in /home/dmitry/tmp/fuzz-60734.php on line 5

Fatal error: Uncaught TypeError: Cannot increment Foo in /home/dmitry/tmp/fuzz-60734.php:5
Stack trace:
#0 {main}
  thrown in /home/dmitry/tmp/fuzz-60734.php on line 5
=================================================================
==12426==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000071410 at pc 0x000001ff965f bp 0x7ffdaeb1e2b0 sp 0x7ffdaeb1e2a8
READ of size 4 at 0x604000071410 thread T0
    #0 0x1ff965e in zend_gc_refcount /home/dmitry/php/php-master/Zend/zend_types.h:1261
    #1 0x1ff9a4c in zval_refcount_p /home/dmitry/php/php-master/Zend/zend_types.h:1310
    #2 0x1fff2fa in zval_call_destructor /home/dmitry/php/php-master/Zend/zend_execute_API.c:215
    #3 0x20f6390 in zend_hash_reverse_apply /home/dmitry/php/php-master/Zend/zend_hash.c:2174
    #4 0x1fff847 in shutdown_destructors /home/dmitry/php/php-master/Zend/zend_execute_API.c:260
    #5 0x2077324 in zend_call_destructors /home/dmitry/php/php-master/Zend/zend.c:1269
    #6 0x1df8c08 in php_request_shutdown /home/dmitry/php/php-master/main/main.c:1826
    #7 0x2808fe8 in do_cli /home/dmitry/php/php-master/sapi/cli/php_cli.c:1136
    #8 0x2809f99 in main /home/dmitry/php/php-master/sapi/cli/php_cli.c:1340
    #9 0x7f848424a50f in __libc_start_call_main (/usr/lib64/../lib64/libc.so.6+0x2750f)
    #10 0x7f848424a5c8 in __libc_start_main@GLIBC_2.2.5 (/usr/lib64/../lib64/libc.so.6+0x275c8)
    #11 0x608854 in _start (/home/dmitry/php/php-master/CGI-DEBUG-64/sapi/cli/php+0x608854)

@Girgias
Copy link
Member Author

Girgias commented Jul 24, 2023

@iluuu1994 has already told me about those and I did do some work on them. Will continue this week

@dstogov
Copy link
Member

dstogov commented Sep 13, 2023

There is yet another problem related to this - https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=62294
The problem occurs without JIT as well.

<?php
set_error_handler(function() {
    unset($GLOBALS['x']);
});
$x=" ";
var_dump(--$x);
?>
$ php-8.3/sapi/cli/php -d opcache.jit=0 fuzz-62294.php 
UNKNOWN:0

cc @iluuu1994 @arnaud-lb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants