Reapply GH-17712 with a fix for internal class constants

[DanielEScherzer]

Add recursion protection when emitting deprecation warnings for class constants, since the deprecation message can come from an attribute that is using the same constant for the message, or otherwise result in recursion.

But, internal constants are persisted, and thus cannot have recursion protection. Otherwise, if a user error handler triggers bailout before the recursion flag is removed then a subsequent request (e.g. with --repeat 2) would start with that flag already applied. Internal constants can presumably be trusted not to use deprecation messages that come from recursive attributes.

FixesGH-18463
FixesGH-17711

[DanielEScherzer]

@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?

[iluuu1994]

I believe this should work. Thank you!

[DanielEScherzer]

<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>

[DanielEScherzer]

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

[SakiTakamachi]

We have decided to include this in version 8.4.7.

I plan to package RC2 with this change later today (Japan time), and will make an announcement once QA is complete.

If everything goes smoothly, I will release 8.4.7 as scheduled next week.

[DanielEScherzer]

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

[iluuu1994]

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

[DanielEScherzer]

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

Segfault is from the module number being 8 because of ZEND_ACC_DEPRECATED

[iluuu1994]

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.

[DanielEScherzer]

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)

[iluuu1994]

Ah ofc, makes sense.

[DanielEScherzer]

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

[iluuu1994]

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.

[remicollet]

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 ?

[iluuu1994]

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.

[DanielEScherzer]

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

[iluuu1994]

@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.

[iluuu1994]

@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]

@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]

@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!

[DanielEScherzer]

<rebased, should be ready for review for 8.4.8>

[NattyNarwhal]

This sounds fine to me.

[DanielEScherzer]

Missed out on 8.4.8 but I'll merge this as soon as CI passes

[DanielEScherzer]

Since I'm still getting used to merging things, would appreciate if someone can confirm I did this right

• cd751f9 for 8.4
• 04522cd for master

[ndossche]

Since I'm still getting used to merging things, would appreciate if someone can confirm I did this right

* [cd751f9](https://github.com/php/php-src/commit/cd751f98cb991bae0bdb8de7e9b6970a14e0664a) for 8.4
* [04522cd](https://github.com/php/php-src/commit/04522cd1c4e902fe8fe69e553564cc2bb2a94196) for master

It's right

[iluuu1994]

Master is failing, though.

[DanielEScherzer]

Master is failing, though.

#18666