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

Ganesh Ajjanagadde gajjanag at mit.edu
Mon Oct 12 16:51:12 CEST 2015 

On Thu, Oct 8, 2015 at 5:07 PM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Sat, Oct 3, 2015 at 8:17 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>> On Tue, Sep 29, 2015 at 10:49 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>> On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>>> On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>>> On Sun, Sep 27, 2015 at 01:23:03PM -0400, Ganesh Ajjanagadde wrote:
>>>>>> 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.
>>>>>
>>>>> how many actual bugs has Wstrict-overflow found ?
>>>>
>>>> No idea; maybe a good place to check is the Google fuzzing effort
>>>> where many bugs were fixed.
>>>
>>> See e.g your commit: 09ef98f1ae3c8a4e08b66f41c3bd97dd7b07405f -
>>> Wstrict-overflow is indeed useful.
>>> I am thus convinced that we should retain it.
>>> Given the fact that local suppression is not worth it for just 2
>>> instances and also that the patch does not reduce readability in any
>>> way, I think this patch and the one for xface are ok.
>
> Here is some more detailed digging. Please note that I am not certain
> of the following, but someone with more asm experience could possibly
> confirm.
>
> First, recall that this is only triggered with -finline-functions
> (automatically enabled at -O3). What -finline-functions does is lets
> the compiler inline stuff even when the "inline" keyword is not used
> when the compiler finds it beneficial. Now when the compiler inlines
> this, it wants to rewrite the expression
> cluster_idx >= track->entry
> to
> cluster_idx - track->entry >= 0,
> a transformation that is not always safe due to integer overflow
> possibilities as explained in detail above. However, as I mention in
> the commit message, I have manually audited and made sure all such
> transformations are safe.
>
> Now the question is: why does it want to do that? The answer is hinted
> at in the warning messages themselves:
>
> libavformat/movenc.c: In function ‘mov_flush_fragment’:
> libavformat/movenc.c:4112:12: warning: assuming signed overflow does
> not occur when assuming that (X - c) > X is always false
> [-Wstrict-overflow]
>  static int mov_flush_fragment(AVFormatContext *s)
>             ^
> libavformat/movenc.c:4112:12: warning: assuming signed overflow does
> not occur when assuming that (X - c) > X is always false
> [-Wstrict-overflow]
> libavformat/movenc.c:857:8: warning: assuming signed overflow does not
> occur when assuming that (X - c) > X is always false
> [-Wstrict-overflow]
>      if (cluster_idx >= track->entry)
>
> In mov_flush_fragment, there are multiple calls to
> get_cluster_duration that are getting inlined due to
> -finline-functions:
> line 4132:        if (get_cluster_duration(track, track->entry - 1) != 0)
> line 4138:         track->track_duration +=
> get_cluster_duration(track, track->entry - 2);
> line 4139:         track->end_pts        +=
> get_cluster_duration(track, track->entry - 2);
>
> Examining these closely, the compiler can easily do a constant
> propagation if it assumes the lack of integer overflow:
> track->entry >= track->entry - 2 is not always true depending on
> track->entry's value (take near INT_MIN for instance),
> but if transformed as I indicated, it becomes a -2 >= 0 (and easily
> optimized out) since the compiler assumes associative arithmetic on
> integers when optimizations are enabled. Yes, illustrated above is the
> fact that any kind of arithmetic expression rewrite (including simple
> associative transformations) is potentially unsafe due to overflow,
> but the compiler wants (since we use O3) and I am sure everybody here
> likes the fact that we get optimizations for them. It wants to warn us
> about the "crazier" arithmetic transformation regarding the >=
> comparison.
>
> Overall, I still believe this patch should be applied:
> 1. It is a clean solution - the code in no way is less readable.
> 2. It silences the compiler.
> 3. Alternative approaches here are far worse for IMHO little gain (e.g
> local disable is much more noisy than the above, disabling of
> -Wstrict-overflow altogether is a terrible idea due to e.g commit
> 09ef98f1ae3c8a4e08b66f41c3bd97dd7b07405f).
> 4. I no longer view it as a compiler bug - it wants to do an
> optimization, I am sure we would like it as well, but its hands are
> tied until we "feed" it some information that the transformation is
> safe. Note that it is essentially impossible for compilers to do such
> "confirmation" analysis themselves, I needed to do a nontrivial manual
> audit myself.

You still don't feel that this is the cleanest and simplest solution
to this problem?

>
>>
>> ping.
>>
>>>
>>>>
>>>>>
>>>>> [...]
>>>>> --
>>>>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>>>>
>>>>> The real ebay dictionary, page 3
>>>>> "Rare item" - "Common item with rare defect or maybe just a lie"
>>>>> "Professional" - "'Toy' made in china, not functional except as doorstop"
>>>>> "Experts will know" - "The seller hopes you are not an expert"
>>>>>
>>>>> _______________________________________________
>>>>> ffmpeg-devel mailing list
>>>>> ffmpeg-devel at ffmpeg.org
>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>

More information about the ffmpeg-devel mailing list