Re: [FFmpeg-devel] [PATCH 00/22] Deprecate av_uninit

[Nicolas George <george@nsup.org>]

Zhao Zhili (HE12025-04-11):
> With UB, the compiler can remove branch check and assign some random
> value to it, which cannot be detected by valgrind.
> 
> For ab792634197e, the UB is there for decades and never detected by
> valgrind, and the warning is silenced by av_uninit.

You make a valid point, but my own point still stands: your change make
it harder to find bugs by removing the difference between “I put this
initialization there because it is the right value” and “I put this
there because the compiler is too stupid to figure it on its own”.

I have to ideas to accommodate both issues:

- Have a FATE instance with valgrind and optimizations disabled, so that
  the compiler does not optimize the code away because of the UB and
  valgrind catches the bug.

- Initialize to 0xDEADBEEF and add av_assert2(val != 0xDEADBEEF) at the
  place where it will be used.

The second one might be fragile, but less than an UB.

> By the way, logic bug isn’t equal to UB, so I’m not hiding UB.

Yes you are.

> Who put av_uninit in the code means there is no logic bug. If there is,
> the patchset fixed UB or replaced UB by deterministic logic error, which
> can’t be worse.

The deterministic logic error is not worse, but it is not better either,
and it is harder to detect.

> If there is, the patchset fixed or makes the issue deterministic.

This patches fixes NOTHING, I hope we agree on it. What you did is
basically equivalent to removing an assert that fails and hoping the
code that comes later will be fine.

Making the bug deterministic is not better if it makes it harder to
detect.

> We don’t initialized all variables when declaration. But if there is a
> sometimes-uninitialized warning, there is some reason for compiler.
> Uninitialized warning isn’t the same as deprecated or unused, it should
> never be ignored in my opinion.

You are wrong on this, the compiler is often unable to figure out that
the code always sets the variable before reading it when the developer
can prove it. Compilers are getting better, but they are still far from
perfect, and av_unused is precisely there for that lack of perfection,
and needs to stay there.

Regards,

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".