On Tue, Sep 29, 2015 at 10:49 AM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: > On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: >> On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer <michae...@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 <michae...@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 <rsbul...@gmail.com> >>>> >> wrote: >>>> >> > Hi, >>>> >> > >>>> >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde <gajja...@mit.edu> >>>> >> > wrote: >>>> >> > >>>> >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer >>>> >> >> <michae...@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 <gajjanaga...@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@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel