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

[Ganesh Ajjanagadde]

On Mon, Oct 12, 2015 at 10:54 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Mon, Oct 12, 2015 at 10:51 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
> wrote:
>>
>> 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?
>
>
> I've been passively following this. No, I honestly don't think it is. I
> understand what the compiler is doing and why, I think your analysis is
> pretty good.
>
> But no, a - b >= 0 is not better than a >= b. a >= b is exactly what's
> intended here. I understand why the compiler wants to rewrite it, but this
> code isn't remotely performance-sensitive, so I'd rather go for explanatory
> code.
>
> I tend to agree with Michael that in this particular case, the warning by
> the compiler suggests it's doing something it shouldn't do. Maybe we'd want
> it to do it, but again, this code is not performance-sensitive, so
> explanatory code is more important then.

All right, then how about a compromise: I can add a small comment
right above this change saying what we really mean, and give the
rewrite right below it?

That yields the same level of explanation, at the cost of a small
increase in verbosity due to the comment. The function is anyway not
too big, so it should not be  an issue.

But yes, if we run into this more often than 2 instances, we can come
up with a more sustainable solution.

>
> Ronald