[FFmpeg-devel] [PATCH] avformat/movenc: suppress -Wstrict-overflow warnings

Ganesh Ajjanagadde gajjanag at mit.edu
Sun Sep 27 19:23:03 CEST 2015


On Sun, Sep 27, 2015 at 12:58 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sat, Sep 26, 2015 at 10:55:26PM -0400, Ganesh Ajjanagadde wrote:
>> On Sat, Sep 26, 2015 at 10:32 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
>> > Hi,
>> >
>> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>> > wrote:
>> >
>> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer <michaelni at gmx.at>
>> >> wrote:
>> >> > On Fri, Sep 18, 2015 at 05:15:50PM -0400, Ganesh Ajjanagadde wrote:
>> >> >> This patch results in identical behavior of movenc, and suppresses
>> >> -Wstrict-overflow
>> >> >> warnings observed in GCC 5.2.
>> >> >> I have manually checked that all usages are safe, and overflow
>> >> possibility does
>> >> >> not exist with this expression rewrite.
>> >> >>
>> >> >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> >> >> ---
>> >> >>  libavformat/movenc.c | 2 +-
>> >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> >> >> index af03d1e..6e4a1a6 100644
>> >> >> --- a/libavformat/movenc.c
>> >> >> +++ b/libavformat/movenc.c
>> >> >> @@ -854,7 +854,7 @@ static int get_cluster_duration(MOVTrack *track,
>> >> int cluster_idx)
>> >> >>  {
>> >> >>      int64_t next_dts;
>> >> >>
>> >> >> -    if (cluster_idx >= track->entry)
>> >> >> +    if (cluster_idx - track->entry >= 0)
>> >> >
>> >> > i do not understand what this fixes or why
>> >> > also plese quote the actual warnings which are fixed in the commit
>> >> > message
>> >>
>> >> I have posted v2 with a more detailed commit message. It should be
>> >> self explanatory.
>> >
>> >
>> > Even with the new message, it's still not clear to me what's being fixed.
>> > What does the warning check for? What is the problem in the initial
>> > expression?
>>
>> Compilers make transformations on the statements in order to possibly
>> get better performance when compiled with optimizations. However, some
>> of these optimizations require assumptions in the code. In particular,
>> the compiler is internally rewriting cluster_idx >= track->entry to
>> cluster_idx - track->entry >= 0 internally for some reason (I am not
>> an asm/instruction set guy, so I can't comment why it likes this).
>> However, such a transformation is NOT always safe as integer
>> arithmetic can overflow (try e.g extreme values close to INT_MIN,
>> INT_MAX). The warning is spit out since the compiler can't be sure
>> that this is safe, but it still wants to do it (I suspect only the
>> -O3/-O2 level that try this, can check if you want).
>
> iam not sure i understand correctly but
> if the compiler changes the code and then warns that what it just
> did might be unsafe then the compiler is broken

https://stackoverflow.com/questions/12984861/dont-understand-assuming-signed-overflow-warning
- gives a detailed explanation.

Some more info: this is triggered only when -finline-functions is
enabled (done by default on -O3, not enabled by default on -O2).
-finline-functions tries to inline stuff even when "inline" keyword is
absent (like in this case).
As for the warning, http://linux.die.net/man/1/gcc - search for
-Wstrict-overflow. It is enabled due to -Wall, and as the man page
suggests, it depends on optimization level as we can see in this
example.
I do consider the compiler broken in this case, but then again
compilers are broken in so many different ways it is not even funny:
see e.g -Warray-bounds, can't use the ISO C correct { 0 } initializer
for compound data types, etc.

If you don't like this, we should add a -Wnostrict-overflow either to
configure, or a local enable/disable via pragmas/macros. I don't like
either of these as compared to this simple workaround:
1. -Wnostrict-overflow: FFmpeg with the amount of integer arithmetic
being done should benefit from this warning in general, so disabling
it globally may be bad.
2. local enable/disable: too ugly and overkill when we need only 2
lines to be changed in trivial ways throughout the codebase: movenc
and xface. IMO, we should consider this only if we run into more false
positives.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I know you won't believe me, but the highest form of Human Excellence is
> to question oneself and others. -- Socrates
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


More information about the ffmpeg-devel mailing list