[PHP-INTERNALS] Bison 3 support for PHP 5.5

Could you explain your changes a bit? You removed %pure-parser, for
example, and you shuffled globals around. This looks like more than just
adding support for Bison 3.

-Rasmus

Hi Rasmus,
I replaced %pure-parser (which was deprecated) with %pure_parser (its replacement). (Also, YYERROR_VERBOSE, which also was deprecated, with %error-verbose.)

Most of the changes in the PR revolve around this change in Zend/zend_language_parser.y:
-%code requires {
-#ifdef ZTS
-# define YYPARSE_PARAM tsrm_ls
-# define YYLEX_PARAM tsrm_ls
-#endif
-}
+%parse-param { void *tsrm_ls }
+%lex-param { void *tsrm_ls }

YYPARSE_PARAM and YYLEX_PARAM were deprecated, and then removed, and replaced with %parse-param and %lex-param. However, you cannot (or I did not see a way to) conditionally define parameters, as PHP was previously doing here.

So instead, I created the TSRMLS_DN and TSRMLS_CN macros which, in the ZTS case, work like TSRMLS_D/TSRMLS_C, but when ZTS is off, still resolve to a parameter (that must always be present), which is passed NULL.

-John

Hi!
I don't think it is a very good idea to add extra parameter that does
nothing to every function. There should be a better solution to this.

Hi Stas,
-John

I agree with Stas that the way how it's done at the moment is pretty ugly.

I was dealing with similar problem in jsond parser (
https://github.com/bukka/php-jsond/blob/master/jsond_parser.y ) where I
decided to use a structure (php_json_parser) for general parser info as
well as scanner data and TSRMLS if ZTS defined. It means that data from the
scanner are passed to the parser through that structure instead of using
global variable (that's the way how it's done in PHP). tsrm_ls can be set
using TSRMLS_SET_CTX fetched from that structure TSRMLS_FETCH_FROM_CTX.

So to sum it up... I think that a better solution would be using a
structure that would contain scanner data (_zend_php_scanner_globals ) and
TSRMLS if ZTS defined. It would be a cleaner solution IMHO. It could also
allow some optimization for the compiler as local variables might be saved
in registers (not sure about this one... :) ).

However this is probaly more something for phpng as it requires a bit more
changes... :)

Jakub

Hi John,
Thanks for your work, always good to support less archaic version of bison.

Some comments:

- does it still work with previous versions? If yes, do you know which
or which minimum version we should then require?
- 5.5 is stable, I am not sure it is a good idea to do these changes
there. 5.6 may be a good candidate (but has to be done quickly, RMs
can confirm if it is still possible)

Cheers,

Pierre,
-John

That's a no-go for 5.5.
5.5 is stable, 1year old (at this email date), and we should not
change such a thing into this stable branch.

For 5.6, we are actually in RC stage (with first RC to come soon).
Seeing the comments saying that it's all compatible and tests have
passed, I'm not against requiring bison 3.x for PHP 5.6.
Waiting for Ferenc's answer ;-)

Thank you for your patch and your support to make PHP better.

Julien Pauli

Hi Julien and Ferenc,
For your convenience, I’ve made a pull request for these changes against master (which would likely prevent at least one merge conflict vs. merging the 5.5-based pull request into master):

https://github.com/php/php-src/pull/685

I would point out that both pull requests only add Bison 3.x support; they don’t prevent Bison 2.x from working, so unless you have another reason for wanting to remove Bison 2.x support, this change doesn’t require dropping it. (I wouldn’t necessarily advocate dropping Bison 2.x support since we’re not actually using any Bison 3-specific features.).

-John

2014.05.23. 13:59, "Julien Pauli" <jpauli@php.net> ezt írta:
support for Bison 3.0 to PHP 5.5.
Bison 2.4 (minimum version for PHP 5.5 and 5.6), 2.7, and 3.0, with and
without --enable-maintainer-zts against the current PHP-5.5 and (with minor
changes) master branches.
just
replacement). (Also, YYERROR_VERBOSE, which also was deprecated, with
%error-verbose.)
Zend/zend_language_parser.y:
replaced with %parse-param and %lex-param. However, you cannot (or I did
not see a way to) conditionally define parameters, as PHP was previously
doing here.
ZTS case, work like TSRMLS_D/TSRMLS_C, but when ZTS is off, still resolve
to a parameter (that must always be present), which is passed NULL.
bison.
I would be fine with this change, even it happens after the first RC, given
that it could only affect people, who are building php from source and not
using the official release tarballs.

given that it could only affect people, who are building php from source
and not using the official release tarballs.

I think it is fine for 5.6 as well.

However issues can also happen in the generated files. We experienced that
a couple of times already.

Cheers,
Pierre

We were talking about 5.6 ;-)

