Re: [VOTE] Reclassifying engine warnings

This is only part of a thread. view whole thread
  107491
October 11, 2019 08:18 nikita.ppv@gmail.com (Nikita Popov)
On Thu, Sep 12, 2019 at 2:17 PM Nikita Popov ppv@gmail.com> wrote:

> Hi internals, > > I've opened the vote on //wiki.php.net/rfc/engine_warnings. > > There are 4 votes, all of them independent. The first 3 are for specific > cases that were controversial during the discussion, the last one is for > the remainder of the proposal. > > Voting closes 2019-09-26. > > Regards, > Nikita >
As people have expressed interest in hearing about direct technical benefits that these kinds of changes have ... let me give you an example that came up yesterday. Opcache performs a bunch of optimizations, and one class of optimizations it does are subsequent jumps on the same operand. For example: if ($x) { A; } if ($x) { B; } Currently, opcache will optimize the first if($x) condition to jump directly until after the second if($x) if the value is false, on the expectation that it is redundant to check the same condition twice in a row: The result is going to be the same. Basically the result is something like this: if ($x) { A; } else { goto end; } if ($x) { B; } end: Now, it turns out that this entire class of optimizations is technically illegal. Why? Because $x might be an undefined variable! That means that this optimization at the least loses an "undefined variable" notice, and at worse changes control flow: set_error_handler(function() { $GLOBALS['x'] = true; }); if ($x) echo "foo\n"; if ($x) echo "bar\n"; Because it's been around for years and doesn't seem to have caused any active issues, we're likely going to keep this, but nonetheless, it illustrates the kind of issue we see with these notices. Either an exception or nothing at all are fine, but notices caused problems. Of course there are also other problems, such as https://bugs.php.net/bug.php?id=78598, which is one of multiple use-after-free issues related to notices thrown during write operations. The root cause is that under the PHP memory model, it is not legal to run arbitrary user code while holding writable references into structures -- an invariant that is violated by some notices, such as the undefined array key one, because those notices may invoke error handlers. Again, either throwing nothing or throwing an exception would be unproblematic. Generally notices thrown by the engine are a pretty big pain to deal with, as well as something of a correctness and safety hazard. We have quite a few bugs in this area, though most of them are thankfully not likely to be hit by accident. Nikita
  107492
October 11, 2019 08:29 benjamin.morel@gmail.com (Benjamin Morel)
> > As people have expressed interest in hearing about direct technical > benefits that these kinds of changes have ... let me give you an example > that came up yesterday.
Too bad this example comes after the vote has been made, and failed. This would be a very strong argument in favour of using exceptions everywhere in the next major version: codebase cleanup, room for more optimization. Nikita, please fork PHP, we'll follow you ;-) — Benjamin
  107493
October 11, 2019 09:12 oludonsexy@gmail.com (Olumide Samson)
On Fri, Oct 11, 2019, 9:29 AM Benjamin Morel morel@gmail.com>
wrote:

> > > > As people have expressed interest in hearing about direct technical > > benefits that these kinds of changes have ... let me give you an example > > that came up yesterday. > > > > Too bad this example comes after the vote has been made, and failed. > This would be a very strong argument in favour of using exceptions > everywhere in the next major version: codebase cleanup, room for more > optimization. > > Nikita, please fork PHP, we'll follow you ;-) > > — Benjamin >
I think I'm always available to contribute to a fork of a better PHP, coz I love the syntax not the garbages included in the current one.
>
  107514
