Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:107498
MIME-Version: 1.0
References: <CAF+90c_Ds+UNZB4hAi1989SRafb=d1JkP2q4pdwkUUXfcHrLCw@mail.gmail.com>
 <CAF+90c_RCYvqpZkxWGyXiM1M+ZzwCh98JKtmed22gbA-OBUqaw@mail.gmail.com>
In-Reply-To: <CAF+90c_RCYvqpZkxWGyXiM1M+ZzwCh98JKtmed22gbA-OBUqaw@mail.gmail.com>
Date: Fri, 11 Oct 2019 13:32:13 +0200
Message-ID: <CAH0Uv3HNv5=98PP68qfzqEta-7VEx=rdNHnLAkCa6XtOu9+xCw@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] Re: [VOTE] Reclassifying engine warnings
From: andreas@dqxtech.net (Andreas Hennings)

On Fri, 11 Oct 2019 at 10:18, Nikita Popov <nikita.ppv@gmail.com> wrote:
>
> On Thu, Sep 12, 2019 at 2:17 PM Nikita Popov <nikita.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