zend_portability: Add ZEND_CONTAINER_OF()

[TimWolla]

Changes made with Coccinelle:

@@
type T_container;
identifier member;
expression e;
@@

- (T_container *)(((char *)(e)) - offsetof(T_container, member))
+ ZEND_CONTAINER_OF(e, T_container, member)

@@
type T_container;
identifier member;
expression e;
typedef uintptr_t;
@@

- (T_container *)(((uintptr_t)(e)) - offsetof(T_container, member))
+ ZEND_CONTAINER_OF(e, T_container, member)

@@
type T_container;
identifier member;
expression e;
@@

- (const T_container *)(((char *)(e)) - offsetof(T_container, member))
+ ZEND_CONTAINER_OF(e, T_container, member)

@@
type T_container;
identifier member;
expression e;
typedef uintptr_t;
@@

- (const T_container *)(((uintptr_t)(e)) - offsetof(T_container, member))
+ ZEND_CONTAINER_OF(e, T_container, member)

[ndossche]

I think this relies on UB (pointer arithmetic on NULL)

[TimWolla]

pointer arithmetic on NULL

That branch is never evaluated; it's also the “accepted” solution to define container_of() in a type-safe way without statement expressions. So of all possible UB, this seems to be reasonably safe.

[ndossche]

pointer arithmetic on NULL

That branch is never evaluated

This doesn't matter for UB.

it's also the “accepted” solution to define container_of() in a type-safe way without statement expressions.

The Linux kernel is compiled with various flags that makes various UB defined, so I'm not so sure this is a valid comparison.

[iluuu1994]

I believe it's not UB unless executed, but we can probably achieve something similar with static assets?

[Girgias]

I believe it's not UB unless executed, but we can probably achieve something similar with static assets?

The issue is that compilers can assume UB never happens, and then start removing stuff or doing whatever they want if that code is present (see famous blog article about UB: https://devblogs.microsoft.com/oldnewthing/20140627-00/?p=633)

[iluuu1994]

So, I have a different understanding.

and then start removing stuff

Yes, but this branch is already provably dead. UB allows the compiler to make some assumptions. For example:

• If some branch contains UB, it's not reachable and can thus be removed. You may wonder why you're even allowed to write such code, but it's pretty common for dead branches to be generated from macros. And probably even more so with C++ templates.
• If an operation on some variable would produce UB with value x, the variable does not hold that value. For example, this allows the compiler to optimize away repeated NULL checks, even on systems that technically have a zero page, or if you were to catch the segfault signal.

In other words, my understanding is: UB in dead branches has an effect on the result of the program, i.e. the compiler can assume the program is correct, and if the branch were reachable the program would not be correct, so the branch is unreachable. But that doesn't mean UB in dead branches makes the behavior of your program undefined if the branches are actually never reached.

That's my understanding, though to be honest, the C spec is not nearly explicit enough about this...

[ndossche]

Unsure, but now I wonder if we even need this.
Doesn't this work: (zend_object *)(obj + 1) - 1, at least for the objects. If std is at the end this should work for all types.

[TimWolla]

Doesn't this work:

It likely would, but my understanding is that it would be equally UB. You can't just cast between pointers of different types. (and this already affected the original implementation, you may go to char *, but may not go from char * to something else).

My suggestion would be to just go with the established way of writing a container_of macro that reasonable compilers will compile to the expected machine code. php-src is already quite reliant on all kinds of technically-UB code, such as the generic zend_function struct.

[ndossche]

Okay then

[TimWolla]

Ah. C++. I'll see if I can add a template-variant.

[derickr]

I see no specific issues with ext/date and the replacement with the casts with the macro.

[arnaud-lb]

I can not check the C++ variant, but this looks good to me otherwise

[arnaud-lb]

Nit: This errors if the member is const because this results in two compatible _Generic cases:

struct Foo {
    int a;
    const double b;
};


const struct Foo *test(const double *v) {
    // error: '_Generic' specifies two compatible types
    return ZEND_CONTAINER_OF(v, struct Foo, b);
}

Using typeof_unqual() would solve this, but I'm not sure about availability. Also may not be a critical issue as it should be rare, and it doesn't silently breaks correctness.

[TimWolla]

Using typeof_unqual() would solve this, but I'm not sure about availability.

As far as I can tell it's in C23 just like typeof() itself, but it's not available as a gcc extension, which would mean that it can only be used if the compiler uses C23 (by default). Given that const fields within a struct should be super rare due to their terrible ergonomics, I would skip this issue for now and revisit it once it actually becomes a problem.