Zend: Add `zend_check_method_accessible()` to DRY method visibility checks #18995

TimWolla · 2025-07-01T10:01:49Z

To (partly) follow up on #18919 (comment) and #18919 (comment). Best reviewed with whitespace changes ignored.

iluuu1994 · 2025-07-01T10:20:44Z

Zend/zend_objects_API.h

+static inline bool zend_check_method_accessible(const zend_function *fn, const zend_class_entry *scope)
+{
+	if (
+		!(fn->common.fn_flags & ZEND_ACC_PUBLIC)


I'd prefer making this an assert, as many of the call-sites have already accounted for this. Some of them are quite hot too, and there's no guarantee the compiler will omit the repeated check.

Symfony instruction count went up by 0.01%. Seems to confirm this is not omitted, or the function isn't inlined.

Indeed. For zend_std_get_constructor it appears the flag is rechecked. I hate this, the point of the method was that I don't need to think about this 😒

Note though that the increase of 0.01% could also be noise. It's within the noise range that I often see on PRs. Looking at the assembly would confirm whether inlining happened or not.

Zend/zend_objects.c

Zend/zend_objects_API.h

iluuu1994 · 2025-07-01T10:42:29Z

I'm also not sure if the inline should be zend_always_inline. Might be worth checking the assembly whether the inlining happens.

nielsdos

Nice cleanup.

nielsdos · 2025-07-01T19:14:42Z

Zend/zend_objects_API.h

+	if (!(fn->common.fn_flags & ZEND_ACC_PUBLIC)
+		&& fn->common.scope != scope
+		&& (UNEXPECTED(fn->common.fn_flags & ZEND_ACC_PRIVATE)
+			|| UNEXPECTED(!zend_check_protected(zend_get_function_root_class(fn), scope)))) {


Typically the style is to indent subconditions with spaces. One space for every sublevel. I.e.

Suggested change

if (!(fn->common.fn_flags & ZEND_ACC_PUBLIC)

&& fn->common.scope != scope

&& (UNEXPECTED(fn->common.fn_flags & ZEND_ACC_PRIVATE)

|| UNEXPECTED(!zend_check_protected(zend_get_function_root_class(fn), scope)))) {

if (!(fn->common.fn_flags & ZEND_ACC_PUBLIC)

&& fn->common.scope != scope

&& (UNEXPECTED(fn->common.fn_flags & ZEND_ACC_PRIVATE)

|| UNEXPECTED(!zend_check_protected(zend_get_function_root_class(fn), scope)))) {

nielsdos · 2025-07-01T19:15:58Z

Zend/zend_vm_def.h

-				ZVAL_UNDEF(EX_VAR(opline->result.var));
-				HANDLE_EXCEPTION();
-			}
+		if (!zend_check_method_accessible(clone, scope)) {


If inlining is not guaranteed then the extra check on the public flag is not eliminated. always inlining might be better.

nielsdos · 2025-07-01T19:22:05Z

Zend/zend_objects_API.h

+static inline bool zend_check_method_accessible(const zend_function *fn, const zend_class_entry *scope)
+{
+	if (
+		!(fn->common.fn_flags & ZEND_ACC_PUBLIC)


Note though that the increase of 0.01% could also be noise. It's within the noise range that I often see on PRs. Looking at the assembly would confirm whether inlining happened or not.

…hecks

nielsdos · 2025-07-02T19:52:02Z

Zend/zend_object_handlers.c

-					zend_object_store_ctor_failed(zobj);
-					constructor = NULL;
-				}
+			if (!zend_check_method_accessible(constructor, scope)) {


To solve the fn_flags reload problem, just insert an ZEND_ASSERT/ZEND_ASSUME here, right before the call to zend_check_method_accessible (preferably an assertion).

Ah, good idea. That would keep zend_check_method_accessible() easy to use compared to asserting within zend_check_method_accessible() itself. I'll check this tomorrow.

Now done. The CI Benchmark still sees a 0.01% increase, which now might actually be noise then?

Can't check now. You didn't specify always_inline though. I also see 0.02 on symfony

Always inline didn't change anything for the non-JIT runs (and my local tests also indicated that the inlining happened).

You mentioned:

I hate this, the point of the method was that I don't need to think about this 😒

This doesn't seem to be better in that regard. An assert can at least remind you that you didn't check for ZEND_ACC_PUBLIC. Anyway, just my opinion, proceed as you like.

This doesn't seem to be better in that regard.

The difference is that the manual checking for public just exists to improve performance when the scope is not yet available. If a scope is already available, then zend_check_method_accessible() can just be used directly.

Ideally, #18998 would work and we wouldn't need the assert at all.

…_method_accessible()`

TimWolla requested review from iluuu1994 and nielsdos July 1, 2025 10:01

TimWolla requested a review from dstogov as a code owner July 1, 2025 10:01

github-actions bot added Category: Engine ABI break labels Jul 1, 2025

TimWolla force-pushed the dry-method-visibility-checks branch 2 times, most recently from 270af52 to 9c89298 Compare July 1, 2025 10:29

iluuu1994 reviewed Jul 1, 2025

View reviewed changes

TimWolla force-pushed the dry-method-visibility-checks branch from 9c89298 to 98dc4f0 Compare July 1, 2025 11:14

This was referenced Jul 1, 2025

zend_execute: Mark zend_get_executed_*() as __attribute__((pure)) #18998

Draft

Zend: Use zend_bad_method_call() when cloning from the wrong scope #18999

Merged

nielsdos reviewed Jul 1, 2025

View reviewed changes

Zend: Add zend_check_method_accessible() to DRY method visibility c…

db4b7ee

…hecks

TimWolla force-pushed the dry-method-visibility-checks branch from 98dc4f0 to db4b7ee Compare July 2, 2025 06:49

nielsdos reviewed Jul 2, 2025

View reviewed changes

TimWolla added 2 commits July 3, 2025 08:53

Zend: Add assertions verifying flags didn't change before `zend_check…

4de0715

…_method_accessible()`

Try zend_always_inline for zend_check_method_accessible

9bdd743

Zend: Add zend_check_method_accessible() to DRY method visibility checks #18995

Are you sure you want to change the base?

Zend: Add zend_check_method_accessible() to DRY method visibility checks #18995

Conversation

TimWolla commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iluuu1994 commented Jul 1, 2025

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsdos Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zend: Add `zend_check_method_accessible()` to DRY method visibility checks #18995

Zend: Add `zend_check_method_accessible()` to DRY method visibility checks #18995

TimWolla commented Jul 1, 2025 •

edited

Loading

nielsdos Jul 2, 2025 •

edited

Loading