[FFmpeg-devel] Removing DCE

[Matt Oliver]

On 23 December 2016 at 15:35, Matt Oliver <protogonoi at gmail.com> wrote:

> 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.
>
>
So I did a quick check over the FFmpeg source for all occurrences of DCE in
the code base. So far I have found the following:
251 uses of DCE with ARCH_XXX
54 uses of DCE with HAVE_XXX
205 uses of DCE with CONFIG_XXX


Basically all of the uses of ARCH_XXX with DCE just call a single function
such as:
if (ARCH_X86)
        ff_aac_dsp_init_x86(s);

These are one of the main problems with DCE as the single function call is
generally undefined on all other toolchains that don't have ARCH_XXX and so
cause linker errors. These are the ones that primarily need to be changed
to fix the linker errors. Also the use of DCE here is not providing much
benefit as far as checking whether code compiles because as long as you
cut/paste the function name there isnt much to get wrong here. So the main
differences will be the addition of an extra line for the '#endif' if
changed to not using DCE.

The use of HAVE_XXX is more complicated as most of the occurrences contain
complex code within the if statement. However these occurrences do _NOT_
need to be changed to using non DCE in order to fix the linker errors. It
is only the DCE sections that call undefined functions that break things so
any use of DCE surrounding pure code blocks can be left untouched. So apart
from a couple of occurrences of "if(HAVE_XXX)" that just have a single
undefined function call proceeding them (same as the ARCH_XXX cases) none
of the other occurrences need to be changed to correct linking issues.

Use of CONFIG_XXX in the code base is similar to HAVE_XXX. Again only a
small number of them actually need to be changed in order fix linking
issues and most can stay untouched.


So to summarize if all we want to do is fix linking problems on various
toolchains then we DONT actually need to remove all uses of DCE. We only
need to remove the uses that contain a undefined function call. Around >90%
of those cases are identical to the ARCH_X86 example above where it is a
minimal change. For the other occurrences that contain complex code blocks
(and the ones where being able to do a compilation check is useful) DONT
need to be changed so the advantages of DCE can still be used. So for >90%
of the changes the only main difference will probably just be the change in
syntax from an 'if' to an '#if/#endif' which i would hope can be accepted
as a valid compromise in order to get compilation on affected toolchains to
work.