[RFC] Add support for attributes on compile-time constants #16952

DanielEScherzer · 2024-11-26T20:14:35Z

https://wiki.php.net/rfc/attributes-on-constants

DanielEScherzer · 2024-11-26T20:15:55Z

There are going to be some memory issues that CI will report, I couldn't figure those out, asked for help on the mailing list, https://news-web.php.net/php.internals/126065

DanielEScherzer · 2024-11-27T01:03:52Z

So there are also opcache failures, not just the failures I had locally - I guess a data op isn't the right way to send a pointer to the attributes from the compilation to the runtime - I'll see if I can have it send the raw AST and delay the attribute compilation until runtime

DanielEScherzer · 2024-11-27T05:26:55Z

So for the life of me, I can't figure out why I'm unable to get the cleanup in free_zend_constant() to run properly - I added debugging code

diff --git a/Zend/zend_constants.c b/Zend/zend_constants.c
index 8b92650816..ade2efb618 100644
--- a/Zend/zend_constants.c
+++ b/Zend/zend_constants.c
@@ -41,6 +41,8 @@ void free_zend_constant(zval *zv)
 {
        zend_constant *c = Z_PTR_P(zv);
 
+       fprintf(stderr, "Freeing constant %s\n", ZSTR_VAL(c->name));
+
        if (!(ZEND_CONSTANT_FLAGS(c) & CONST_PERSISTENT)) {
                zval_ptr_dtor_nogc(&c->value);
                if (c->name) {
@@ -50,7 +52,7 @@ void free_zend_constant(zval *zv)
                        zend_string_release_ex(c->filename, 0);
                }

and none of the compile-time constants in my test script are listed as being freed, though plenty of php-provided ones are. If the cleanup is never reached, then it makes sense that the attributes are not properly freed, but why isn't the cleanup reached?

Zend/zend_compile.c

TimWolla · 2024-11-27T16:33:40Z

@DanielEScherzer

From f411ddf4c984ed8d614d003e9eb209387b904f9b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= <[email protected]>
Date: Wed, 27 Nov 2024 17:32:48 +0100
Subject: [PATCH] Fix constant attribute leak

---
 Zend/zend_execute_API.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Zend/zend_execute_API.c b/Zend/zend_execute_API.c
index 9ebc15f3a4..f4adeb855f 100644
--- a/Zend/zend_execute_API.c
+++ b/Zend/zend_execute_API.c
@@ -302,6 +302,9 @@ ZEND_API void zend_shutdown_executor_values(bool fast_shutdown)
                                if (c->filename) {
                                        zend_string_release_ex(c->filename, 0);
                                }
+                               if (c->attributes) {
+                                       zend_hash_release(c->attributes);
+                               }
                                efree(c);
                                zend_string_release_ex(key, 0);
                        } ZEND_HASH_MAP_FOREACH_END_DEL();
-- 
2.43.0

This appears to fix the leak for me. I did not verify if it is possible to call free_zend_constant in that location. If it is, that should probably be a separate PR.

DanielEScherzer · 2024-11-27T19:36:38Z

@DanielEScherzer

[patch omitted for brevity]

This appears to fix the leak for me. I did not verify if it is possible to call free_zend_constant in that location. If it is, that should probably be a separate PR.

Thanks - I forgot there was a second place that constants are freed, though I should have remembered it from adding ReflectionConstant::getFileName()

DanielEScherzer · 2024-11-27T20:39:01Z

So on circleci the deprecation messages don't seem to be properly found, and on windows there are failures with exit code 1073741819, but only for the Reflection tests

TimWolla · 2024-11-29T11:30:51Z

So on circleci the deprecation messages don't seem to be properly found

@DanielEScherzer This is not related to CircleCI / ARM, but to JIT. You should be able to reproduce the issue locally with:

sapi/cli/php run-tests.php --asan --show-diff -P -q -d zend_extension=$(pwd)/modules/opcache.so -d opcache.enable_cli=1 -d opcache.jit=tracing -d opcache.protect_memory=1 --repeat 2 Zend/tests/attributes/deprecated/constants/const_messages.phpt

see: #11293 (comment)

TimWolla · 2024-11-29T11:34:12Z

I have taken the liberty to push a fix into your PR.

Zend/tests/attributes/deprecated/constants/const_messages.phpt

TimWolla · 2024-11-29T15:38:54Z

I can reproduce issues for the 3 tests that fail on Windows by using a non-debug ZTS build and running with Valgrind.

Specifically when I configure as follows:

./configure --enable-zend-test --enable-option-checking=fatal --enable-phpdbg --enable-fpm --enable-werror --enable-zts

And run the tests as follows:

sapi/cli/php run-tests.php -m --show-diff Zend/tests/attributes/001_placement.phpt ext/reflection/tests/ReflectionConstant_getAttributes.phpt ext/reflection/tests/ReflectionConstant_isDeprecated_userland.phpt

Running bash Zend/tests/attributes/001_placement.sh valgrind after the failed test then reveals:

==810251== Conditional jump or move depends on uninitialised value(s)
==810251==    at 0x6A1A9A: zend_resolve_class_name (zend_compile.c:1175)
==810251==    by 0x6A5A44: zend_resolve_class_name_ast (zend_compile.c:1209)
==810251==    by 0x6A5A44: zend_compile_attributes (zend_compile.c:7374)
==810251==    by 0x6B8D31: zend_constant_add_attributes (zend_constants.c:551)
==810251==    by 0x6DA516: ZEND_DECLARE_ATTRIBUTED_CONST_SPEC_CONST_CONST_HANDLER (zend_vm_execute.h:8031)
==810251==    by 0x713424: execute_ex (zend_vm_execute.h:59663)
==810251==    by 0x71DDD8: zend_execute (zend_vm_execute.h:64291)
==810251==    by 0x77FC0B: zend_execute_script (zend.c:1934)
==810251==    by 0x611626: php_execute_script_ex (main.c:2577)
==810251==    by 0x7819E2: do_cli (php_cli.c:938)
==810251==    by 0x3538B2: main (php_cli.c:1313)
==810251== 
==810251== Conditional jump or move depends on uninitialised value(s)
==810251==    at 0x69DF30: zend_prefix_with_ns (zend_compile.c:1062)
==810251==    by 0x6A5A44: zend_resolve_class_name_ast (zend_compile.c:1209)
==810251==    by 0x6A5A44: zend_compile_attributes (zend_compile.c:7374)
==810251==    by 0x6B8D31: zend_constant_add_attributes (zend_constants.c:551)
==810251==    by 0x6DA516: ZEND_DECLARE_ATTRIBUTED_CONST_SPEC_CONST_CONST_HANDLER (zend_vm_execute.h:8031)
==810251==    by 0x713424: execute_ex (zend_vm_execute.h:59663)
==810251==    by 0x71DDD8: zend_execute (zend_vm_execute.h:64291)
==810251==    by 0x77FC0B: zend_execute_script (zend.c:1934)
==810251==    by 0x611626: php_execute_script_ex (main.c:2577)
==810251==    by 0x7819E2: do_cli (php_cli.c:938)
==810251==    by 0x3538B2: main (php_cli.c:1313)

DanielEScherzer · 2024-11-30T17:06:55Z

RFC filed: https://wiki.php.net/rfc/attributes-on-constants
Discussion thread: https://news-web.php.net/php.internals/126084

DanielEScherzer · 2025-01-04T17:11:09Z

Rebased to update for #17101 fix, then squashed and added UPGRADING

DanielEScherzer · 2025-01-06T07:39:25Z

Fixed conflict from #17306

DanielEScherzer · 2025-02-03T07:19:35Z

Rebased to fix conflict from #17056 - @iluuu1994 do you think you might have a chance to review this soon? I see that @TimWolla requested that you take a look

iluuu1994 · 2025-02-04T00:25:39Z

Yes, I'll review tomorrow.

iluuu1994

You should try running with ASAN, it will reveal some failures in both Zend/tests/attributes/constants and Zend/tests/attributes/deprecated related to shutdown/cleanup of attributes.

Zend/tests/attributes/034_target_values.phpt

Zend/tests/attributes/constants/ast_export.phpt

Zend/tests/attributes/constants/tokenization.phpt

Zend/tests/attributes/constants/yes_repeatable-internal.phpt

Zend/tests/attributes/constants/allow_named_parameters.phpt

Zend/zend_attributes.c

ext/zend_test/tests/compile_to_ast/enum.phpt

Zend/zend_constants.c

Zend/zend_compile.c

DanielEScherzer

Thanks for figuring out that the infinite recursion wasn't caused by me, and about how to pass a pointer to the runtime - once the fix for #17711 is merged I'll rebase this patch and address any remaining feedback

Zend/tests/attributes/constants/named_parameter_validation.phpt

Zend/tests/attributes/deprecated/constants/error_code.phpt

Zend/zend_vm_def.h

iluuu1994 · 2025-02-06T10:57:43Z

This is also missing function JIT support... zend_jit_fetch_constant() needs to check for deprecations. ASan also complains for various tests in Zend/tests/attributes/constants with -d opcache.jit=function.

/home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:8012:2: runtime error: member access within misaligned address 0x00004294ca0f for type 'const struct zval', which requires 8 byte alignment
0x00004294ca0f: note: pointer points here
00 d2 01 01 00  79 04 89 01 00 00 00 00  00 fe ff ff ff ff ff ff  ff ff ff ff 00 00 00 00  07 00 00

https://wiki.php.net/rfc/attributes-on-constants

DanielEScherzer · 2025-04-09T19:32:30Z

I've rebased and squashed this patch. I plan to merge in around a week if there are no further review comments

iluuu1994 · 2025-04-09T19:33:35Z

@DanielEScherzer Please wait, I haven't gotten around the final review. Sorry for the delay.

TimWolla

Some comments. Did not have an in-depth look at this.

TimWolla · 2025-04-09T20:33:51Z

UPGRADING

+  . Added support for attributes on compile-time non-class constants.
+  . Added constant Attribute::TARGET_CONSTANT.
+  . The #[\Deprecated] attribute can now be used on constants.
+    RFC: https://wiki.php.net/rfc/attributes-on-constants


Suggested change

. Added support for attributes on compile-time non-class constants.

. Added constant Attribute::TARGET_CONSTANT.

. The #[\Deprecated] attribute can now be used on constants.

RFC: https://wiki.php.net/rfc/attributes-on-constants

. Added support for attributes on compile-time non-class constants.

RFC: https://wiki.php.net/rfc/attributes-on-constants

. The #[\Deprecated] attribute can now be used on constants.

RFC: https://wiki.php.net/rfc/attributes-on-constants

The Attribute::TARGET_CONSTANT is implied and the RFC should be linked for every change, since it's not always clear what belongs together.

TimWolla · 2025-04-09T20:35:15Z

Zend/zend_compile.h

+/* Has IS_PTR operands that needs special cleaning        |     |     |     */
+#define ZEND_ACC_PTR_OPS                 (1 << 28) /*     |  X  |     |     */
+/*                                                        |     |     |     */


Does this conflict with ZEND_ACC_OVERRIDE / is this placed in the correct section?

It is applied to function flags, so it is in the correct section, see usage in zend_compile_const_decl()

It shouldn't conflict because constants are only valid at the top level, where CG(active_op_array) isn't a real function, and definitely isn't a class method

DanielEScherzer requested review from kocsismate and dstogov as code owners November 26, 2024 20:14

github-actions bot added Category: Engine Extension: reflection SAPI: phpdbg ABI break labels Nov 26, 2024

TimWolla reviewed Nov 27, 2024

View reviewed changes

Zend/zend_compile.c Show resolved Hide resolved

github-actions bot added the Extension: opcache label Nov 29, 2024

TimWolla reviewed Nov 29, 2024

View reviewed changes

Zend/tests/attributes/deprecated/constants/const_messages.phpt Outdated Show resolved Hide resolved

github-actions bot added the Extension: zend_test label Dec 5, 2024

DanielEScherzer mentioned this pull request Dec 9, 2024

AST->string does not reproduce constructor property promotion correctly #17101

Closed

TimWolla added the RFC label Jan 3, 2025

DanielEScherzer force-pushed the constant-attribs branch from e8f731b to 50f88e5 Compare January 4, 2025 17:10

DanielEScherzer force-pushed the constant-attribs branch from 50f88e5 to 819ed7e Compare January 4, 2025 17:55

DanielEScherzer changed the title ~~Add support for attributes on compile-time constants~~ [RFC] Add support for attributes on compile-time constants Jan 4, 2025

DanielEScherzer force-pushed the constant-attribs branch from 819ed7e to 5b65cd9 Compare January 6, 2025 07:38

TimWolla requested a review from iluuu1994 January 6, 2025 08:04

TimWolla removed the request for review from iluuu1994 January 24, 2025 11:35

TimWolla requested a review from iluuu1994 January 24, 2025 11:35

azjezz mentioned this pull request Feb 1, 2025

feat: support attributes on constants for PHP 8.5 carthage-software/mago#64

Merged

7 tasks

DanielEScherzer force-pushed the constant-attribs branch from 5b65cd9 to 6f5e5cc Compare February 3, 2025 07:19

iluuu1994 reviewed Feb 4, 2025

View reviewed changes

iluuu1994 mentioned this pull request Feb 5, 2025

Fix infinite recursion on deprecated attribute evaluation #17712

Closed

DanielEScherzer commented Feb 5, 2025

View reviewed changes

Zend/tests/attributes/constants/named_parameter_validation.phpt Outdated Show resolved Hide resolved

Zend/tests/attributes/deprecated/constants/error_code.phpt Outdated Show resolved Hide resolved

Zend/zend_vm_def.h Show resolved Hide resolved

DanielEScherzer force-pushed the constant-attribs branch from a7e9a57 to 9a97441 Compare March 26, 2025 22:54

DanielEScherzer requested a review from iluuu1994 March 27, 2025 19:54

[RFC] Add support for attributes on compile-time constants

59b778a

https://wiki.php.net/rfc/attributes-on-constants

DanielEScherzer force-pushed the constant-attribs branch from 9a97441 to 59b778a Compare April 9, 2025 19:04

TimWolla reviewed Apr 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Add support for attributes on compile-time constants #16952

[RFC] Add support for attributes on compile-time constants #16952

DanielEScherzer commented Nov 26, 2024 •

edited

Loading

DanielEScherzer commented Nov 26, 2024 •

edited

Loading

DanielEScherzer commented Nov 27, 2024

DanielEScherzer commented Nov 27, 2024

TimWolla commented Nov 27, 2024 •

edited

Loading

DanielEScherzer commented Nov 27, 2024

DanielEScherzer commented Nov 27, 2024

TimWolla commented Nov 29, 2024 •

edited

Loading

TimWolla commented Nov 29, 2024

TimWolla commented Nov 29, 2024

DanielEScherzer commented Nov 30, 2024

DanielEScherzer commented Jan 4, 2025 •

edited

Loading

DanielEScherzer commented Jan 6, 2025

DanielEScherzer commented Feb 3, 2025

iluuu1994 commented Feb 4, 2025

iluuu1994 left a comment •

edited

Loading

DanielEScherzer left a comment

iluuu1994 commented Feb 6, 2025 •

edited

Loading

DanielEScherzer commented Apr 9, 2025

iluuu1994 commented Apr 9, 2025

TimWolla left a comment

TimWolla Apr 9, 2025

TimWolla Apr 9, 2025

DanielEScherzer Apr 9, 2025

[RFC] Add support for attributes on compile-time constants #16952

Are you sure you want to change the base?

[RFC] Add support for attributes on compile-time constants #16952

Conversation

DanielEScherzer commented Nov 26, 2024 • edited Loading

DanielEScherzer commented Nov 26, 2024 • edited Loading

DanielEScherzer commented Nov 27, 2024

DanielEScherzer commented Nov 27, 2024

TimWolla commented Nov 27, 2024 • edited Loading

DanielEScherzer commented Nov 27, 2024

DanielEScherzer commented Nov 27, 2024

TimWolla commented Nov 29, 2024 • edited Loading

TimWolla commented Nov 29, 2024

TimWolla commented Nov 29, 2024

DanielEScherzer commented Nov 30, 2024

DanielEScherzer commented Jan 4, 2025 • edited Loading

DanielEScherzer commented Jan 6, 2025

DanielEScherzer commented Feb 3, 2025

iluuu1994 commented Feb 4, 2025

iluuu1994 left a comment • edited Loading

Choose a reason for hiding this comment

DanielEScherzer left a comment

Choose a reason for hiding this comment

iluuu1994 commented Feb 6, 2025 • edited Loading

DanielEScherzer commented Apr 9, 2025

iluuu1994 commented Apr 9, 2025

TimWolla left a comment

Choose a reason for hiding this comment

TimWolla Apr 9, 2025

Choose a reason for hiding this comment

TimWolla Apr 9, 2025

Choose a reason for hiding this comment

DanielEScherzer Apr 9, 2025

Choose a reason for hiding this comment

DanielEScherzer commented Nov 26, 2024 •

edited

Loading

DanielEScherzer commented Nov 26, 2024 •

edited

Loading

TimWolla commented Nov 27, 2024 •

edited

Loading

TimWolla commented Nov 29, 2024 •

edited

Loading

DanielEScherzer commented Jan 4, 2025 •

edited

Loading

iluuu1994 left a comment •

edited

Loading

iluuu1994 commented Feb 6, 2025 •

edited

Loading