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

Ganesh Ajjanagadde gajjanag at mit.edu
Sat Oct 3 14:17:35 CEST 2015


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.

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