I think it is fine. We'll have several RC to test that anyway.
I don't consider that as beeing a huge critical change, so it's all
right for 5.6.

Who does the merge ? :-)

Julien Pauli

Right, but it's going to be supported for at least another year. And
right now, on the latest Ubuntu's with only bison 3 you can't actually
build it. Because of that, a patch like this should absolutely be
considered.

cheers,
Derick

Why not just disable checking of bison version in stable versions. We
distribute generated tab files and there is no point to fail the configure
step just because someone cannot create tab file. The bison is used only if
the *.y files are changed. The thing is that such change is not allowed for
stable branches anyway. In case someone really wants to play with grammar
on old branches, then he/she should be able to install a correct version of
bison. But I think that we can count number of such users on one hand.

Just disabling version check is only config change and should not be a
problem for stable branches IMHO.

We can add a proper support for bison3 to phpng. The current patch seems to
me more like a hack than a proper solution...

Thoughts?

Jakub

Ooops. We don't actaully distribute tab files. Thought that we do... :)

Wouldn't be better to add them to the git? What's the actual reason to not
to add them. The same thing we do for scanner files already. I'm not aware
that bison would generate some platform specific tab files...?

Regards

Jakub

We don't in git ut snapshots and releases. Git is only for developers.
Having generated files in version control is questionable as this leads
to annoyance when timestamps come out wrong they are recreated using a
different tool version and then are being committed or have conflicts
"all the time".

johannes

That would make sense if we didn't have re2c generated files in the repo
already. Anyway I see your point. Bison version varies across the systems
more than re2c so the diffs in files would be more often which is not a
good idea...

The solution could be to stick just with one version of bison for people
that want to do changes to the grammar. It should be possible fail the make
phase (not configuration) when the version is different (for example empty
YACC variable when the version is different and then add check to
Makefile.frag or something like that). During the configuration you would
just get a warning. But that's a bit off topic and just an idea that could
be considered only for new major version... :)

Jakub

The main reason is that re2c is extremely uncommon whereas flex&bison
are common tools for developer meta packages from distributions. Also
historically re2c had only been used for unserialisation which is way
less touched than other scanners and parsers. Maybe when switching from
flex to re2c for ini and source scanners this decision might have been
revised (and I think there actually was a debate)

johannes

I find the generated files in GIT annoying. Almost every single time I
compile I now have to changed files that GIT bitches about. I would be
all for removing generated files from the repository.

cheers,
Derick

+1. I don't think re2c is that onerous a requirement anyway, for the
most part: it's available through apt-get, brew, yum, and probably
most other packaging systems. Given the amount of other things a
developer has to install to build php-src from git, re2c is hardly
going to break the camel's back.

Adam

The patch (as proposed) changes the ABI for zendlex and zendparse. As such
I don't think it's applicable to PHP 5.5 in this form.

Nikita

Agreed, but as Derick said, there's definitely an argument for some
sort of Bison 3 compatibility in 5.5.

John, can you see a way — however hackish — to get Bison 3 support in
5.5 without changing those functions?

Thanks,

Adam

My $0.02: don't touch PHP 5.5. I think it is reasonable to expect
people to install a new version of a piece of software if their OS
doesn't ship it by default. Mac OS X users have to do this all the
time anyway.

If we want to fix this up for PHP 5.5, I think I see a way, and it’ll be super-hackish, but I think I can make it work. Basically, where necessary, the six functions affected (zendlex, zendparse, zenderror, ini_lex, ini_parse, and ini_error) would be given new names, and the old names would be wrapper functions with the old ABI that call-through (and either pass in NULL or TSRM_LS as appropriate). PHP internally would call the new names; the only names/wrapper functions would be there for ABI compatibility only.

If that sounds reasonable, I’ll go ahead and give that a try, and if it works, we can merge that implementation into 5.5 and 5.6, and then I can revert the really hacky bits in the 5.6 branch since they’re not necessary because we can change the ABI.


Two notes:

At the least, I need to update the 5.6 version of the PR to account for that zenderror is exported as phperror, and the definition in main/php.h is wrong. (I think; following the macros is a bit confusing.) I’ll take care of that later tonight.

I’d also suggest that perhaps in >= 5.6, those functions shouldn’t even be exported as part of the public API. In php-src, they’re basically treated as internal implementation details. zenderror and zendlex are only ever called directly from the parser, and zendparse is only ever called directly from the compile_(file|filename) functions in the scanner. (That doesn’t mean they aren’t called from external modules, but there’s a lot of setup and teardown you have to do besides, so it’d certainly be a quite advanced use. Does anyone know if those functions actually are used by pecl or other third-party modules?)

-John

Hi!
I think this is reasonable, though making them inaccessible to modules
compiled with PHP would not be possible, I think. But none of those seem
to be suited for external consumption, so I think it's OK to make them
more private.