[pulseaudio-discuss] Allowing anonymous structs and unions

[Ahmed S. Darwish]

Hi Arun, everyone,

On Wed, Sep 23, 2015 at 06:07:44PM +0530, Arun Raghavan wrote:
> Hi Ahmed,
> 
> On 23 September 2015 at 14:04, Ahmed S. Darwish <darwish.07 at gmail.com> wrote:
> > Hi everyone,
> >
> > On Wed, Sep 23, 2015 at 02:27:55AM -0400, Alexander E. Patrakov wrote:
> >> On 09/23/2015 02:12 AM, Tanu Kaskinen wrote:
> >> >Hi all,
> >> >
> >> >We haven't been using anonymous structs and unions so far, because
> >> >they've been non-standard in the past. C11 added support for them,
> >> >however. GCC has supported them longer than that, I don't know about
> >> >other compilers. Anonymous structs and unions are nice, and I think we
> >> >should start allowing them. Any objections?
> >>
> >> I think it would be a good idea to send one small example patch that makes a
> >> use of them, just to demonstrate where exactly in PulseAudio they would make
> >> the code more elegant.
> >>
> >
> > Sure, this pattern is used heavily in the memfd patches :-)
> >
> > ### Anonymous unions
> >
> > Used to provide the mempool with polymorphic memory region access
> > regardless of the type of memory used:
> >
> >     union {
> >         pa_mem mem;
> >         union {
> >             pa_shm shm;
> >             pa_memfd memfd;
> >             pa_privatemem privatemem;
> >         } per_type;
> >     };
> >
> > This way, a lot of the mempools code can transparently access its
> > backend memory by using `mempool->mem.ptr' and `mempool->mem.size`.
> >
> > This also saves us from polluting the mempool memblock.c code with
> > a huge number of switch cases ;-)
> 
> The more common way to do this is by embedding the parent struct. This
> makes for cleaner code, because (a) we're now depending on the
> compiler packing all the members of the union exactly the same (only
> the parent is packed, and not the children), and (b) the packed
> attribute that can generate less efficient code, and (c) attribute
> packing is not a language feature, so how it works can vary across
> compilers.
>
> typedef struct {
>   void *ptr;
>   size_t size;
> } pa_mem;
> 
> #define PA_MEM(a) ((pa_mem *) (a))
> #define PA_MEM_PTR(a) (PA_MEM(a)->ptr)
> 
> typedef struct {
>   pa_mem mem;
>   int fd;
> } pa_memfd;
> 
> typedef struct {
>   pa_mem mem;
>   int id;
>   bool do_unlink;
> } pa_shm;
> 
> Then you can basically do things like:
> 
>   pa_memfd *memfd;
>   ...
>   memcpy(PA_MEM_PTR(memfd), src, n);
> 
> You can see example usage of this pattern in pa_object, pa_msgobject,
> etc. (and that includes some optional GLib-esque run-time type
> checking too).
>

OK, after a second look, it seems that the anonymous structure indeed
does not provide any extra benefit. So I believe everyone now agrees
now that the following definitions are the most appropriate:

    typedef struct {
        pa_mem mem;           /* Parent; must be first */
        int fd;
    } pa_memfd;

    typedef struct {
        pa_mem mem;           /* Parent; must be first */
        int id;
        bool do_unlink;
    } pa_shm;

Good :-)

But I can't get my head around not using the anonymous unions, and
basically whether they provide any perceived disadvantage:

    struct pa_mempool {
        ...
        union {
            pa_mem mem;
            union {
                pa_shm shm;
                pa_memfd memfd;
                pa_privatemem privatemem;
            } per_type;
        };
        ...
    }

Kindly note that

- pa_shm/memfd/privatemem are not dynamically allocated with a _new()
  constructor and then exclusively accessed through a pointer like
  what is done with the GLIB-esque objects. They are embedded within
  'pa_mempool' and 'pa_memimport_segment' instead.

- The C standard _requires_ zero padding between a structure and its
  first element. [*] So we have a guarantee that the start address of
  the union itself is pa_mem, and the start address of pa_shm,
  pa_memfd, and pa_privatemem is also pa_mem.

- Given the above, we do not need to put any packing attributes on
  pa_mem. Regardless of how the compiler aligns the pa_mem elements
  themselves, we don't care.

- All of the memblock.c and pstream.c code is already embedding
  objects and using the `per_type' union trick instead of the
  GLIB-esque pattern of dynamic memory allocation and casting. That
  is why the memfd patches (where most of its important code is
  indeed under memblock.c and pstream.c) followed suit.

Yes, we can transfrom the parent union above into a single pointer:

    struct pa_mempool {
        ...
        pa_mem *mem;
        ...
    }

And then like GLIB we can do do:

    pa_mempool *pa_mempool_new(pa_mem_type_t type, ...) {

        switch (type) {
        case PA_MEMORY_SHARED_POSIX:
             mem = pa_shm_new(...);
             ...
        case PA_MEMORY_SHARED_MEMFD:
             mem = pa_memfd_new(...);
             ...
        }
    }

And then whenever we want to access the shm or memfd fields
themselves we can do:

        switch (type) {
        case PA_MEMORY_SHARED_POSIX:
             shm = PA_SHM(mempool->mem);
             xx = shm->id;
             ...
        case PA_MEMORY_SHARED_MEMFD:
             memfd = PA_MEMFD(mempool->mem);
             yy = memfd->fd;
             ...
        }

And this is indeed what is done with pa_msgobject and its children
objects like pa_sink, pa_core, file_stream, etc.

I personally prefer the first pattern: it needs no further dynamic
allocations and it goes more in line with rest of the memblock and
pstreams code.

Nonetheless, I honestly have no hard opinions. But .. if the second
pattern is still preferred, maybe we could then just use pa_object
and its interfaces instead of re-inventing our own?

Thanks,

[*] Otherwise a lot of the fancy stuff done in kernels, virtual
    machines, and networking stacks will simply fail. Quoting the
    C standard structure and union specifiers section: "there may
    be unnamed padding within a structure object, but not at its
    beginning."

-- 
Darwish
http://darwish.chasingpointers.com