Skip to content

Zend: Add tests for offsets and containers #12723

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

Merged
merged 30 commits into from
Jan 25, 2024
Merged

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Nov 19, 2023

This has highlighted some JIT bugs, I think I managed to fix 2 of them, but the following tests are marked as XFAIL:

=====================================================================
EXPECTED FAILED TEST SUMMARY
---------------------------------------------------------------------
Object offset behaviour [Zend/tests/offsets/strings/const_dimension/object_offset_behaviour.phpt]  XFAIL REASON: Object of class stdClass could not be converted to int warning with const offset in JIT
Object offset behaviour [Zend/tests/offsets/false/const_dimension/object_offset_behaviour.phpt]  XFAIL REASON: Bug Coalesce operator with constant offset emits isset/empty error in JIT
Object offset behaviour [Zend/tests/offsets/arrays/const_dimension/object_offset_behaviour.phpt]  XFAIL REASON: Bug Coalesce operator with constant offset emits isset/empty error in JIT
Object offset behaviour [Zend/tests/offsets/null/const_dimension/object_offset_behaviour.phpt]  XFAIL REASON: Bug Coalesce operator with constant offset emits isset/empty error in JIT
=====================================================================

Any suggestion to reduce the test duplication is also welcomed.

@iluuu1994
Copy link
Member

Were these tests generated? Can't we do something like Zend/tests/runtime_compile_time_binary_operands.phpt?

@Girgias
Copy link
Member Author

Girgias commented Nov 20, 2023

Were these tests generated? Can't we do something like Zend/tests/runtime_compile_time_binary_operands.phpt?

Nope, I wrote them by hand, as I wasn't planning on doing all types of containers and offsets generally.

Also, what is relevant is the output here, as it is, sadly, inconsistent. This is kinda related to #7173 and some other stuff I am planning to RFC to make the behaviour more consistent around offsets.

Edit: I could merge the int, float, and true cases as those behave identically.

Edit 2: I suppose I could write that sort of test to check the compile and run-time behaviour to be identical and keep the variable tests to check for the output.

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

I'm not sure, if this is the best way to keep tests.
Now I have to look into 3 or 4 files to just understand the difference.
Anyway, this is better than nothing.
It's great these test caught the bugs.

The two JIT related fixes look right.

Few XFAIL-ed tests with "Cannot access offset of type %s on array" vs "Cannot access offset of type %s in isset or empty" error messages mismatch seem related to your changes. Do you need my help to fix them?

@Girgias Girgias force-pushed the offset-bugs branch 2 times, most recently from a6a79c9 to 58cfe39 Compare November 20, 2023 12:49
@Girgias
Copy link
Member Author

Girgias commented Nov 20, 2023

I'm not sure, if this is the best way to keep tests. Now I have to look into 3 or 4 files to just understand the difference. Anyway, this is better than nothing. It's great these test caught the bugs.

I'm trying to work on a test that generates the compile time and run time behaviour so that a lot of the current tests can be removed, see Zend/tests/offsets/runtime_compile_time_offset_access.phpt however, this fails with the JIT as it seems like the call to file_put_contents($filename, makeTestFile($container_orig, $offset)); stops happening?

The two JIT related fixes look right.

Should I backport them to 8.2?

Few XFAIL-ed tests with "Cannot access offset of type %s on array" vs "Cannot access offset of type %s in isset or empty" error messages mismatch seem related to your changes. Do you need my help to fix them?

I don't think they are related to my changes, and yes I would need your help @dstogov as I think the issue is that in the normal VM the coalesce operator (??) use the read handler and thus provides BP_VAR_R to zend_illegal_container_offset(), whereas the JIT seems to always use zend_jit_fetch_dim_is_helper() even for the coalesce operator and thus passes BP_VAR_IS to zend_illegal_container_offset().

But I might also be wrong.

@dstogov
Copy link
Member

dstogov commented Nov 21, 2023

The two JIT related fixes look right.

Should I backport them to 8.2?

yes. Please do it in a separate PR with a minimal tests.

Few XFAIL-ed tests with "Cannot access offset of type %s on array" vs "Cannot access offset of type %s in isset or empty" error messages mismatch seem related to your changes. Do you need my help to fix them?

I don't think they are related to my changes, and yes I would need your help @dstogov as I think the issue is that in the normal VM the coalesce operator (??) use the read handler and thus provides BP_VAR_R to zend_illegal_container_offset(), whereas the JIT seems to always use zend_jit_fetch_dim_is_helper() even for the coalesce operator and thus passes BP_VAR_IS to zend_illegal_container_offset().

Could you please create a bug report with a minimal case(s) and assign it to me. I'll take care when have time.

@dstogov
Copy link
Member

dstogov commented Nov 22, 2023

Both JIT related issues are fixed. You may remove the XFAIL marks.

@Girgias
Copy link
Member Author

Girgias commented Nov 27, 2023

@dstogov can you have a look at the Zend/tests/offsets/runtime_compile_time_offset_access.phpt test? As it fails with the JIT, and I'm not sure why. :(

string(5) "value"
}

Deprecated: Automatic conversion of false to array is deprecated in /home/girgias/Dev/php-src/Zend/tests/offsets/appending_containers.php on line 17
Copy link
Member

Choose a reason for hiding this comment

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

The hard-coded path should be replaced by %s

@dstogov
Copy link
Member

dstogov commented Nov 27, 2023

@dstogov can you have a look at the Zend/tests/offsets/runtime_compile_time_offset_access.phpt test? As it fails with the JIT, and I'm not sure why. :(

This failure is not related to JIT but to opcache.file_update_protection=0 setting. The following two command produce different results.

$ sapi/cli/php -n -d zend_extension=opcache.so -d opcache.enable_cli=1 -d opcache.jit=0 -d "opcache.file_update_protection=0" Zend/tests/offsets/runtime_compile_time_offset_access.php
$ sapi/cli/php -n -d zend_extension=opcache.so -d opcache.enable_cli=1 -d opcache.jit=0 -d "opcache.file_update_protection=1" Zend/tests/offsets/runtime_compile_time_offset_access.php

@iluuu1994
Copy link
Member

iluuu1994 commented Nov 27, 2023

@Girgias Please xfail this test in the meantime if you can't solve the issue so the build stays green. Edit: Oops, I think I commented on the wrong PR. In any case, this seems to be fixed on release branches now, thanks 🙂

@Girgias
Copy link
Member Author

Girgias commented Nov 29, 2023

I might add some tests for unset() and more tests regarding appending a container after some peculiar behaviour @dseguy has shown me.

The only thing that I feel is slightly related to #7173 is the behaviour around isset($string[$INF]) that emits a float to int coercion warning but if the offset is 5.5 this doesn't happen which is slightly confusing.

@Girgias Girgias merged commit a479ed7 into php:master Jan 25, 2024
@Girgias Girgias deleted the offset-bugs branch January 25, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants