Make vm_interrupt and timed_out atomic

[morrisonlevi]

On Windows these are already set from another thread and should be
atomic. Only one of these was previously atomic using
InterlockedExchange8. On Linux, it's unclear if to me if it should be
atomic based on existing code.

However, extensions which use vm interrupts are very likely to do the
same thing that Windows is doing: set a vm_interrupt from another
thread.

This adds a new zend_atomic_bool type. The type definition is only
available for compiler alignment and size info; it should be treated as
opaque and only the zend_atomic_bool_* family of functions should be
used. Extensions inspecting EG(vm_interrupt) and EG(timed_out) need
to be updated.

Note that directly using atomic_bool is complicated. All C++ stdlibs
that I checked typedef atomic_bool to std::atomic, which can't
be used in an extern "C" section, and there's at least one usage of
this in core, and probably more outside of it.

So, instead use platform specific functions, preferring compiler
intrinsics.

[morrisonlevi]

I'm trying with compiler intrinsics now. I suspect I may need to determine if I need to link with -latomic sometimes.

[cmb69]

Thank you! The Windows part looks good to me.

[nikic]

As I've told you many times before, please only use static inline in php-src, not inline or extern inline. I'm not having this discussion again.

[morrisonlevi]

@nikic I think given the past discussions, you may have misunderstood the point of inline + extern inline on these specific functions. So, here are the goals:

1. I want there to be ZEND_API functions here which are callable from places such as extensions written in Rust.
2. For performance reasons, we want these same functions to inline whenever possible within the engine itself, hence inline.

A quick grep (which means it may miss some cases with strange formatting, macros, or multiline definitions) show that within the engine today, this is novel: nothing is marked inline that is also ZEND_API.

I tested on a modified zend_atomic.{c,h} on Mac saw that static inline ZEND_API means that the functions with external linkage don't exist. You can verify this with a reduced example:

__attribute__((visibility("default")))
static inline int four(void) {
  return 4;
}

extern inline int four(void); // does nothing in this case

__attribute__((visibility("default")))
inline int five(void) {
  return 5;
}

extern inline int five(void); // without this, five won't have external linkage either

int main() { return four() == 4 && five() == 5 ? 0 : 1; }

I built with clang -fvisibility=hidden -O2 -g -Wall -Wextra inline.c -o inline and checked with nm -g inline. Here are the symbols:

0000000100000000 T __mh_execute_header
0000000100003fa0 T _five
                 U dyld_stub_binder

So, that's why I used non-static inline + extern inline. When discussing this a bit with bwoebi, he suggested I could alternatively have static zend_always_inline, non-ZEND_API functions the engine uses, then in the zend_atomic.c file have non-static, non-inline functions which delegate to the static inline ones. That didn't occur to me when writing the PR, but I prefer extern inline anyway because then there aren't two different names for each function and much less boilerplate overall. What do you think?

[morrisonlevi]

I've made the change to remove extern inline. Now there is zend_atomic_bool_* and zend_atomic_bool_*_ex where the former just delegates to the latter and the engine uses _ex everywhere. I believe this change actually makes this PR worse, however, I don't care to die on this hill today.

[ramsey]

Needs @nikic to review and unblock

[bwoebi]

LGTM

[morrisonlevi]

For whatever reason Nikita hasn't responded to messages here or in other places. I removed the extern inline that he complained about, so I'm going ahead with the merge.

[twose]

A feature I was developing just needed to use atomic, so I re-read this PR today, I found that there are still some things that can be improved, so I opened #8764.

And, to be honest, I do not like such a design that both have ZEND_API and inline functions.

I want there to be ZEND_API functions here which are callable from places such as extensions written in Rust.

IMO, providing atomic APIs is not Zend's job, if you want to support set EG(vm_interrupt) or EG(timed_out) in Rust, I recommend to add specified APIs like zend_vm_imterrupt() and zend_timed_out().

There are so many macros and inline functions in Zend that you can't require everything to be API...

Please correct me if I am wrong :)

[tstarling]

[deleted wrong comment]