Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use standard boolean type as zend_bool typedef #5624

Closed
wants to merge 4 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented May 25, 2020

As we are now using C99 and bool is always defined in the stdbool.h header, make zend_bool a typedef of a standard boolean.

This catches some funky usages of zend_bool which I still have trouble debugging.

Things which need to be handled/fixed:

  • Parser being confused about the short hand syntax [] for nested lists (fails with Fatal error: Cannot mix [] and list():
    I.e. [$a, [$b]] = array(new stdclass, array(new stdclass));
    See Zend/tests/list_001.phpt (or Zend/tests/list/list_reference_001.phpt and Zend/tests/list/list_reference_009.phpt
    Fixed by: baa2858
  • JIT related failures (mostly type declarations and GC)

@Girgias Girgias force-pushed the zend_bool-as-stdbool branch 2 times, most recently from 1ead3d8 to 815d573 Compare May 26, 2020 14:01
Zend/zend_globals.h Outdated Show resolved Hide resolved
@Girgias
Copy link
Member Author

Girgias commented May 26, 2020

Legit test failures in extensions:

DOM:

========DIFF========
002+ <r:root xmlns:r="urn::root"><data xmlns="urn::data">aaa</data></r:root>
002- <r:root xmlns:r="urn::root"><data xmlns="urn::data"/></r:root>
========DONE========
FAIL Bug #47849 (Non-deep import loses the namespace). [ext/dom/tests/bug47849.phpt]

Fixed in: 706d4f3

JIT build is still broken in regards to type declarations and GC:

Ensure type hints are enforced for functions invoked as callbacks. [tests/classes/type_hinting_004.phpt]
ZE2 type hinting [tests/lang/type_hints_002.phpt]
Bug #63635 (Segfault in gc_collect_cycles) [Zend/tests/bug63635.phpt]
Bug #68446 (Array constant not accepted for array parameter default) [Zend/tests/bug68446.phpt]
Testing type-hinted lambda parameter inside namespace [Zend/tests/ns_074.phpt]
Testing parameter type-hinted with default value inside namespace [Zend/tests/ns_070.phpt]
Testing parameter type-hinted (array) with default value inside namespace [Zend/tests/ns_071.phpt]
Testing parameter type-hinted with interface [Zend/tests/ns_072.phpt]
Testing type-hinted lambda parameter inside namespace [Zend/tests/ns_073.phpt]
Type errors for methods from traits should refer to using class [Zend/tests/trait_type_errors.phpt]
callable type#003 [Zend/tests/type_declarations/callable_003.phpt]
Scalar type - default via constants [Zend/tests/type_declarations/scalar_constant_defaults.phpt]
Scalar type - default via constants - error condition [Zend/tests/type_declarations/scalar_constant_defaults_error.phpt]

All of these fail with a Segmentation faults (Termsig=11) except tests/lang/type_hints_002.phpt which fails with an Illegal Instruction (Termsig=4)

Fixed in: 13909e5

@Girgias Girgias force-pushed the zend_bool-as-stdbool branch 2 times, most recently from 5656643 to 64a79f0 Compare June 2, 2020 23:34
ext/dom/tests/bug47849.phpt Outdated Show resolved Hide resolved
@Girgias Girgias force-pushed the zend_bool-as-stdbool branch 2 times, most recently from 17aa3ed to 1d90ab6 Compare June 5, 2020 10:22
@Girgias
Copy link
Member Author

Girgias commented Jun 5, 2020

@dstogov can you have a look, or point me in the direction where to look, in regards to the JIT related segfaults? From what I seemed to have understood there is a NULL pointer which gets passed around when it is trying to do some type checks, Nikita had cursory look and didn't find any obvious misuse of zend_bool which could cause this.

Also the backtraces aren't that great as they look like this (this one in particular I got by using tests/lang/type_hints_002.sh gdb):

Program received signal SIGSEGV, Segmentation fault.
0x000000007fffffff in ?? ()
(gdb) bt
#0  0x000000007fffffff in ?? ()
#1  0x00000000086ce138 in ?? ()
#2  0x00007ffffb25b200 in ?? ()
#3  0x00007ffffffea2f0 in ?? ()
#4  0x00007ffffb215020 in ?? ()
#5  0x800000000002b614 in ?? ()
#6  0x00000000fb25b2e0 in ?? ()
#7  0x0000000000000000 in ?? ()

@dstogov
Copy link
Member

dstogov commented Jun 8, 2020

@Girgias I have fixed the problem in master.

@Girgias Girgias marked this pull request as ready for review June 8, 2020 11:43
@Girgias
Copy link
Member Author

Girgias commented Jun 8, 2020

Now that the segfaults are fixed I would just like an opinion on the way I deal with the error output stream, any better way comes to mind to someone?

@nikic
Copy link
Member

nikic commented Jun 8, 2020

@Girgias Wouldn't just converting display_errors to zend_uchar work?

@@ -81,7 +81,8 @@ struct _zend_compiler_globals {

HashTable *auto_globals;

zend_bool parse_error;
/* Refer to zend_yytnamerr() in zend_language_parser.y for meaning of values */
unsigned int parse_error;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zend_uchar would be enough here.

@Girgias
Copy link
Member Author

Girgias commented Jun 8, 2020

@Girgias Wouldn't just converting display_errors to zend_uchar work?

It would, just felt weird that the error output stream is bundled with the fact that displaying errors is toggled by the same variable, but that just may be me, will change it to zend_uchar

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remaining test failure is unrelated. Please commit the parse_error/display_errors fixes separately from the typedef change.

@php-pulls php-pulls closed this in 4b77a15 Jun 9, 2020
@Girgias Girgias deleted the zend_bool-as-stdbool branch June 9, 2020 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants