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

Fix GH-15901: phpdbg: Assertion failure on i funcs #15929

Closed
wants to merge 2 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Sep 16, 2024

New hash tables are not automatically packed, so we must not treat them as such. Therefore we guard the foreach appropriately.

New hash tables are not automatically packed, so we must not treat them
as such.  Therefore we guard the foreach appropriately.
@cmb69 cmb69 linked an issue Sep 16, 2024 that may be closed by this pull request
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

HT_IS_INITIALIZED() would make slightly more sense to me, but this looks fine too.

@cmb69
Copy link
Member Author

cmb69 commented Sep 17, 2024

HT_IS_INITIALIZED() would make slightly more sense to me, […]

Ah, right, when reading the code this is better. However, for maintenance using HT_IS_PACKED() might be better, since using ZEND_HASH_PACKED_FOREACH_PTR on an arbitrary initialized array would not work (I think).

@iluuu1994
Copy link
Member

Indeed, but then at least we'd crash, rather than just silently not hand the array.

@cmb69
Copy link
Member Author

cmb69 commented Sep 18, 2024

Makes sense. Changed to HT_IS_INITIALIZED().

@cmb69
Copy link
Member Author

cmb69 commented Sep 18, 2024

The macOS CI failure seems to be unrelated to the PR.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

LGTM
and yeah, curl has started failing on macOS since yesterday or so, it's unrelated

@cmb69 cmb69 closed this in 422aa17 Sep 18, 2024
@cmb69 cmb69 deleted the cmb/gh15901 branch September 18, 2024 21:53
@cmb69
Copy link
Member Author

cmb69 commented Sep 18, 2024

curl has started failing on macOS since yesterday or so

Should we update the PHP-8.2 macOS CI?

@iluuu1994
Copy link
Member

@cmb69 Do you know what the cause is? I have no clue. The failures are seemingly random, although it's always the same tests that fail.

@cmb69
Copy link
Member Author

cmb69 commented Sep 19, 2024

@iluuu1994
Copy link
Member

I see around 20 successfully macOS builds after that commit, so it seems unlikely. But who knows?

@cmb69
Copy link
Member Author

cmb69 commented Sep 19, 2024

See #15958. If that fails, it's something else; if it succeeds, well, who knows? :)

@cmb69
Copy link
Member Author

cmb69 commented Sep 19, 2024

Well, #15958 failed; we may continue the discussion there.

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.

phpdbg: Assertion failure on i funcs
3 participants