Hi Bob, The patch breaks few tests:
> Bug #64267 (CURLOPT_INFILE doesn't allow reset) [ext/curl/tests/bug64267.phpt] > Bug # #68937 (Segfault in curl_multi_exec) [ext/curl/tests/bug68937.phpt] > ReflectionParameter::isDefaultValueConstant() && getDefaultValueConstantName() [ext/reflection/tests/ReflectionParameter_DefaultValueConstant_basic1.phpt] > Bug #51800 proc_open on Windows hangs forever, the right way to do it with more data [ext/standard/tests/stream/proc_open_bug51800_right2.phpt] > executing a file with -F [sapi/cli/tests/010.phpt] Please, revert it. Thanks. Dmitry, On Mon, Feb 9, 2015 at 3:49 PM, Bob Weinand <bobw...@hotmail.com> wrote: > Okay, > > I think I’ll leave the patch now as is and then, in case where it’d be > necessary, you can quickly add another CG(compiler_options) (won’t break > ABI etc., so should be safe to do). > > Bob > > Am 9.2.2015 um 13:42 schrieb Dmitry Stogov <dmi...@zend.com>: > > this may work, but checking the list of enabled/disabled extensions may be > too expensive. > > Thanks. Dmitry. > > On Mon, Feb 9, 2015 at 3:34 PM, Xinchen Hui <xinche...@zend.com> wrote: > >> Hey: >> >> On Mon, Feb 9, 2015 at 8:30 PM, Dmitry Stogov <dmi...@zend.com> wrote: >> > >> > >> > On Mon, Feb 9, 2015 at 2:18 PM, Bob Weinand <bobw...@hotmail.com> >> wrote: >> >> >> >> Hey >> >> >> >> You’re actually always substituting a CONST_PERSISTENT in case the >> >> constant is an exact match: >> >> >> http://lxr.php.net/xref/PHP_TRUNK/ext/opcache/Optimizer/block_pass.c#33 >> >> >> >> >> >> That not more than ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION is doing >> >> *currently* (without my patch), all you’re doing is duplicating the >> logic we >> >> already have in compiler. >> > >> > >> > yeah, this is a duplication from the time, when opcache was developed >> > separately. >> > >> >> >> >> >> >> Normal constants aren’t marked as CONST_PERSISTENT, so it should be >> safe >> >> for your means. >> >> >> >> So, well, I *can* split it, but I don’t see no point in it. Are you >> sure >> >> you need it? Even in case of storing to disk, the only substituted >> constants >> >> are internal, never changing constants... >> > >> > >> > This is not true. Imagine, we removed some php extension then restarted >> PHP. >> > The persistent constants defined in that extension are not valid >> anymore. >> actually, this is not a problem >> >> we can added some signatures about what the php version, >> debug/nodebug, what extensions are loaded, and their versions to the >> header fields of a generated file.. >> >> then check that infos while loading, if not-match, then just invalid it.. >> >> thanks >> > >> > Thanks. Dmitry. >> > >> >> >> >> >> >> Bob >> >> >> >> Am 9.2.2015 um 08:27 schrieb Dmitry Stogov <dmi...@zend.com>: >> >> >> >> Hi Bob, >> >> >> >> In general we may reuse PHP compiler to generate bytecode, then >> serialize >> >> it, and then run on different PHP installation. >> >> The other PHP may have different set of extensions and different >> constants >> >> values. >> >> So we substitute only persistent constants marked with CONST_CT_SUBST >> >> flag. >> >> May be had some other related issues that I forgot... >> >> >> >> opcache disables constant substitution, because constant may be >> defined in >> >> another file. >> >> if that file is changed the constant value may be not updated >> correctly. >> >> Access a.php, then change it, then reload, the output value must be >> valid. >> >> >> >> a.php >> >> ==== >> >> define("C", 5); >> >> include("b.php"); >> >> >> >> b.php >> >> ==== >> >> var_dump(C); >> >> >> >> I'm not sure if your patch handles all possible cases correctly. >> >> Class constants may be defined in other files as well, but opcache must >> >> hide them. >> >> So, most probably it should work. >> >> >> >> I think we may split ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION into two >> >> different options. >> >> - disable persistent constants and persistent class constants >> substitution >> >> (opcache won't set it), but if we extend opcache to store data on >> disk, we >> >> will have to set it. >> >> - disable substitution of constants and class constants defined in >> other >> >> files >> >> >> >> Can you take care about this? >> >> >> >> Thanks. Dmitry. >> >> >> >> On Sat, Feb 7, 2015 at 2:47 PM, Bob Weinand <bobw...@hotmail.com> >> wrote: >> >>> >> >>> Hey >> >>> >> >>> >> >>> >> https://github.com/bwoebi/php-src/compare/better-ct-constants#diff-3a8139128d4026ce0cb0c86beba4e6b9L1172 >> >>> >> >>> I’m chaging the meaning of ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION from >> >>> prevent substitution of persistent constants to prevent substitution >> of >> >>> other constants, and now always substitute persistent constants. >> >>> I’m not sure if this makes any problems with opcache, so I’m asking… >> >>> >> >>> See this commit: >> >>> >> https://github.com/php/php-src/commit/ed2d3e4c7ede51ba6102afd50301ab3c06280d3a >> >>> >> >>> Just wondering why there was checked for >> >>> ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION? These constants really are >> always the >> >>> same… >> >>> Also, I see that opcache sets this flag, but later substitutes itself >> >>> persistent constants ?? (Looks like meaning of this constant was >> >>> misinterpreted to me, but as it anyway already was guarded against, >> not an >> >>> issue?) >> >>> >> >>> May you please check whether this commit should be safe to be pushed >> to >> >>> php-src/master? >> >>> >> >>> Thanks, >> >>> Bob >> > >> > >> >> >> >> -- >> Xinchen Hui >> @Laruence >> http://www.laruence.com/ > > > > >
