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

Ganesh Ajjanagadde gajjanag at mit.edu
Sat Oct 17 20:04:35 CEST 2015


On Sat, Oct 17, 2015 at 1:51 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Fri, Oct 16, 2015 at 8:45 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
> wrote:
>
>> On Fri, Oct 16, 2015 at 8:34 PM, Ronald S. Bultje <rsbultje at gmail.com>
>> wrote:
>> > Hi,
>> >
>> > On Fri, Oct 16, 2015 at 8:18 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>> > wrote:
>> >
>> >> On Fri, Oct 16, 2015 at 8:05 PM, Ronald S. Bultje <rsbultje at gmail.com>
>> >> wrote:
>> >> > Hi,
>> >> >
>> >> > On Fri, Oct 16, 2015 at 5:48 PM, Ganesh Ajjanagadde <gajjanag at mit.edu
>> >
>> >> > wrote:
>> >> >
>> >> >> On Fri, Oct 16, 2015 at 5:45 PM, Hendrik Leppkes <
>> h.leppkes at gmail.com>
>> >> >> wrote:
>> >> >> > On Fri, Oct 16, 2015 at 11:39 PM, Ganesh Ajjanagadde
>> >> >> > <gajjanagadde at gmail.com> wrote:
>> >> >> >> On Wed, Oct 14, 2015 at 10:05 PM, Ganesh Ajjanagadde
>> >> >> >> <gajjanagadde at gmail.com> wrote:
>> >> >> >>> This patch results in identical behavior of movenc, and
>> suppresses
>> >> >> -Wstrict-overflow
>> >> >> >>> warnings observed in GCC 5.2:
>> >> >> >>>
>> >> >>
>> >>
>> http://fate.ffmpeg.org/log.cgi?time=20150926231053&log=compile&slot=x86_64-archlinux-gcc-threads-misc
>> >> >> ,
>> >> >> >>> "warning: assuming signed overflow does not occur when assuming
>> that
>> >> >> (X - c) > X is always false [-Wstrict-overflow]"
>> >> >> >>> I have manually checked that all usages are safe, and overflow
>> >> >> possibility does
>> >> >> >>> not exist with this expression rewrite.
>> >> >> >>>
>> >> >> >>> Some expressed concern over readability loss, hence a comment is
>> >> added.
>> >> >> >>> This is the simplest way to suppress this warning.
>> >> >> >>>
>> >> >> >>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> >> >> >>> ---
>> >> >> >>>  libavformat/movenc.c | 4 +++-
>> >> >> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >> >> >>>
>> >> >> >>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> >> >> >>> index 5115585..ff997f2 100644
>> >> >> >>> --- a/libavformat/movenc.c
>> >> >> >>> +++ b/libavformat/movenc.c
>> >> >> >>> @@ -854,7 +854,9 @@ static int get_cluster_duration(MOVTrack
>> *track,
>> >> >> int cluster_idx)
>> >> >> >>>  {
>> >> >> >>>      int64_t next_dts;
>> >> >> >>>
>> >> >> >>> -    if (cluster_idx >= track->entry)
>> >> >> >>> +    /* GCC 5.2 wants to "optimize" cluster_idx >= track->entry
>> to
>> >> the
>> >> >> below
>> >> >> >>> +     * expression. We actually mean cluster_idx >=
>> track->entry. */
>> >> >> >>> +    if (cluster_idx - track->entry >= 0)
>> >> >> >>>          return 0;
>> >> >> >>>
>> >> >> >>>      if (cluster_idx + 1 == track->entry)
>> >> >> >>> --
>> >> >> >>> 2.6.1
>> >> >> >>>
>> >> >> >>
>> >> >> >> ping, is this solution acceptable? Note that I will get rid of the
>> >> >> >> extra * on the second line of the comment.
>> >> >> >
>> >> >> > Not a fan myself of any such hacks to get compilers to shut up. Is
>> GCC
>> >> >> > doing something clearly wrong here, and the false warning should be
>> >> >> > reported?
>> >> >>
>> >> >> I gave an (extremely detailed) explanation of what was going on
>> >> >> earlier in the thread:
>> >> >> https://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/180976.html.
>> >> >> The punch line is that it is not clearly wrong, and alternatives for
>> >> >> warning suppression are less clean.
>> >> >
>> >> >
>> >> > And I responded in the same thread:
>> >> > https://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/180977.html
>> >> >
>> >> > I'm still not a fan of this either. I don't think clang should be
>> doing
>> >> > what it's doing here.
>> >>
>> >> Neither am I a fan of this whole business. I try to make do with what
>> >> is feasible, clean, and easy to revert at a future stage.
>> >>
>> >> My report was with gcc, have not checked clang. Funny that both of
>> >> them try to do this.
>> >>
>> >> What alternative do you propose? Just keep the warning as it is, and
>> >> forget suppressing it?
>> >>
>> >> Also, I would like to point out the following. Just see the following
>> >> lines in avcodec/aacdec_template.c:
>> >>     /* This assignment is to silence a GCC warning about the variable
>> >> being used
>> >>      * uninitialized when in fact it always is.
>> >>      */
>> >>     pulse.num_pulse = 0;
>> >>
>> >> I have tested, and confirmed that only compilers strictly prior to GCC
>> >> 4.6 actually complain. I showed this to Michael, and he did not feel
>> >> like removing this ridiculous bit of code. It was in light of this
>> >> that I crafted a patch with a comment, and thought people would be
>> >> fine with it.
>> >
>> >
>> > Sometimes these things are just based on personal opinions. :( I
>> personally
>> > wouldn't mind removing the assignment if recent gcc versions don't
>> complain
>> > anymore.
>>
>> I guess the reason why I am keen on the movenc suppression is that I
>> highly value compiler warnings, and am trying to move FFmpeg towards a
>> clean build (I have outlined my rationale before).
>>
>> I admit, initially I was very naive: in personal projects I always
>> have -Werror, -pedantic, -Wall, -Wextra, -Wshadow and god knows what
>> :). I wondered why FFmpeg is not as liberal and why warnings are not
>> taken as seriously.
>>
>> -Warray-bounds (and the current false positives) made me realise that
>> for large projects, it is very hard to have clean builds.
>>
>> Nevertheless, I am still triving towards that goal. Why? It lets us
>> fruitfully use warnings to our advantage - currently they are drowned
>> in a sea of noise and very few hence pay attention to them.
>>
>> Let us face it: The above movenc example is likely to become a dead
>> link on the GCC bug tracker. Even more likely, GCC may claim that this
>> is not really a GCC bug. To me, in such a situation the choice is
>> clear - we suppress it. I took your point and placed a comment, so
>> that people know what we want semantically.
>>
>> It is a trivial patch and may be easily reverted, why not just add it
>> (with the comment) and remove as soon as GCC "fixes" it in some future
>> release?
>
>
> Well, so, there's various schools-of-thought on this issue. Some (as you'd
> expect) think it's not our problem and compilers should fix themselves. If
> we can easily prove that a compiler is misguided, why can't it do that
> itself?
>
> One line of code, just a few characters, seems like not such a big deal.
> But when you follow that line of thought, neither is two, or ten, and
> before you know it you end up putting in helgrind suppressions because most
> tools - as useful as they claim to be - really just suck.
>
> We'd like our source to be as pure and simple as they can be. We take that
> to the other extreme, so we take hardly any suppressions where the code is
> right.

Yes, it is really a questions of where on the ROC curve you want to
land, and as such has no technical solution. Since there seems to be
almost universal consensus against my proposal in this case, consider
the patch dropped. I may return to it in a few years :).

>
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list