Skip to content
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

PHP 8.4 min function fails on typed integer #14873

Closed
Alkarex opened this issue Jul 8, 2024 · 5 comments
Closed

PHP 8.4 min function fails on typed integer #14873

Alkarex opened this issue Jul 8, 2024 · 5 comments

Comments

@Alkarex
Copy link

Alkarex commented Jul 8, 2024

Description

Hello,
The min() function seems to fail (returns null) on a typed integer, but only in specific situations and environments.

Notes:

  1. It fails when called from Apache module php84-apache2 (i.e. from a Web browser) but not when called from CLI (command line).
  2. It fails with a typed int parameter (function testMin(int $value)) but not on untyped parameter (function testMin($value)).

The following code:

<?php

function testMin(int $value): int {
	$value = min($value, 100);
	return $value;
}

echo testMin(5);

Resulted in this output:

PHP Fatal error:  Uncaught TypeError: testMin(): Return value must be of type int, null returned in ./test-min.php:5
Stack trace:
#0 ./test-min.php(8): testMin()
#1 {main}
  thrown in ./test-min.php on line 5

But I expected this output instead:

5

System information:

# cat /etc/issue 
Welcome to Alpine Linux 3.21.0_alpha20240606 (edge)
Kernel \r on an \m (\l)

# php -v
PHP 8.4.0alpha1 (cli) (built: Jul  4 2024 10:02:18) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.4.0-dev, Copyright (c) Zend Technologies
    with Zend OPcache v8.4.0alpha1, Copyright (c), by Zend Technologies

PHP Version

PHP 8.4.0alpha1

Operating System

Alpine Linux 3.21.0_alpha20240606 (edge)

Alkarex added a commit to Alkarex/FreshRSS that referenced this issue Jul 8, 2024
Sole fix needed so far seems to be related to https://wiki.php.net/rfc/deprecate-implicitly-nullable-types

See also upstream PR PhpGt/CssXPath#227

We are also hitting was seems to be a PHP bug php/php-src#14873
@nielsdos
Copy link
Member

nielsdos commented Jul 8, 2024

Also reproduces in CLI webserver. I'll take a look... I think this is related to frameless function calls.
EDIT: only reproduces with opcache on
EDIT2: gets broken by the DFA optimizer pass somehow

@nielsdos
Copy link
Member

nielsdos commented Jul 8, 2024

The problem is that this line in the VM: ZVAL_NULL(result); changes the type of arg1 as well, because after the DFA pass the result and input both use CV0($result).

@nielsdos nielsdos self-assigned this Jul 8, 2024
nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 8, 2024
The DFA pass may cause the op1 and result argument to be equal to each
other. In the VM we always use ZVAL_NULL(result) first, which will also
destroy the first argument. Use a temporary result to fix the issue.
@nielsdos nielsdos linked a pull request Jul 8, 2024 that will close this issue
nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 8, 2024
The DFA pass may cause the op1 and result argument to be equal to each
other. In the VM we always use ZVAL_NULL(result) first, which will also
destroy the first argument. Use a temporary result to fix the issue.
nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 8, 2024
The DFA pass may cause the op1 and result argument to be equal to each
other. In the VM we always use ZVAL_NULL(result) first, which will also
destroy the first argument. Use a temporary result to fix the issue.
nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 8, 2024
The DFA pass may cause the op1 and result argument to be equal to each
other. In the VM we always use ZVAL_NULL(result) first, which will also
destroy the first argument. Use a temporary result to fix the issue.
nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 8, 2024
The DFA pass may cause the op1 and result argument to be equal to each
other. In the VM we always use ZVAL_NULL(result) first, which will also
destroy the first argument. Use a temporary result to fix the issue.
@nielsdos nielsdos removed their assignment Jul 8, 2024
@nielsdos
Copy link
Member

nielsdos commented Jul 8, 2024

I attempted a fix in #14876, but got stuck on the JIT which fails on arm64 with "stack smashing detected". And I don't feel like digging into it more. The VM part of my patch works fine, it's just the JIT stuff that needs more work. Someone else can pick up where I left off with my PR.

@Alkarex
Copy link
Author

Alkarex commented Jul 9, 2024

Note: for some reason, the max() function does not seem affected

Alkarex added a commit to FreshRSS/FreshRSS that referenced this issue Jul 9, 2024
* Initial support for PHP 8.4
Sole fix needed so far seems to be related to https://wiki.php.net/rfc/deprecate-implicitly-nullable-types

See also upstream PR PhpGt/CssXPath#227

We are also hitting was seems to be a PHP bug php/php-src#14873

* Fix return type

* Disable OPCache while waiting for PHP fix
@nielsdos
Copy link
Member

It seems like working around this in the optimizer is as simple as changing opline_supports_assign_contraction on first sight. Let me try...

nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 10, 2024
The problem is that this line in the VM: `ZVAL_NULL(result);` changes the type
of arg1 as well, because after the DFA pass the result and input both use
CV0($result).
We should not contract assignments with CVs in frameless calls with
arguments.
An older attempt is found at phpGH-14876 that tried to modify the VM/JIT.
@nielsdos nielsdos linked a pull request Jul 10, 2024 that will close this issue
nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 10, 2024
The problem is that this line in the VM: `ZVAL_NULL(result);` changes the type
of arg1 as well, because after the DFA pass the result and input both use
CV0($result).
We should not contract assignments with CVs in frameless calls with
arguments.
An older attempt is found at phpGH-14876 that tried to modify the VM/JIT.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants