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? _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel