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

Ronald S. Bultje rsbultje at gmail.com
Sat Oct 17 02:34:01 CEST 2015


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.

Ronald


More information about the ffmpeg-devel mailing list