[FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Andreas Rheinhardt]

Nicolas George:
> Andreas Rheinhardt (12023-02-01):
>> "One special guarantee is made in order to simplify the use of unions:
>> if a union contains several structures that share a common initial
>> sequence (see below), and if the union object currently contains one of
>> these structures, it is permitted to inspect the common initial part of
>> any of them anywhere that a declaration of the completed type of the
>> union is visible. Two structures share a common initial sequence if
>> corresponding members have compatible types (and, for bit-fields, the
>> same widths) for a sequence of one or more initial members."
>>
>> But there just is no union between the FF_INTERNAL_FIELDS
>> !defined(FF_INTERNAL_FIELDS) structures in the whole codebase.
> 
> It is not necessary that there is one: it is enough that there could be
> one in another compilation unit, the code that handles these structures
> does not know what exists in the rest of the code base. This guarantee
> was made FOR unions, but it does not REQUIRE unions, it applies to
> anything that is semantically equivalent to a union.
> 

1. The common initial sequence rule indeed forced traditional compilers
to use the same offsets for the members of the common initial sequence
of two arbitrary structs, because there might be a union of these two in
another translation unit. But this is no longer true for lto compilers
that see the whole program at once (although I don't think that they
will deviate from the behaviour of traditional compilers in this regard).
2. But even if the offsets are fine, does not mean that the accesses are
fine. Notice the clause "anywhere that a declaration of the completed
type of the union is visible" above? It is accompanied with the
following example:

"The following is not a valid fragment (because the union type is not
visible within function f):
struct t1 { int m; };
struct t2 { int m; };
int f(struct t1 *p1, struct t2 *p2)
{
    if (p1->m < 0)
        p2->m = -p2->m;
    return p1->m;
}
int g()
{
    union {
        struct t1 s1;
        struct t2 s2;
    } u;
    /* ... */
    return f(&u.s1, &u.s2);
}"


>> Furthermore, said guarantee is only for inspecting, i.e. reading. For
>> example, for the following two structs sharing a common initial sequence,
>>
>> struct Small {
>>     uint64_t foo;
>>     uint32_t bar;
>> };
>> struct Big {
>>     uint64_t foo;
>>     uint32_t bar;
>>     uint32_t baz;
>> };
>>
>> if one had a union { struct Small; struct Big; }, a write to Small.bar
>> may clobber Big.baz.
> 
> That is a good point. It does not apply when the first field of Big is
> Small itself, which is the case that I absolutely know is supported,
> because in that case Big will embed the padding of Small. I had not
> thought of this.
> 
> Fortunately, even if we are not 100% in the case that is officially
> supported, there are multiple reasons that each should be enough to
> guarantee that our code will work:
> 

I'd rather have something that is actually supported and spec-compliant.
Anyway, I worry more about lto-compilers than about traditional
compilers accidentally clobbering something (after all, most of our
fields are sizeof(int)-sized or a multiple (double) thereof, so CPU
instruction sets can write this natively and there is no advantage in
touching padding).

> - Between the public part of the structure and the #ifede
>   FF_INTERNAL_FIELDS, there are a lot of "not part of the public API"
>   fields", changing ch_layout will not clobber incfg because incfg is
>   known at this point.
> 
> - Even if there was no fields in between, reserved[] offers the same
>   protection.
> 
> - And even if there was no gap, we are good because applications, and
>   even filters, are not supposed to modify AVFilterLink, only the
>   framework and the framework knows the whole structure.
> 
> In fact, that last remark makes me think of another solution, for the
> slightly longer term: make AVFilterLink entirely opaque. The only
> documented reason for application to access AVFilterLink was to get the
> parameters of buffersink, but buffersink has a real API to get this
> since 2016.
> 
> Anyway, if you prefer using a compilation option, I have no objection.
> My only concern is that when a developer writes:
> 
> 	if (link->x == ... &&
> 	    link->y == ... &&
> 	    link->status_in == ... &&
> 	    link->min_samples == ..)
> 
> they have to remember that no, status_in is different from all the
> others.
> 
> And it is especially bad in this particular case because the distinction
> is not public / private, which makes some semantic sense and might be
> easier to remember, the distinction is actually
> public-or-private-because-of-a-comment / private-because-of-a-ifdef.
> 
> But even if the distinction really was public / private, I consider it
> unacceptable if we can do otherwise. And we can.
> 
> Regards,
>