php / php-src

The PHP Interpreter
https://www.php.net
Other
38.22k stars 7.75k forks source link

PHP 8.4 min function fails on typed integer #14873

Closed Alkarex closed 4 months ago

Alkarex commented 4 months ago

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)

nielsdos commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

I attempted a fix in https://github.com/php/php-src/pull/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 commented 4 months ago

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

nielsdos commented 4 months ago

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