Bump fn_flags size #19959
Bump fn_flags size #19959
Conversation
iluuu1994
requested review from
DanielEScherzer,
bukka and
dstogov
as code owners
iluuu1994
force-pushed
the
bump-fn_flags-size
branch
2 times, most recently
from
2f87075 to
6b3df5b
Compare
iluuu1994
force-pushed
the
bump-fn_flags-size
branch
from
6b3df5b to
6b417c8
Compare
| zend_function *mptr; | ||
| uint32_t keep_flags = ZEND_ACC_PPP_MASK | ||
| /* Keep in mind that all userland-exposed flags should fit in the low 32-bits | ||
| * for 32-bit architectures. Shuffle ZEND_ACC_* flags around accordingly. */ |
There was a problem hiding this comment.
we should add some kind of test or assertion for this. Maybe
ZEND_ASSERT(keep_flags < (1 << 31));
so that if we ever change the values of the flags things would instantly break?
| #define ZEND_ACC_NOT_SERIALIZABLE (1 << 29) /* X | | | */ | ||
| /* | | | */ | ||
| /* Function Flags (unused: 30) | | | */ | ||
| /* Function Flags (unused: 30,32-63) | | | */ |
There was a problem hiding this comment.
can just say
| /* Function Flags (unused: 30,32-63) | | | */ | |
| /* Function Flags (unused: 30,32...) | | | */ |
like property flags above
| /* }}} */ | ||
|
|
||
| const char *zend_visibility_string(uint32_t fn_flags) /* {{{ */ | ||
| const char *zend_visibility_string(uint32_t flags) /* {{{ */ |
There was a problem hiding this comment.
why is this not updated to use zend_fn_flags? Given that visibility is used for methods, we no longer guarantee that the flags fit into 32 bits. Maybe add an assertion at the top?
ZEND_ASSERT((ZEND_ACC_PUBLIC|ZEND_ACC_PRIVATE|ZEND_ACC_PROTECTED) < (1 << 31));
There was a problem hiding this comment.
This is used for non-functions (e.g. class constants). For functions, we'll simply drop the high-bits when calling the function. The assertion makes sense, though it can probably be a static assert.
| ZEND_ASSERT(rx != IR_UNUSED); | ||
| ir_ref if_trampoline_or_generator = ir_IF(ir_AND_U32( | ||
| ir_LOAD_U32(ir_ADD_OFFSET(func_ref, offsetof(zend_function, common.fn_flags))), | ||
| ir_TRUNC_U32(ir_LOAD_U64(ir_ADD_OFFSET(func_ref, offsetof(zend_function, common.fn_flags)))), |
There was a problem hiding this comment.
I don't understand the JIT enough to be confident in this comment, but it seems that this makes it so that only 32 of the bits are usable here? Or that at least ZEND_ACC_CALL_VIA_TRAMPOLINE and ZEND_ACC_GENERATOR need to be in the bottom 32 bits?
Same with some of the other flags down below
There was a problem hiding this comment.
They need to be in the bottom bits, yes. I haven't tested it, but IR should be able to optimize this back to a ir_LOAD_U32() on little-endian platforms. The aim was to change as few things as possible.
Or that at least ZEND_ACC_CALL_VIA_TRAMPOLINE and ZEND_ACC_GENERATOR need to be in the bottom 32 bits?
They are (nothing uses the high bits yet), but we can also add an assert.
| const struct _zend_internal_arg_info *arg_info; | ||
| uint32_t num_args; | ||
| uint32_t flags; | ||
| zend_fn_flags flags; |
There was a problem hiding this comment.
I think it would be easier to introduce yet another filed (e.g. flags2).
There was a problem hiding this comment.
It would be easier indeed, but also a bit more confusing (i.e. having to remember which field is stored in which flag). This can be partially mitigated by naming the flags accordingly (e.g. ZEND_ACC2_*). But ok, Niels has suggested the same thing, so let me implement that instead.
Alternative of phpGH-19959.
Alternative to phpGH-19959.
Alternative to phpGH-19959.
3 participants
Fixes php/php-tasks#35
ce->flagswill follow in a separate PR.