[FFmpeg-devel] [PATCH] avformat/movenc: suppress -Wstrict-overflow warnings
Ganesh Ajjanagadde
gajjanag at mit.edu
Thu Oct 8 23:07:17 CEST 2015
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.
>
> 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