-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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.
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
base: master
Are you sure you want to change the base?
Conversation
Filed to see possible regressions |
Just to clarify, based on your comment in https://bugs.php.net/bug.php?id=81689 this does not fix the ppc64le issue yet, right? |
Yes, this 2 tests still fail, but as I see other places with The whole build log could be found at https://gitlab.alpinelinux.org/alpine/aports/-/jobs/554235 |
@@ -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"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
Bug was fixed via 7773 but the PR still makes sense to fix int/long issue with |
Fixes https://bugs.php.net/bug.php?id=81689