[FFmpeg-devel] Removing DCE

[wm4]

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.)