October 11, 2019 16:16 claude.pache@gmail.com (Claude Pache)
> Le 11 oct. 2019 à 11:12, Olumide Samson <oludonsexy@gmail.com> a écrit : > > On Fri, Oct 11, 2019, 9:29 AM Benjamin Morel morel@gmail.com> > wrote: > >>> >>> As people have expressed interest in hearing about direct technical >>> benefits that these kinds of changes have ... let me give you an example >>> that came up yesterday. >> >> >> >> Too bad this example comes after the vote has been made, and failed. >> This would be a very strong argument in favour of using exceptions >> everywhere in the next major version: codebase cleanup, room for more >> optimization. >> >> Nikita, please fork PHP, we'll follow you ;-) >> >> — Benjamin >> > > I think I'm always available to contribute to a fork of a better PHP, coz I > love the syntax not the garbages included in the current one.
If you’re seeking a fork of PHP that wilfully breaks BC for the sake of cleanup and optimisation, you should seriously consider Hack. Although I don’t know whether they’ve already removed support of the appalling implicit initialisation of variables to `null`, or of the dreadful backtick operator, you’ll be delighted to learn that they’re on the process of removing references, PHP arrays (in favour of Hack arrays and collections), and even that little pesky `https://hhvm.com/blog/2019/02/11/hhvm-4.0.0.html <https://hhvm.com/blog/2019/02/11/hhvm-4.0.0.html> Enjoy. (But not with me: our company does not have the budget to migrate 30 Mo of code without counting external libraries.) —Claude
  107549
October 15, 2019 08:27 gertp93@gmail.com (Gert)
I may be a bit late to this party, but will the promotion to error
exception of illegal offsets also happen for using resources as array
keys? These currently produce a notice, and are then casted to an
integer: https://3v4l.org/cQ8hf
Or will the behaviour of that remain the same?

On Fri, 11 Oct 2019 at 18:16, Claude Pache pache@gmail.com> wrote:
> > > > > Le 11 oct. 2019 à 11:12, Olumide Samson <oludonsexy@gmail.com> a écrit : > > > > On Fri, Oct 11, 2019, 9:29 AM Benjamin Morel morel@gmail.com> > > wrote: > > > >>> > >>> As people have expressed interest in hearing about direct technical > >>> benefits that these kinds of changes have ... let me give you an example > >>> that came up yesterday. > >> > >> > >> > >> Too bad this example comes after the vote has been made, and failed. > >> This would be a very strong argument in favour of using exceptions > >> everywhere in the next major version: codebase cleanup, room for more > >> optimization. > >> > >> Nikita, please fork PHP, we'll follow you ;-) > >> > >> — Benjamin > >> > > > > I think I'm always available to contribute to a fork of a better PHP, coz I > > love the syntax not the garbages included in the current one. > > If you’re seeking a fork of PHP that wilfully breaks BC for the sake of cleanup and optimisation, you should seriously consider Hack. Although I don’t know whether they’ve already removed support of the appalling implicit initialisation of variables to `null`, or of the dreadful backtick operator, you’ll be delighted to learn that they’re on the process of removing references, PHP arrays (in favour of Hack arrays and collections), and even that little pesky ` > https://hhvm.com/blog/2019/02/11/hhvm-4.0.0.html <https://hhvm.com/blog/2019/02/11/hhvm-4.0.0.html> > > Enjoy. (But not with me: our company does not have the budget to migrate 30 Mo of code without counting external libraries.) > > —Claude > >
  107550
October 15, 2019 10:31 cmbecker69@gmx.de ("Christoph M. Becker")
On 15.10.2019 at 10:27, Gert wrote:

> I may be a bit late to this party, but will the promotion to error > exception of illegal offsets also happen for using resources as array > keys? These currently produce a notice, and are then casted to an > integer: https://3v4l.org/cQ8hf > Or will the behaviour of that remain the same?
Only objects and arrays will throw; resources are promoted from E_NOTICE to E_WARNING. -- Christoph M. Becker
  107498
October 11, 2019 11:32 andreas@dqxtech.net (Andreas Hennings)
On Fri, 11 Oct 2019 at 10:18, Nikita Popov ppv@gmail.com> wrote:
> > On Thu, Sep 12, 2019 at 2:17 PM Nikita Popov ppv@gmail.com> wrote: > > > Hi internals, > > > > I've opened the vote on //wiki.php.net/rfc/engine_warnings. > > > > There are 4 votes, all of them independent. The first 3 are for specific > > cases that were controversial during the discussion, the last one is for > > the remainder of the proposal. > > > > Voting closes 2019-09-26. > > > > Regards, > > Nikita > > > > As people have expressed interest in hearing about direct technical > benefits that these kinds of changes have ... let me give you an example > that came up yesterday. > > Opcache performs a bunch of optimizations, and one class of optimizations > it does are subsequent jumps on the same operand. For example: > > if ($x) { A; } > if ($x) { B; } > > Currently, opcache will optimize the first if($x) condition to jump > directly until after the second if($x) if the value is false, on the > expectation that it is redundant to check the same condition twice in a > row: The result is going to be the same. Basically the result is something > like this: > > if ($x) { A; } else { goto end; } > if ($x) { B; } > end: > > Now, it turns out that this entire class of optimizations is technically > illegal. Why? Because $x might be an undefined variable! That means that > this optimization at the least loses an "undefined variable" notice, and at > worse changes control flow: > > set_error_handler(function() { > $GLOBALS['x'] = true; > }); > if ($x) echo "foo\n"; > if ($x) echo "bar\n";
To be fair, the same problem would still apply for other code that emits notices in an if condition, right? Or does the opcache only optimize this for simple variables? The "correct" behavior would be to analyse the code before the if(), and only optimize if we are sure that $x will always be defined.. Otherwise, we would need to convert it to if ($x) {..} elseif (variable_exists($x)) {goto end;} Sadly there is currently no variable_exists() in php, and the above code would probably lose the optimization benefit with the extra logic.
> > Because it's been around for years and doesn't seem to have caused any > active issues, we're likely going to keep this, but nonetheless, it > illustrates the kind of issue we see with these notices. Either an > exception or nothing at all are fine, but notices caused problems. > > Of course there are also other problems, such as > https://bugs.php.net/bug.php?id=78598, which is one of multiple > use-after-free issues related to notices thrown during write operations. > The root cause is that under the PHP memory model, it is not legal to run > arbitrary user code while holding writable references into structures -- an > invariant that is violated by some notices, such as the undefined array key > one, because those notices may invoke error handlers. Again, either > throwing nothing or throwing an exception would be unproblematic. > > Generally notices thrown by the engine are a pretty big pain to deal with, > as well as something of a correctness and safety hazard. We have quite a > few bugs in this area, though most of them are thankfully not likely to be > hit by accident. > > Nikita