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

Ganesh Ajjanagadde gajjanag at mit.edu
Sat Oct 17 02:45:59 CEST 2015


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?

>
> 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