[FFmpeg-devel] Removing DCE

[Matt Oliver]

On 21 December 2016 at 23:55, wm4 <nfxjfg at googlemail.com> wrote:

> On Fri, 16 Dec 2016 13:48:16 +1100
> Matt Oliver <protogonoi at gmail.com> wrote:
>
> > Recently we have again received several patches that are trying to add
> > workarounds for ffmpegs use of DCE. This is not the first time this has
> > happened and wont be the last until a decision is made about the use of
> > DCE. So I think its time that we made a official decision on the use of
> > DCE. This is of course something that should be properly agreed upon by
> > developers going forward as different people have different opinions on
> the
> > matter so to help assist this I will summaries all of the previously made
> > arguments from both sides of the discussion.
> >
> > For DCE:
> > 1) Ends up with a horrible mess of ifdefs.
> > 2) Disabled parts of code will not be checked for syntax.
> >
> > Against DCE:
> > 3) DCE is not actually specified in the C specification. So compilers are
> > actually being 100% compliant by not supporting it and should not be
> > expected to change just for ffmpegs use case.
> > 4) Breaks debug and lto builds on msvc.
> > 5) Breaks debug and lto builds on icl.
> > 6) Issues with lto with Clang and Gold.
> > 7) Other unspecified issues with debug builds
> >
> > Rebuttals against above arguments:
> > 8) There are already 3961 #ifs(not including header guards) in the code
> so
> > there is already a lot of code that doesn't use DCE. (In response to #1
> for
> > DCE).
> > 9) Avoiding #ifdefs is a personal opinion as to whether they are ugly or
> > not (some prefer ifdefs as IDEs will correctly highlight disabled
> > sections). Someones personal preference for what looks better should not
> be
> > justification to break supported configurations/compilers. (In response
> to
> > #1 for DCE).
> > 10) There is --enable-random which is supposed to be used to find
> > configurations that don't compile. (in response to #2 for DCE).
> > 11) Just because something compiles does not mean that it actually works,
> > relying on just syntax checking is dangerous as it gives false security
> as
> > the code is not actually being tested. (in response to #2 for DCE)
> > 12) There are numerous FATE tests that should find all the configuration
> > errors. (in response to #2 for DCE)
> > 12) MSVC is broken and we shouldn't support it so Microsoft are forced to
> > fix it (in response to #4 against DCE) - This point is countermanded by
> #3
> > against DCE and reported issues with other compilers as well.
> >
> >
> > Most of the above points were taken from peoples posts in the following
> > mailing list thread:
> > https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2016-April/193437.html
> > https://ffmpeg.org/pipermail/ffmpeg-devel/2016-May/194115.html
> >
> > Its my personal opinion that DCE should be removed from the code but this
> > is something I am aware will require a developer consensus and perhaps
> even
> > a vote. Stating something is broken is one thing so I will also put
> forward
> > a solution in that if it is agreed upon to remove DCE usage then I will
> > spend the time and effort to go through the existing code base and
> replace
> > DCE with appropriate #ifs.
>
> I was completely lost here, until I opened one of the link and realized
> you're talking about Dead Code Elimination.
>
> Summary for those missing context: we rely on DCE to remove code that
> would otherwise fail at link time.
>
> For example consider this code, which will be compiled on _all_
> platforms:
>
>   void decode_something_x86();
>
>   if (HAVE_X86)
>      function_ptr = decode_something_x86;
>
> Now if this is compiled on ARM, HAVE_X86 will be 0, and the
> function_ptr assignment is dead code. DCE will remove this code, and
> remove the decode_something_x86 reference. If DCE doesn't work, this
> will fail to link, since decode_something_x86 will not be defined
> anywhere on ARM.
>
> I still think this is a bad idea, because compilers are not required to
> perform DCE. In fact, ffmpeg will fail to compile with many compilers
> if optimizations are disabled. This can make debugging a pain. Beyond
> that, it could legitimately fail if the compiler just decides not to do
> DCE for whatever reasons. (The argument has always been that a compiler
> that fails to DCE such simple cases is not worth being used... strange
> argument, since we work around other compiler bugs in a regular
> fashion.)


Yes, so basically the discussion is about converting things that currently
look like the following (1):

#define CONFIG_SOME_OPTION 0
...
        if (CONFIG_SOME_OPTION) {
            ff_undefined_function();
            //some other code here
            ...
        }

where the use of the above coding style causes link errors on certain
toolchains because ff_undefined_function is undefined.
The option is to convert code like the above to (2):

#define CONFIG_SOME_OPTION 0
...
#if CONFIG_SOME_OPTION
            ff_undefined_function();
            //some other code here
            ...
#endif

or depending on preferences (3):

#define CONFIG_SOME_OPTION 0
...
#        if CONFIG_SOME_OPTION
            ff_undefined_function();
            //some other code here
            ...
#        endif

Using either (2) or (3) results in compilation succeeding on toolchains
where (1) does not. But so far people have resisted changing as they either
dont like the appearance of (2)/(3) over (1) or because when using (1) any
additional code within the if statement is still checked by the compiler
for consistency (even though it is never executed). Some developers use
this as a means to do quick checks to see if the code compiles without
having to actually compile the code with that feature enabled and
explicitly check it.