Add Internal observers

[bwoebi]

Adding observers for internal functions:

There are two main motivations to this:
a) The logic for handling internal and userland observation can be unified.
b) Unwinding of observed functions on a bailout does notably not include observers. Even if users of observers were to ensure such handling themselves, it would be impossible to retain the relative ordering - either the user has to unwind all internal observed frames before the automatic unwinding (zend_observer_fcall_end_all) or afterwards, but not properly interleaved.

This PR is stacked on top of #9062 (merged and rebased).

[krakjoe]

This looks okay to me, I wouldn't mind another pair of eyes with karma having a look ...

[arnaud-lb]

Looks good to me apart from the map_ptr issue

[arnaud-lb]

I think that we can not do this at runtime, otherwise the opcache will not see it and may allocate the same slots again in other processes. We can do this only during startup, or during an opcache hook.

Clashes may happen if these slots are imported into the current process during include:

dl("dl_test.so");
include "script.php";

An other unfortunate effect would be that the slot is used only temporarily, but will never be freed or reused again.

[bwoebi]

But ... zend_declare_class_constant_ex is also doing ZEND_MAP_PTR_NEW(ce->mutable_data) - code which is also invoked when a module declaring class constants is loaded via dl() at run-time. Or is that broken as well?

[arnaud-lb]

I think it's broken as well :D (or I'm wrong about ZEND_MAP_PTR_NEW).

Here ZEND_MAP_PTR_NEW() is only called for internal classes with AST constants (e.g. internal enums), which are probably rare, which would explain why we haven't seen the problem yet. Also I've just noticed that dl()ing an extension with internal enums crashes for an other reason.

[bwoebi]

Damn :-D I was looking at that code, saw it, and thought … okay, must be safe then.
However, how would one then fix this category of issues? I mean … doing a check for MODULE_TEMPORARY each time run_time_cache is to be accessed is obviously not viable for performance reasons.

[arnaud-lb]

We would only need to do this check before ZEND_MAP_PTR_NEW().

Apparently ZEND_MAP_PTR_GET / ZEND_MAP_PTR_SET can handle heap-allocated pointers. In this case the map ptr is a real pointer, not a CG(map_ptr_base) offset:

php-src/Zend/zend_map_ptr.h

Lines 49 to 52 in 9e2de4c

	# define ZEND_MAP_PTR_GET(ptr) \
	((ZEND_MAP_PTR_IS_OFFSET(ptr) ? \
	ZEND_MAP_PTR_GET_IMM(ptr) : \
	((void*)(ZEND_MAP_PTR(ptr)))))

Here we could do a ZEND_MAP_PTR_INIT(internal_function->run_time_cache, zend_arena_alloc(...)) instead of ZEND_MAP_PTR_NEW() + ZEND_MAP_PTR_SET() when EG(active).

[arnaud-lb]

An alternative would be to use a .reserved slot, or to introduce a new zend_internal_function field.

The run_time_cache / map_ptr mechanism is useful for user functions because they are immutable. We don't really need this for internal functions.

[bwoebi]

Applied the change, I guess looks good now?

[bwoebi]

Note that installed observers are different from request to request, so this needs run-time mutable data, hence the map_ptr mechanism. The big benefit of the map_ptr mechanism is that it's just an offset into some run-time memory, so that one can easily jump from the immutable (internal function) data structure to mutable run-time data. A .reserved slot or new fixed field on zend_internal_function does not suffice in itself for this purpose; they would also have to contain offsets into some per request to be allocated structure, essentially ending up being what map_ptrs currently are anyway providing.

Further, it perfectly aligns with the existing userland run_time_cache mechanism, reusing the same variables, saving extra conditions (avoiding having to check whether its user code or internal code), and having also the same access pattern for internal and userland run_time_caches does nothing but make the devs life a bit easier.

Additionally, at least while writing extension code, I also had the need to attach runtime information to internal functions. The workaround is doing a hashtable lookup with the function address, but obviously, this has a performance penalty.
The usefulness for core itself is probably a bit more limited, but the run-time overhead of initializing run_time cache is avoided if cache size is basically zero.

[arnaud-lb]

A .reserved slot or new fixed field on zend_internal_function does not suffice in itself for this purpose; they would also have to contain offsets into some per request to be allocated structure, essentially ending up being what map_ptrs currently are anyway providing.

I'm not sure about this. map_ptr is needed for structures that may live in SHM, because we don't store pointers to non-SHM in SHM. But zend_internal_functions leave on the heap and don't have that restriction. We can store a pointer to an array of observers in a zend_internal_function, and modify the array independently in each process or request.

I don't object though, with the ZEND_MAP_PTR_SET change this looks good to me 👍