@php/release-managers-84 this fixes a regression from #17712 that is part of the not-yet-released PHP 8.4.7. Should this be included in 8.4.7 rather than waiting for 8.4.8?

<adding the PHP 8.4 release managers as reviewers - if this should be included in 8.4.7 I'd rather one of you merge this, if it should wait until 8.4.8 let me know and I can merge it>

So it seems that something is setting ZEND_ACC_DEPRECATED in the flags for the constant (u2.constant_flags) rather than using CONST_DEPRECATED and the module number; ZEND_ACC_DEPRECATED is 1 << 11, so the module number is treated as 1 << 3 and thus not PHP_USER_CONSTANT
But that is because ZEND_CLASS_CONST_FLAGS() is using it to hold a bunch of flags, like private, final, etc., that aren't applicable to normal constants

Okay, I'm working on a fix now for the class constants

Obviously, we'll need to check why this segfaults. 🙂

Obviously, we'll need to check why this segfaults. 🙂

Segfault is from the module number being 8 because of ZEND_ACC_DEPRECATED

Ah sorry, I missed your earlier comment. I don't quite understand your explanation, and why that would cause a segfault now but not before. But it's way too late for me now, so I'll check tomorrow if you haven't already found the solution.

Ah sorry, I missed your earlier comment. I don't quite understand your explanation, and why that would cause a segfault now but not before. But it's way too late for me now, so I'll check tomorrow if you haven't already found the solution.

The segfault now is from infinite recursion. Because ZEND_ACC_DEPRECATED is 1<<11, ZEND_CONSTANT_MODULE_NUMBER() returns 3 rather than PHP_USER_CONSTANT (0x7fffff)

Ah ofc, makes sense.

Okay, this is a bit trickier that that, because a class constant needs to be able to have the following flags

1. ZEND_ACC_PUBLIC
2. ZEND_ACC_PROTECTED
3. ZEND_ACC_PRIVATE
4. ZEND_ACC_FINAL
5. ZEND_CLASS_CONST_IS_CASE
6. ZEND_ACC_DEPRECATED
   and, for the CONST_ flags, at least some of
7. CONST_PERSISTENT
8. CONST_NO_FILE_CACHE
9. CONST_DEPRECATED
10. CONST_OWNED
11. CONST_RECURSIVE
    plus, during evaluation
12. IS_CONSTANT_VISITED_MARK

So basically class constants can't store the module number in the top 3 bytes if they want to support all of the things, and I'll adjust so that we have 16 bits for the flags (which is what I thought was already the case when I wrote https://php.github.io/php-src/core/data-structures/zend_constant.html). This limits the number of modules possible to 2^16 rather than 2^24, which should still be enough.

I'll clean things up on master to be better documented

Do we need to store the module id at all? I see this was not done previously for class constants, so can we not just check for if (c->ce->type == ZEND_USER_CLASS) { instead? This PR is ABI breaking (ZEND_CONSTANT_MODULE_NUMBER() from extensions that aren't recompiled will incorrectly access the module number).

The analogous fix for global constants can use ZEND_CONSTANT_MODULE_NUMBER() instead.

Perhaps simpler to revert the problem (for 8.4.7RC2/8.4.7) and give more time for a proper fix in 8.4.8 ?

I can revert GH-17712 and postpone until 8.4.8 if RMs prefer. Let me know if I should do that. Otherwise, I believe the if (c->ce->type == ZEND_USER_CLASS) { approach should work.

Do we need to store the module id at all? I see this was not done previously for class constants, so can we not just check for if (c->ce->type == ZEND_USER_CLASS) { instead? This PR is ABI breaking (ZEND_CONSTANT_MODULE_NUMBER() from extensions that aren't recompiled will incorrectly access the module number).

The analogous fix for global constants can use ZEND_CONSTANT_MODULE_NUMBER() instead.

Where would we check c->ce->type? The recursion protection is also used by global constants where c->ce isn't present

@DanielEScherzer Sadly, you can't add them to the macros but you'll have to add it around them at call-site. Regardless, I think it's better to revert GH-17712 for now so that we can tag. I'll do that in approx. half an hour. We can then re-apply with this fix.

@SakiTakamachi Please cherry-pick 386ab1d for 8.4.7RC2. We'll reapply the patch for the 8.4 branch for 8.4.8 with the proper fix in the coming days. Edit: And please also d991215.

@DanielEScherzer Sadly, you can't add them to the macros but you'll have to add it around them at call-site. Regardless, I think it's better to revert GH-17712 for now so that we can tag. I'll do that in approx. half an hour. We can then re-apply with this fix.

Okay, added around the call site - new patch reapplies yours with my fixes

@SakiTakamachi Please cherry-pick 386ab1d for 8.4.7RC2. We'll reapply the patch for the 8.4 branch for 8.4.8 with the proper fix in the coming days. Edit: And please also d991215.

done!