[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?= <tim@tideways-gmbh.com>
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-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.

UPGRADING

Zend/zend_compile.h

iluuu1994

It seems this still misses support for opcache file cache. Tests fail with --file-cache-prime.

iluuu1994 · 2025-04-22T12:29:34Z

UPGRADING

@@ -130,6 +130,10 @@ PHP 8.5 UPGRADE NOTES
    RFC: https://wiki.php.net/rfc/marking_return_value_as_important
  . Added asymmetric visibility support for static properties.
    RFC: https://wiki.php.net/rfc/static-aviz
+  . Added support for attributes on compile-time non-class constants.


Suggested change

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

. Added support for attributes on global constants.

Global constants are not actually compile time in PHP.

They aren't created at compile time, but they are declared at compile time, unlike those with define(). https://www.php.net/manual/en/language.constants.syntax.php

As opposed to defining constants using define(), constants defined using the const keyword must be declared at the top-level scope because they are defined at compile-time.

Since the UPGRADING file keeps causing merge conflicts, I've removed my changes to that file for now, but the language I'm planning to include is:

. 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

Zend/zend_ast.c

Zend/zend_compile.c

iluuu1994 · 2025-04-22T13:09:12Z

Zend/zend_compile.c

+	if (list->children > 2) {
+		zend_error_noreturn(
+			E_COMPILE_ERROR,
+			"Cannot apply attributes to multiple constants at once"


Unfortunately, this is inconsistent with how attributes work on other lists. https://3v4l.org/01Nhv I prefer the error, but optimally they should be aligned.

This was pointed out before the RFC #16952 (comment) and I explained my reasoning at the time, it was voted on as part of the RFC

To prevent confusing code, attributes are only allowed to be applied to constants that are declared in their own statement:

with an example demonstrating this behavior. Personally I would prefer to deprecate having attributes applied to multiple properties/constants in other places

Zend/zend_compile.h

Zend/zend_vm_def.h

docs/source/core/data-structures/zend_constant.rst

DanielEScherzer · 2025-04-22T16:49:17Z

It seems this still misses support for opcache file cache. Tests fail with --file-cache-prime.

Can you document how to reproduce this? I ran in order

php run-tests.php --file-cache-prime Zend/tests/attributes/constants/
php run-tests.php --file-cache-use Zend/tests/attributes/constants/
php run-tests.php --file-cache-prime Zend/tests/attributes/deprecated/constants/
php run-tests.php --file-cache-use Zend/tests/attributes/deprecated/constants/

and they all passed

iluuu1994 · 2025-04-22T16:50:51Z

Can you document how to reproduce this? I ran in order

You need -d opcache.enable_cli=1 for this flag to be effective.

DanielEScherzer · 2025-04-22T16:52:48Z

Can you document how to reproduce this? I ran in order

You need -d opcache.enable_cli=1 for this flag to be effective.

I tried with that too and wasn't able to get failures, what is the exact command you were using?

iluuu1994 · 2025-04-22T16:56:01Z

Are you sure you have opcache enabled? It should error right away. E.g.

========DIFF========
001- array(1) {
002-   [0]=>
003-   object(ReflectionAttribute)#%d (1) {
004-     ["name"]=>
005-     string(11) "MyAttribute"
006-   }
007- }
001+ php: /home/ilutov/Developer/php-src/ext/opcache/zend_file_cache.c:435: zend_file_cache_serialize_zval: Assertion `zval_get_type(&(*(zv))) < 6' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Attributes with TARGET_ALL (from an explicit parameter) can target constants [Zend/tests/attributes/constants/target_all_targets_const-explicit.phpt]

DanielEScherzer · 2025-04-22T17:23:48Z

Are you sure you have opcache enabled? It should error right away. E.g.

========DIFF========
001- array(1) {
002-   [0]=>
003-   object(ReflectionAttribute)#%d (1) {
004-     ["name"]=>
005-     string(11) "MyAttribute"
006-   }
007- }
001+ php: /home/ilutov/Developer/php-src/ext/opcache/zend_file_cache.c:435: zend_file_cache_serialize_zval: Assertion `zval_get_type(&(*(zv))) < 6' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Attributes with TARGET_ALL (from an explicit parameter) can target constants [Zend/tests/attributes/constants/target_all_targets_const-explicit.phpt]

Thanks, figured it

Reproduced with php run-tests.php -d "zend_extension=opcache.so" -d opcache.enable_cli=1 --file-cache-prime Zend/tests/attributes/constants/

And in GDB: run -d "zend_extension=opcache.so" -d opcache.enable_cli=1 -d opcache.file_cache=/tmp /usr/src/php/Zend/tests/attributes/constants/target_all_targets_const-explicit.php

Should be fixed now

TimWolla · 2025-04-22T17:34:21Z

And in GDB:

Hint: You can easily spawn a single (failed) test in gdb by doing: bash Zend/tests/attributes/constants/target_all_targets_const-explicit.sh gdb (i.e. replace phpt by sh and append the gdb argument).

iluuu1994 · 2025-04-22T17:53:20Z

You're missing the equivalent in zend_file_cache_unserialize_zval(). It also doesn't seem correct, you would actually need to call SERIALIZE_ATTRIBUTES on that pointer. The code should look similar to ext/opcache/zend_persist.c (or ext/opcache/zend_persist_calc.c if there's no existing loop over the opcodes). Another execution of the tests with --file-cache-use should reveal those issues.

DanielEScherzer · 2025-04-22T18:06:01Z

You're missing the equivalent in zend_file_cache_unserialize_zval(). It also doesn't seem correct, you would actually need to call SERIALIZE_ATTRIBUTES on that pointer. The code should look similar to ext/opcache/zend_persist.c (or ext/opcache/zend_persist_calc.c if there's no existing loop over the opcodes). Another execution of the tests with --file-cache-use should reveal those issues.

Tested with --file-cache-use now too, thanks for bearing with me

iluuu1994 · 2025-04-22T18:22:51Z

No worries, this part of the code is not easy to understand. I would prefer if not all IS_PTR zvals would assumed to be attributes, like we do in zend_persist_op_array_ex(). If that's more difficult for some reason, this is fine too.

DanielEScherzer · 2025-04-22T18:55:09Z

No worries, this part of the code is not easy to understand. I would prefer if not all IS_PTR zvals would assumed to be attributes, like we do in zend_persist_op_array_ex(). If that's more difficult for some reason, this is fine too.

I had poked around in GDB and didn't see any way to tell that it was an attribute (or not) but given that so far there was nothing that would be IS_PTR it seems that at least for now the only thing that can be IS_PTR is attributes.

--
FREEBSD failure seems unrelated, and I don't understand the CircleCI since the tests are passing on the other checks and that code shouldn't have changed

iluuu1994 · 2025-04-22T20:25:02Z

I had poked around in GDB and didn't see any way to tell that it was an attribute (or not) but given that so far there was nothing that would be IS_PTR it seems that at least for now the only thing that can be IS_PTR is attributes.

We can't tell from the zval alone, see how zend_persist_op_array_ex() checks for the given opcode and handles it specially. It looks like we're already looping through opcodes in zend_file_cache_serialize_op_array(), so adding a similar check there shouldn't cost too much.

DanielEScherzer · 2025-04-22T21:07:19Z

I had poked around in GDB and didn't see any way to tell that it was an attribute (or not) but given that so far there was nothing that would be IS_PTR it seems that at least for now the only thing that can be IS_PTR is attributes.

We can't tell from the zval alone, see how zend_persist_op_array_ex() checks for the given opcode and handles it specially. It looks like we're already looping through opcodes in zend_file_cache_serialize_op_array(), so adding a similar check there shouldn't cost too much.

Except that here the values are stored in op_array->literals and that stores all of the literals, not just the attributes. For the target_all_targets_const-explicit.phpt test, we have op_array->last_literal being 12, with the literals (op_array->literals[0]):

[0] - the string EXAMPLE
[1] - the string FOO
[2] - the attributes pointer
[3] - the string ReflectionConstant
[4] - lowercase string reflectionconstant
[5] - the string getAttributes
[6] - lowercase string getattributes
[7] - the string var_dump
[8] - the lval 0
[9] - the string newInstance
[10] - lowercase string newinstance
[11] - the lval 1

So we would need to somehow add that information to the parsing of ->literals

Any objections to merging this as is? (I'll update UPGRADING, rebase, and squash)

iluuu1994 · 2025-04-22T21:23:36Z

Except that here the values are stored in op_array->literals and that stores all of the literals, not just the attributes.

The same goes for persist. See how it's done there. The zval persisting skips the IS_PTR, the pointer is then handled when looping over the oparray, because only then we know what the content of the pointer actually is.

DanielEScherzer · 2025-04-22T21:26:16Z

Except that here the values are stored in op_array->literals and that stores all of the literals, not just the attributes.

The same goes for persist. See how it's done there. The zval persisting skips the IS_PTR, the pointer is then handled when looping over the oparray, because only then we know what the content of the pointer actually is.

I assumed that the point of this was to ensure that we didn't just assume that all IS_PTR zvals were pointing to attributes, but even if we come back after looping through the oparray, how do we know that any given literal is associated with the declaration of attributes? It seems we would just move the assumption

Until the assumption is incorrect, is there any harm in leaving this as-is?

iluuu1994 · 2025-04-22T21:32:29Z

From ext/opcache/zend_persist.c:

if (opline->opcode == ZEND_OP_DATA && (opline-1)->opcode == ZEND_DECLARE_ATTRIBUTED_CONST) {
	zval *literal = RT_CONSTANT(opline, opline->op1);
	HashTable *attributes = Z_PTR_P(literal);
	attributes = zend_persist_attributes(attributes);
	ZVAL_PTR(literal, attributes);
}

This changes the assumption from "all IS_PTR literals are attributes" to "DECLARE_ATTRIBUTED_CONST OP_DATA literals are attributes". That's quite a different assumption.

Until the assumption is incorrect, is there any harm in leaving this as-is?

Then, the question is, why do we make the assumption only half the time (i.e. not persist and persist_calc)? If that's really the preferred way, then it should at least be the same everywhere.

DanielEScherzer · 2025-04-22T21:35:54Z

if (opline->opcode == ZEND_OP_DATA && (opline-1)->opcode == ZEND_DECLARE_ATTRIBUTED_CONST) {
	zval *literal = RT_CONSTANT(opline, opline->op1);
	HashTable *attributes = Z_PTR_P(literal);
	attributes = zend_persist_attributes(attributes);
	ZVAL_PTR(literal, attributes);
}

I didn't realize I could also access the literal from there, that makes more sense, will do

iluuu1994

I no longer have any complaints

iluuu1994 · 2025-04-28T22:20:40Z

Oh, just as i approved I saw the arm failure. This look related, no?

DanielEScherzer · 2025-04-29T02:17:29Z

I spent way too long to figure out that the issue was that in ext/opcache/jit/zend_jit_vm_helpers.c instead of calling CONST_UNPROTECT_RECURSION() to release the recursion I was just calling CONST_UNPROTECT_RECURSION() a second time - facepalm

DanielEScherzer · 2025-04-29T03:53:13Z

Tests now all pass
If there is no further review I'll merge this tomorrow

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

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 Outdated 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 reviewed Apr 9, 2025

View reviewed changes

UPGRADING Outdated Show resolved Hide resolved

Zend/zend_compile.h Outdated Show resolved Hide resolved

iluuu1994 reviewed Apr 22, 2025

View reviewed changes

iluuu1994 approved these changes Apr 28, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

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

84be69d

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

DanielEScherzer force-pushed the constant-attribs branch from d57801e to 84be69d Compare April 29, 2025 18:14

DanielEScherzer merged commit 3f03f7e into php:master Apr 29, 2025
10 checks passed

DanielEScherzer deleted the constant-attribs branch April 29, 2025 19:50

DanielEScherzer mentioned this pull request Apr 29, 2025

Recursion protection for deprecation constants not released on bailout #18463

Open

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

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

[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 Apr 9, 2025

iluuu1994 commented Apr 9, 2025

TimWolla left a comment

Choose a reason for hiding this comment

iluuu1994 left a comment

Choose a reason for hiding this comment

iluuu1994 Apr 22, 2025

Choose a reason for hiding this comment

DanielEScherzer Apr 22, 2025

Choose a reason for hiding this comment

DanielEScherzer Apr 22, 2025

Choose a reason for hiding this comment

iluuu1994 Apr 22, 2025

Choose a reason for hiding this comment

DanielEScherzer Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

DanielEScherzer commented Apr 22, 2025

iluuu1994 commented Apr 22, 2025

DanielEScherzer commented Apr 22, 2025

iluuu1994 commented Apr 22, 2025

DanielEScherzer commented Apr 22, 2025

TimWolla commented Apr 22, 2025

iluuu1994 commented Apr 22, 2025 • edited Loading

DanielEScherzer commented Apr 22, 2025

iluuu1994 commented Apr 22, 2025

DanielEScherzer commented Apr 22, 2025

iluuu1994 commented Apr 22, 2025

DanielEScherzer commented Apr 22, 2025 • edited Loading

iluuu1994 commented Apr 22, 2025

DanielEScherzer commented Apr 22, 2025

iluuu1994 commented Apr 22, 2025

DanielEScherzer commented Apr 22, 2025

iluuu1994 left a comment

Choose a reason for hiding this comment

iluuu1994 commented Apr 28, 2025

This comment was marked as outdated.

DanielEScherzer commented Apr 29, 2025

DanielEScherzer commented Apr 29, 2025

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

DanielEScherzer Apr 22, 2025 •

edited

Loading

iluuu1994 commented Apr 22, 2025 •

edited

Loading

DanielEScherzer commented Apr 22, 2025 •

edited

Loading