[vlc-devel] [PATCH 0/3] Respect C++ ODR (first patches)

[Romain Vimont]

On Sun, Apr 22, 2018 at 07:28:48PM +0300, Rémi Denis-Courmont wrote:
> Le dimanche 22 avril 2018, 18:03:35 EEST Romain Vimont a écrit :
> > In C++, there cannot be more than one definition of a single type in the
> > entire program:
> > <http://en.cppreference.com/w/cpp/language/definition#One_Definition_Rule>
> > 
> > This has been reported by #17078 and #18033.
> > 
> > To fix the issue:
> >  1. In C++ code, declare *_sys_t types in a separate namespace, either
> >     unnamed if it's used only in one translation unit, or named otherwise.
> >  2. Remove the *_sys_t typedefs in include/vlc_common.h.
> >  3. Declare *_sys_t typedefs locally for C modules.
> >  4. Declare references to private data as "void *" instead of "*_sys_t *".
> >  5. Adapt code that now dereferences "void *" or relied on the *_sys_t
> >     forward declarations.
> 
> This might fix the compilation, but as long as the same typename is defined 
> multiple times, the debugging will remain broken.

At least, the program behavior is not undefined due to ODR anymore.

It seems to me that it also solves debugging "confusion", at least as I
experienced it.

For example, on commit 5a20d22~ (parent of 5a20d22), in
modules/demux/mkv/mkv.cpp:Demux, it is clear that gdb decodes p_sys as a
demux_sys_t structure from modules/demux/sid.cpp:

    (gdb) p p_sys->i_pcr
    There is no member named i_pcr.
    (gdb) p *p_sys
    $1 = {player = 0x7fff9240f700, config = {clockDefault = (unknown: 2160103168), clockForced = 255,
       ...

On the contrary, after applying the ODR patch for demux_sys_t ([PATCH
1/3]), gdb correctly decodes it against the right demux_sys_t from
modules/demux/mkv/demux.hpp:

    (gdb) p p_sys->i_pcr
    $1 = 0
    (gdb) p *p_sys
    $2 = {_vptr.demux_sys_t = 0x7fff9240f700, demuxer = @0x7fff80c14b10, b_seekable = true, b_fastseekable = true, i_pts = 1, i_pcr = 0,
       ...

> Also, as far as aliasing problems are concerned, there is another potential 
> separate problem. I am not sure if treating objects alternatively as 
> vlc_object_t and as ${MODULE_TYPE}_t is valid by C++ either. In fact, I am not 
> even sure if it is valid in C.

I agree there might be aliasing problems. Maybe we should compile with
-fno-strict-aliasing for now, unless we are convinced that there are no
strict aliasing violations or that they have been fixed, what do you
think?