Skip to content

Fix fibers tests on ppc64le #7710

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix fibers tests on ppc64le #7710

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Zend/zend_fibers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ static ZEND_STACK_ALIGNED void zend_fiber_execute(zend_fiber_transfer *transfer)
zend_fiber *fiber = EG(active_fiber);

/* Determine the current error_reporting ini setting. */
zend_long error_reporting = INI_INT("error_reporting");
int error_reporting = INI_INT("error_reporting");
Copy link
Member

Choose a reason for hiding this comment

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

zend_ini_long() returns zend_long, so why should we use a narrower type here?

Copy link
Member

@trowski trowski Dec 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This value is later assigned to EG(error_reporting), an int, so I suggested this change thinking perhaps there was something unusual happening where it wasn't being cast properly on that assignment because it was stored in a long.

Thinking about it further, I'm not sure this is origin of the problem anyway. The problem appears to be with saving and restoring EG(error_reporting), which is done within zend_fiber_capture_vm_state and zend_fiber_restore_vm_state. Given the types there, I'm not seeing why this is failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wondering as git grep INI_INT returns mostly int so it needs separate PR to normalize

btw gonna try 5459ed4 as it the fixed looks related

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it does not help too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As tests fixed, the remaining question is clean-up still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commit 492af9f adds new function for parsing ints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow it still needs to fix type or use cast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least counters could affect all little-endian arches

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmb69 any suggestion to fix it?

/* If error_reporting is 0 and not explicitly set to 0, INI_STR returns a null pointer. */
if (!error_reporting && !INI_STR("error_reporting")) {
error_reporting = E_ALL;
Expand Down