On Thu, Oct 8, 2015 at 5:07 PM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: > On Sat, Oct 3, 2015 at 8:17 AM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: >> 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. > > 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@ffmpeg.org >>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>>> _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel