On Mon, Oct 12, 2015 at 11:23 AM, wm4 <nfx...@googlemail.com> wrote: > On Mon, 12 Oct 2015 11:16:00 -0400 > Ganesh Ajjanagadde <gajja...@mit.edu> wrote: > >> On Mon, Oct 12, 2015 at 10:54 AM, Ronald S. Bultje <rsbul...@gmail.com> >> wrote: >> > Hi, >> > >> > On Mon, Oct 12, 2015 at 10:51 AM, Ganesh Ajjanagadde <gajja...@mit.edu> >> > wrote: >> >> >> >> 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? >> > >> > >> > 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. > > Haven't really been following this, but how about we disable warnings > that just waste our time?
Agreed. However, I don't consider this one a waste of time - I give an explicit commit ID that shows how -Wstrict-overflow caught something. Warray-bounds on the other hand - I think it is worth disabling, at least locally (all were false positives till date). I would still hesitate to do it globally though, given the number of buffers we use and their large security implications. > _______________________________________________ > 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