Skip to content

Bump fn_flags size #19959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
+69 −65
Closed

Bump fn_flags size #19959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Zend/Optimizer/optimize_func_calls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ static void zend_delete_call_instructions(zend_op_array *op_array, zend_op *opli

static void zend_try_inline_call(zend_op_array *op_array, zend_op *fcall, zend_op *opline, zend_function *func)
{
const uint32_t no_discard = RETURN_VALUE_USED(opline) ? 0 : ZEND_ACC_NODISCARD;
const zend_fn_flags no_discard = RETURN_VALUE_USED(opline) ? 0 : ZEND_ACC_NODISCARD;

if (func->type == ZEND_USER_FUNCTION
&& !(func->op_array.fn_flags & (ZEND_ACC_ABSTRACT|ZEND_ACC_HAS_TYPE_HINTS|ZEND_ACC_DEPRECATED|no_discard))
Expand Down
2 changes: 1 addition & 1 deletion Zend/Optimizer/zend_optimizer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1726,7 +1726,7 @@ ZEND_API void zend_optimize_script(zend_script *script, zend_long optimization_l

ZEND_ASSERT(orig_op_array != NULL);
if (orig_op_array != op_array) {
uint32_t fn_flags = op_array->fn_flags;
zend_fn_flags fn_flags = op_array->fn_flags;
zend_function *prototype = op_array->prototype;
HashTable *ht = op_array->static_variables;

Expand Down
2 changes: 1 addition & 1 deletion Zend/zend_API.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ typedef struct _zend_function_entry {
zif_handler handler;
const struct _zend_internal_arg_info *arg_info;
uint32_t num_args;
uint32_t flags;
zend_fn_flags flags;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be easier to introduce yet another filed (e.g. flags2).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const zend_frameless_function_info *frameless_function_infos;
const char *doc_comment;
} zend_function_entry;
Expand Down
6 changes: 3 additions & 3 deletions Zend/zend_compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3916,7 +3916,7 @@ static uint32_t zend_compile_args(

ZEND_API uint8_t zend_get_call_op(const zend_op *init_op, zend_function *fbc, bool result_used) /* {{{ */
{
uint32_t no_discard = result_used ? 0 : ZEND_ACC_NODISCARD;
zend_fn_flags no_discard = result_used ? 0 : ZEND_ACC_NODISCARD;

if (fbc && init_op->opcode != ZEND_NEW) {
ZEND_ASSERT(!(fbc->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE));
Expand Down Expand Up @@ -7543,7 +7543,7 @@ static void zend_compile_attributes(
}
}

uint32_t flags = (CG(active_op_array)->fn_flags & ZEND_ACC_STRICT_TYPES)
zend_fn_flags flags = (CG(active_op_array)->fn_flags & ZEND_ACC_STRICT_TYPES)
? ZEND_ATTRIBUTE_STRICT_TYPES : 0;
attr = zend_add_attribute(
attributes, name, args ? args->children : 0, flags, offset, el->lineno);
Expand Down Expand Up @@ -8244,7 +8244,7 @@ static zend_string *zend_begin_method_decl(zend_op_array *op_array, zend_string
{
zend_class_entry *ce = CG(active_class_entry);
bool in_interface = (ce->ce_flags & ZEND_ACC_INTERFACE) != 0;
uint32_t fn_flags = op_array->fn_flags;
zend_fn_flags fn_flags = op_array->fn_flags;

zend_string *lcname;

Expand Down
12 changes: 7 additions & 5 deletions Zend/zend_compile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ typedef struct _zend_oparray_context {
/* Class cannot be serialized or unserialized | | | */
#define ZEND_ACC_NOT_SERIALIZABLE (1 << 29) /* X | | | */
/* | | | */
/* Function Flags (unused: 30) | | | */
/* Function Flags (unused: 30,32-63) | | | */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can just say

Suggested change
/* Function Flags (unused: 30,32-63) | | | */
/* Function Flags (unused: 30,32...) | | | */

like property flags above

/* ============== | | | */
/* | | | */
/* Function returning by reference | | | */
Expand Down Expand Up @@ -443,7 +443,7 @@ static zend_always_inline uint32_t zend_visibility_to_set_visibility(uint32_t vi
// Must not clash with ZEND_SHORT_CIRCUITING_CHAIN_MASK
#define ZEND_JMP_NULL_BP_VAR_IS 4

const char *zend_visibility_string(uint32_t fn_flags);
const char *zend_visibility_string(uint32_t flags);

#define ZEND_PROPERTY_HOOK_COUNT 2
#define ZEND_PROPERTY_HOOK_STRUCT_SIZE (sizeof(zend_function*) * ZEND_PROPERTY_HOOK_COUNT)
Expand Down Expand Up @@ -512,11 +512,13 @@ typedef struct _zend_internal_function_info {
const char *default_value;
} zend_internal_function_info;

typedef uint64_t zend_fn_flags;

struct _zend_op_array {
/* Common elements */
uint8_t type;
uint8_t arg_flags[3]; /* bitset of arg_info.pass_by_reference */
uint32_t fn_flags;
zend_fn_flags fn_flags;
zend_string *function_name;
zend_class_entry *scope;
zend_function *prototype;
Expand Down Expand Up @@ -575,7 +577,7 @@ typedef struct _zend_internal_function {
/* Common elements */
uint8_t type;
uint8_t arg_flags[3]; /* bitset of arg_info.pass_by_reference */
uint32_t fn_flags;
zend_fn_flags fn_flags;
zend_string* function_name;
zend_class_entry *scope;
zend_function *prototype;
Expand Down Expand Up @@ -604,7 +606,7 @@ union _zend_function {
struct {
uint8_t type; /* never used */
uint8_t arg_flags[3]; /* bitset of arg_info.pass_by_reference */
uint32_t fn_flags;
zend_fn_flags fn_flags;
zend_string *function_name;
zend_class_entry *scope;
zend_function *prototype;
Expand Down
2 changes: 1 addition & 1 deletion Zend/zend_enum.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ static void zend_enum_register_func(zend_class_entry *ce, zend_known_string_id n

void zend_enum_register_funcs(zend_class_entry *ce)
{
const uint32_t fn_flags =
const zend_fn_flags fn_flags =
ZEND_ACC_PUBLIC|ZEND_ACC_STATIC|ZEND_ACC_HAS_RETURN_TYPE|ZEND_ACC_ARENA_ALLOCATED;
zend_internal_function *cases_function = zend_arena_calloc(&CG(arena), sizeof(zend_internal_function), 1);
cases_function->handler = zend_enum_cases_func;
Expand Down
2 changes: 1 addition & 1 deletion Zend/zend_globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ struct _zend_compiler_globals {
/* and don't use them as lead/trail units */

zend_string *doc_comment;
uint32_t extra_fn_flags;
zend_fn_flags extra_fn_flags;

uint32_t compiler_options; /* set of ZEND_COMPILE_* constants */

Expand Down
24 changes: 12 additions & 12 deletions Zend/zend_inheritance.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,27 +200,27 @@ static void do_inherit_parent_constructor(zend_class_entry *ce) /* {{{ */
}
/* }}} */

const char *zend_visibility_string(uint32_t fn_flags) /* {{{ */
const char *zend_visibility_string(uint32_t flags) /* {{{ */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
if (fn_flags & ZEND_ACC_PUBLIC) {
if (flags & ZEND_ACC_PUBLIC) {
return "public";
} else if (fn_flags & ZEND_ACC_PRIVATE) {
} else if (flags & ZEND_ACC_PRIVATE) {
return "private";
} else {
ZEND_ASSERT(fn_flags & ZEND_ACC_PROTECTED);
ZEND_ASSERT(flags & ZEND_ACC_PROTECTED);
return "protected";
}
}
/* }}} */

static const char *zend_asymmetric_visibility_string(uint32_t fn_flags) /* {{{ */
static const char *zend_asymmetric_visibility_string(uint32_t flags) /* {{{ */
{
if (fn_flags & ZEND_ACC_PRIVATE_SET) {
if (flags & ZEND_ACC_PRIVATE_SET) {
return "private(set)";
} else if (fn_flags & ZEND_ACC_PROTECTED_SET) {
} else if (flags & ZEND_ACC_PROTECTED_SET) {
return "protected(set)";
} else {
ZEND_ASSERT(!(fn_flags & ZEND_ACC_PUBLIC_SET));
ZEND_ASSERT(!(flags & ZEND_ACC_PUBLIC_SET));
return "omitted";
}
}
Expand Down Expand Up @@ -1126,8 +1126,8 @@ static inheritance_status do_inheritance_check_on_method(
zend_function *parent, zend_class_entry *parent_scope,
zend_class_entry *ce, zval *child_zv, uint32_t flags) /* {{{ */
{
uint32_t child_flags;
uint32_t parent_flags = parent->common.fn_flags;
zend_fn_flags child_flags;
zend_fn_flags parent_flags = parent->common.fn_flags;
zend_function *proto;

#define SEPARATE_METHOD() do { \
Expand Down Expand Up @@ -1408,7 +1408,7 @@ static void inherit_property_hook(

child->common.prototype = parent->common.prototype ? parent->common.prototype : parent;

uint32_t parent_flags = parent->common.fn_flags;
zend_fn_flags parent_flags = parent->common.fn_flags;
if (parent_flags & ZEND_ACC_PRIVATE) {
child->common.fn_flags |= ZEND_ACC_CHANGED;
return;
Expand Down Expand Up @@ -2440,7 +2440,7 @@ static void zend_fixup_trait_method(zend_function *fn, zend_class_entry *ce) /*
}
/* }}} */

static void zend_traits_check_private_final_inheritance(uint32_t original_fn_flags, const zend_function *fn_copy, const zend_string *name)
static void zend_traits_check_private_final_inheritance(zend_fn_flags original_fn_flags, const zend_function *fn_copy, const zend_string *name)
{
/* If the function was originally already private+final, then it will have
* already been warned about. Only emit this error when the used trait method
Expand Down
4 changes: 2 additions & 2 deletions Zend/zend_vm_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4200,7 +4200,7 @@ ZEND_VM_HOT_HANDLER(131, ZEND_DO_FCALL_BY_NAME, ANY, ANY, SPEC(RETVAL,OBSERVER))
SAVE_OPLINE();
EX(call) = call->prev_execute_data;

const uint32_t no_discard = RETURN_VALUE_USED(opline) ? 0 : ZEND_ACC_NODISCARD;
const zend_fn_flags no_discard = RETURN_VALUE_USED(opline) ? 0 : ZEND_ACC_NODISCARD;

if (UNEXPECTED(fbc->common.fn_flags & (ZEND_ACC_DEPRECATED|no_discard))) {
if (fbc->common.fn_flags & ZEND_ACC_DEPRECATED) {
Expand Down Expand Up @@ -4311,7 +4311,7 @@ ZEND_VM_HOT_HANDLER(60, ZEND_DO_FCALL, ANY, ANY, SPEC(RETVAL,OBSERVER))
SAVE_OPLINE();
EX(call) = call->prev_execute_data;

const uint32_t no_discard = RETURN_VALUE_USED(opline) ? 0 : ZEND_ACC_NODISCARD;
const zend_fn_flags no_discard = RETURN_VALUE_USED(opline) ? 0 : ZEND_ACC_NODISCARD;

if (UNEXPECTED(fbc->common.fn_flags & (ZEND_ACC_DEPRECATED|no_discard))) {
if (fbc->common.fn_flags & ZEND_ACC_DEPRECATED) {
Expand Down
24 changes: 12 additions & 12 deletions Zend/zend_vm_execute.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion ext/opcache/ZendAccelerator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4359,7 +4359,7 @@ static void preload_fix_trait_op_array(zend_op_array *op_array)

zend_string *function_name = op_array->function_name;
zend_class_entry *scope = op_array->scope;
uint32_t fn_flags = op_array->fn_flags;
zend_fn_flags fn_flags = op_array->fn_flags;
zend_function *prototype = op_array->prototype;
HashTable *ht = op_array->static_variables;
*op_array = *orig_op_array;
Expand Down
16 changes: 8 additions & 8 deletions ext/opcache/jit/zend_jit_ir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4629,7 +4629,7 @@ static struct jit_observer_fcall_is_unobserved_data jit_observer_fcall_is_unobse
// JIT: if (function->common.fn_flags & (ZEND_ACC_CALL_VIA_TRAMPOLINE | ZEND_ACC_GENERATOR)) {
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)))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ir_CONST_U32(ZEND_ACC_CALL_VIA_TRAMPOLINE | ZEND_ACC_GENERATOR)));
ir_IF_TRUE(if_trampoline_or_generator);
ir_END_list(data.ir_end_inputs);
Expand Down Expand Up @@ -8440,7 +8440,7 @@ static int zend_jit_free_trampoline(zend_jit_ctx *jit, ir_ref func)
{
// JIT: if (UNEXPECTED(func->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE))
ir_ref if_trampoline = ir_IF(ir_AND_U32(
ir_LOAD_U32(ir_ADD_OFFSET(func, offsetof(zend_function, common.fn_flags))),
ir_TRUNC_U32(ir_LOAD_U64(ir_ADD_OFFSET(func, offsetof(zend_function, common.fn_flags)))),
ir_CONST_U32(ZEND_ACC_CALL_VIA_TRAMPOLINE)));

ir_IF_TRUE(if_trampoline);
Expand Down Expand Up @@ -8665,7 +8665,7 @@ static int zend_jit_push_call_frame(zend_jit_ctx *jit, const zend_op *opline, co
// (closure->func->common.fn_flags & ZEND_ACC_FAKE_CLOSURE);
call_info = ir_OR_U32(
ir_AND_U32(
ir_LOAD_U32(ir_ADD_OFFSET(func_ref, offsetof(zend_closure, func.common.fn_flags))),
ir_TRUNC_U32(ir_LOAD_U64(ir_ADD_OFFSET(func_ref, offsetof(zend_closure, func.common.fn_flags)))),
ir_CONST_U32(ZEND_ACC_FAKE_CLOSURE)),
ir_CONST_U32(ZEND_CALL_NESTED_FUNCTION | ZEND_CALL_DYNAMIC | ZEND_CALL_CLOSURE));
// JIT: if (Z_TYPE(closure->this_ptr) != IS_UNDEF) {
Expand Down Expand Up @@ -9117,7 +9117,7 @@ static int zend_jit_init_method_call(zend_jit_ctx *jit,
if (!func) {
// JIT: if (fbc->common.fn_flags & ZEND_ACC_STATIC) {
if_static = 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)))),
ir_CONST_U32(ZEND_ACC_STATIC)));
ir_IF_TRUE_cold(if_static);
}
Expand Down Expand Up @@ -9288,7 +9288,7 @@ static int zend_jit_init_static_method_call(zend_jit_ctx *jit,
if (!func) {
// JIT: if (fbc->common.fn_flags & ZEND_ACC_STATIC) {
if_static = 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)))),
ir_CONST_U32(ZEND_ACC_STATIC)));
ir_IF_FALSE_cold(if_static);
}
Expand Down Expand Up @@ -10104,7 +10104,7 @@ static int zend_jit_do_fcall(zend_jit_ctx *jit, const zend_op *opline, const zen
func_ref = ir_LOAD_A(jit_CALL(rx, func));
ir_GUARD_NOT(
ir_AND_U32(
ir_LOAD_U32(ir_ADD_OFFSET(func_ref, offsetof(zend_op_array, fn_flags))),
ir_TRUNC_U32(ir_LOAD_U64(ir_ADD_OFFSET(func_ref, offsetof(zend_op_array, fn_flags)))),
ir_CONST_U32(ZEND_ACC_DEPRECATED|ZEND_ACC_NODISCARD)),
ir_CONST_ADDR(exit_addr));
}
Expand Down Expand Up @@ -10136,7 +10136,7 @@ static int zend_jit_do_fcall(zend_jit_ctx *jit, const zend_op *opline, const zen
uint32_t no_discard = RETURN_VALUE_USED(opline) ? 0 : ZEND_ACC_NODISCARD;

if_deprecated_nodiscard = ir_IF(ir_AND_U32(
ir_LOAD_U32(ir_ADD_OFFSET(func_ref, offsetof(zend_op_array, fn_flags))),
ir_TRUNC_U32(ir_LOAD_U64(ir_ADD_OFFSET(func_ref, offsetof(zend_op_array, fn_flags)))),
ir_CONST_U32(ZEND_ACC_DEPRECATED|no_discard)));
ir_IF_TRUE_cold(if_deprecated_nodiscard);

Expand Down Expand Up @@ -10349,7 +10349,7 @@ static int zend_jit_do_fcall(zend_jit_ctx *jit, const zend_op *opline, const zen
if (!func) {
// JIT: if (EXPECTED((op_array->fn_flags & ZEND_ACC_HAS_TYPE_HINTS) == 0))
ir_ref if_has_type_hints = ir_IF(ir_AND_U32(
ir_LOAD_U32(ir_ADD_OFFSET(func_ref, offsetof(zend_op_array, fn_flags))),
ir_TRUNC_U32(ir_LOAD_U64(ir_ADD_OFFSET(func_ref, offsetof(zend_op_array, fn_flags)))),
ir_CONST_U32(ZEND_ACC_HAS_TYPE_HINTS)));
ir_IF_TRUE(if_has_type_hints);
ir_END_list(merge_inputs);
Expand Down
Loading