[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

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

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

iluuu1994 · 2025-02-04T14:59:54Z

Zend/tests/attributes/constants/ast_export.phpt

+#[MyAttrib(5, param: "example")]
+const WITH_PARAMETERS = true;
+
+echo zend_test_compile_to_ast( file_get_contents( __FILE__ ) );


Why not just use the same assert(false && function () { ... }); approach we use everywhere?

Because global constants cannot go in functions and so I couldn't find a way to test the ast export with that approach

Ok, but doesn't that mean that constants never appear here? And shouldn't that just mean that we don't need to implement support for this in the first place? A simple assert may do.

I don't really understand what you mean by

A simple assert may do.

I figured that if

since AST pretty printing exists for constant declarations, it should be accurate for constants with attributes

since there isn't an existing way to test that pretty printing, I should add one

We could just remove the logic for ZEND_AST_CONST_DECL from ast pretty printing entirely, but personally I find it helpful while debugging with gdb to be able to examine the ast

since AST pretty printing exists for constant declarations, it should be accurate for constants with attributes

Other declarations can actually occur in assert() though, which is the only reason they are supported.

https://3v4l.org/6Z9jg

I don't think it's a good idea to write code for a case that can't actually execute, at least if it isn't trivial, and then also requires a special testing mechanism.

but personally I find it helpful while debugging with gdb to be able to examine the ast

Note that we've recently added AST dumping from GDB, which will be more accurate and detailed.

php-src/scripts/gdb/php_gdb.py

Line 158 in dc7161c

class ZendAstPrettyPrinter(gdb.printing.PrettyPrinter):

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

iluuu1994 · 2025-02-04T17:36:15Z

ext/zend_test/tests/compile_to_ast/enum.phpt

+    }
+}
+
+echo zend_test_compile_to_ast( file_get_contents( __FILE__ ) );


The tests in ext/zend_test/tests/compile_to_ast don't seem related to this PR. Does this actually test something we don't have yet? See Zend/tests/enum/ast-dumper.phpt.

They are not strictly related to the attributes on constants feature, but are included because I introduce zend_test_compile_to_ast() for use in testing the attributes on constants, and figured we should also include some tests for that function directly.

iluuu1994 · 2025-02-04T17:51:45Z

Zend/zend_constants.c

+	zend_file_context original_file_context;
+	zend_file_context_begin(&original_file_context);
+
+	zend_compile_attributes(&c->attributes, ast, 0, ZEND_ATTRIBUTE_TARGET_CONST, 0);


I don't think you should call this function at runtime. It's likely possible to call it at compile time and pass an IS_PTR to the attribute array to op_data of ZEND_DECLARE_ATTRIBUTED_CONST instead.

So I spent some time trying to do this again, but couldn't get it to work with a constant deprecated with itself as the message. I earlier posted that

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

would it be okay to move the call to compile time as part of a follow-up, and get the main functionality merged?

I can have a look tomorrow. The benefit of handling it in the same PR is that it makes it obvious which changes are no longer needed. But if it turns out to be too difficult we can delay.

The infinite recursion problem is not unique to constants, it also occurs with class constants: https://3v4l.org/DibeG#v8.4.3 It only wasn't exposed because the tests use self::C where the constant is comp-time, and thus it is inlined, avoiding runtime access and thus infinite recursion. /cc @TimWolla

Zend/zend_compile.c

Avoid spaces in `()` for function definitions and calls, rename tests for attributes being repeatable, drop tokenization tests

To show why the ZEND_AST_NAMED_ARG handling is needed

The ast contained in ast_copy_ref is already destroyed by zend_const_expr_to_zval(). Avoid the double free by just freeing the ast ref itself.

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

We might require similar changes in more places.

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

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

6f5e5cc

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

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

DanielEScherzer and others added 6 commits February 4, 2025 12:21

Tests cleanup

9733df2

Avoid spaces in `()` for function definitions and calls, rename tests for attributes being repeatable, drop tokenization tests

Add and use zend_ast_export_list_ex()

b0a63f1

Better test for named parameters

4058c71

To show why the ZEND_AST_NAMED_ARG handling is needed

Fix use-after-free

ffb75c5

The ast contained in ast_copy_ref is already destroyed by zend_const_expr_to_zval(). Avoid the double free by just freeing the ast ref itself.

Compile constant attributes at compile time

1d22adc

Revert some more unnecessary changes

882e1b5

iluuu1994 mentioned this pull request Feb 5, 2025

Fix infinite recursion on deprecated attribute evaluation #17712

Open

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

iluuu1994 and others added 2 commits February 6, 2025 12:12

Fix alignemnt issue with function JIT

fd43d58

We might require similar changes in more places.

Tweaks and improvements

a7e9a57

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

iluuu1994 Feb 4, 2025

DanielEScherzer Feb 4, 2025

iluuu1994 Feb 4, 2025

DanielEScherzer Feb 4, 2025

iluuu1994 Feb 5, 2025 •

edited

Loading

iluuu1994 Feb 4, 2025

DanielEScherzer Feb 4, 2025

iluuu1994 Feb 4, 2025

DanielEScherzer Feb 4, 2025

iluuu1994 Feb 4, 2025

iluuu1994 Feb 5, 2025

DanielEScherzer left a comment

iluuu1994 commented Feb 6, 2025 •

edited

Loading

[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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iluuu1994 Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 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 Feb 5, 2025 •

edited

Loading

iluuu1994 commented Feb 6, 2025 •

edited

Loading