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

[DanielEScherzer]

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

[DanielEScherzer]

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]

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]

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?

[TimWolla]

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

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

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]

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]

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

[TimWolla]

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]

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

[DanielEScherzer]

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

[DanielEScherzer]

Fixed conflict from #17306