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

[Ganesh Ajjanagadde]

On Mon, Oct 12, 2015 at 12:00 PM, Ganesh Ajjanagadde  wrote:
> On Mon, Oct 12, 2015 at 11:47 AM, Michael Niedermayer
>  wrote:
>> On Mon, Oct 12, 2015 at 11:16:00AM -0400, Ganesh Ajjanagadde wrote:
>>> On Mon, Oct 12, 2015 at 10:54 AM, Ronald S. Bultje  
>>> wrote:
>>> > Hi,
>>> >
>>> > On Mon, Oct 12, 2015 at 10:51 AM, Ganesh Ajjanagadde 
>>> > wrote:
>>> >>
>>> >> On Thu, Oct 8, 2015 at 5:07 PM, Ganesh Ajjanagadde 
>>> >> wrote:
>>> >> > On Sat, Oct 3, 2015 at 8:17 AM, Ganesh Ajjanagadde 
>>> >> > wrote:
>>> >> >> On Tue, Sep 29, 2015 at 10:49 AM, Ganesh Ajjanagadde 
>>> >> >> 
>>> >> >> wrote:
>>> >> >>> On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde 
>>> >> >>> 
>>> >> >>> wrote:
>>> >>  On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer
>>> >>   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
>>> >> >>  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
>>> >> >> >>  wrote:
>>> >> >> >> > Hi,
>>> >> >> >> >
>>> >> >> >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde
>>> >> >> >> > 
>>> >> >> >> > wrote:
>>> >> >> >> >
>>> >> >> >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer
>>> >> >> >> >> 
>>> >> >> >> >> 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 
>>> >> >> >> >> >> 
>>> >> >> >> >> >> ---
>>> >> >> >> >> >>  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 

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

[wm4]

On Mon, 12 Oct 2015 11:16:00 -0400
Ganesh Ajjanagadde  wrote:

> On Mon, Oct 12, 2015 at 10:54 AM, Ronald S. Bultje  wrote:
> > Hi,
> >
> > On Mon, Oct 12, 2015 at 10:51 AM, Ganesh Ajjanagadde 
> > wrote:
> >>
> >> On Thu, Oct 8, 2015 at 5:07 PM, Ganesh Ajjanagadde 
> >> wrote:
> >> > On Sat, Oct 3, 2015 at 8:17 AM, Ganesh Ajjanagadde 
> >> > wrote:
> >> >> On Tue, Sep 29, 2015 at 10:49 AM, Ganesh Ajjanagadde 
> >> >> wrote:
> >> >>> On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde 
> >> >>> wrote:
> >>  On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer
> >>   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
> >> >>  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
> >> >> >>  wrote:
> >> >> >> > Hi,
> >> >> >> >
> >> >> >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde
> >> >> >> > 
> >> >> >> > wrote:
> >> >> >> >
> >> >> >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer
> >> >> >> >> 
> >> >> >> >> 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 
> >> >> >> >> >> ---
> >> >> >> >> >>  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 

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

[Michael Niedermayer]

On Mon, Oct 12, 2015 at 11:16:00AM -0400, Ganesh Ajjanagadde wrote:
> On Mon, Oct 12, 2015 at 10:54 AM, Ronald S. Bultje  wrote:
> > Hi,
> >
> > On Mon, Oct 12, 2015 at 10:51 AM, Ganesh Ajjanagadde 
> > wrote:
> >>
> >> On Thu, Oct 8, 2015 at 5:07 PM, Ganesh Ajjanagadde 
> >> wrote:
> >> > On Sat, Oct 3, 2015 at 8:17 AM, Ganesh Ajjanagadde 
> >> > wrote:
> >> >> On Tue, Sep 29, 2015 at 10:49 AM, Ganesh Ajjanagadde 
> >> >> wrote:
> >> >>> On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde 
> >> >>> wrote:
> >>  On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer
> >>   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
> >> >>  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
> >> >> >>  wrote:
> >> >> >> > Hi,
> >> >> >> >
> >> >> >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde
> >> >> >> > 
> >> >> >> > wrote:
> >> >> >> >
> >> >> >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer
> >> >> >> >> 
> >> >> >> >> 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 
> >> >> >> >> >> ---
> >> >> >> >> >>  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 

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

[Ganesh Ajjanagadde]

On Mon, Oct 12, 2015 at 11:47 AM, Michael Niedermayer
 wrote:
> On Mon, Oct 12, 2015 at 11:16:00AM -0400, Ganesh Ajjanagadde wrote:
>> On Mon, Oct 12, 2015 at 10:54 AM, Ronald S. Bultje  
>> wrote:
>> > Hi,
>> >
>> > On Mon, Oct 12, 2015 at 10:51 AM, Ganesh Ajjanagadde 
>> > wrote:
>> >>
>> >> On Thu, Oct 8, 2015 at 5:07 PM, Ganesh Ajjanagadde 
>> >> wrote:
>> >> > On Sat, Oct 3, 2015 at 8:17 AM, Ganesh Ajjanagadde 
>> >> > wrote:
>> >> >> On Tue, Sep 29, 2015 at 10:49 AM, Ganesh Ajjanagadde 
>> >> >> wrote:
>> >> >>> On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde 
>> >> >>> wrote:
>> >>  On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer
>> >>   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
>> >> >>  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
>> >> >> >>  wrote:
>> >> >> >> > Hi,
>> >> >> >> >
>> >> >> >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde
>> >> >> >> > 
>> >> >> >> > wrote:
>> >> >> >> >
>> >> >> >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer
>> >> >> >> >> 
>> >> >> >> >> 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 
>> >> >> >> >> >> ---
>> >> >> >> >> >>  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
>> >> >>
>> >> >>
>> >> >> 

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

[Ronald S. Bultje]

Hi,

On Mon, Oct 12, 2015 at 10:51 AM, Ganesh Ajjanagadde 
wrote:

> On Thu, Oct 8, 2015 at 5:07 PM, Ganesh Ajjanagadde 
> wrote:
> > On Sat, Oct 3, 2015 at 8:17 AM, Ganesh Ajjanagadde 
> wrote:
> >> On Tue, Sep 29, 2015 at 10:49 AM, Ganesh Ajjanagadde 
> wrote:
> >>> On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde 
> 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 
> >> >> >> >> ---
> >> >> >> >>  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
> 

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

[Ganesh Ajjanagadde]

On Sat, Oct 3, 2015 at 8:17 AM, Ganesh Ajjanagadde  wrote:
> On Tue, Sep 29, 2015 at 10:49 AM, Ganesh Ajjanagadde  wrote:
>> On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde  wrote:
>>> On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer  
>>> 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  
> 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 
> >>  wrote:
> >> > Hi,
> >> >
> >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde 
> >> > 
> >> > wrote:
> >> >
> >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer 
> >> >> 
> >> >> 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 
> >> >> >> ---
> >> >> >>  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 ?
>>>

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

[Ganesh Ajjanagadde]

On Tue, Sep 29, 2015 at 10:49 AM, Ganesh Ajjanagadde  wrote:
> On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde  wrote:
>> On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer  
>> 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  
 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  
 >> wrote:
 >> > Hi,
 >> >
 >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde 
 >> > wrote:
 >> >
 >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer 
 >> >> 
 >> >> 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 
 >> >> >> ---
 >> >> >>  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 

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

[Ganesh Ajjanagadde]

On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde  wrote:
> On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer  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  
>>> 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  
>>> >> wrote:
>>> >> > Hi,
>>> >> >
>>> >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde 
>>> >> > wrote:
>>> >> >
>>> >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer 
>>> >> >> 
>>> >> >> 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 
>>> >> >> >> ---
>>> >> >> >>  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 

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

[Michael Niedermayer]

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  wrote:
> > Hi,
> >
> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde 
> > wrote:
> >
> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer 
> >> 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 
> >> >> ---
> >> >>  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

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

On Sun, Sep 27, 2015 at 12:58 PM, Michael Niedermayer  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  
>> wrote:
>> > Hi,
>> >
>> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde 
>> > wrote:
>> >
>> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer 
>> >> 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 
>> >> >> ---
>> >> >>  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.
2. local enable/disable: too ugly and overkill when we need only 2
lines to be changed in trivial ways throughout the codebase: movenc
and xface. IMO, we should consider this only if we run into more false
positives.

>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I know you won't believe me, but the highest form of Human Excellence is
> to question oneself and others. -- Socrates
>
> ___
> 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

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

[Michael Niedermayer]

On Sun, Sep 27, 2015 at 01:23:03PM -0400, Ganesh Ajjanagadde wrote:
> On Sun, Sep 27, 2015 at 12:58 PM, Michael Niedermayer  
> 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  
> >> wrote:
> >> > Hi,
> >> >
> >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde 
> >> > wrote:
> >> >
> >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer 
> >> >> 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 
> >> >> >> ---
> >> >> >>  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 ?

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 3
"Rare item" - "Common item with rare defect or maybe just a lie"
"Professional" - "'Toy' made in china, not functional except as doorstop"
"Experts will know" - "The seller hopes you are not an expert"


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer  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  
>> 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  
>> >> wrote:
>> >> > Hi,
>> >> >
>> >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde 
>> >> > wrote:
>> >> >
>> >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer 
>> >> >> 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 
>> >> >> >> ---
>> >> >> >>  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.

>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The real ebay dictionary, page 3
> "Rare item" - "Common item with rare defect or maybe just a lie"
> "Professional" - "'Toy' made in china, not functional except as doorstop"
> "Experts will know" - "The seller hopes you are not an expert"
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

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

[Ganesh Ajjanagadde]

On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer  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 
>> ---
>>  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.

>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> No snowflake in an avalanche ever feels responsible. -- Voltaire
>
> ___
> 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

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

[Michael Niedermayer]

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

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ronald S. Bultje]

Hi,

On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde 
wrote:

> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer 
> 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 
> >> ---
> >>  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?

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

On Sat, Sep 26, 2015 at 10:55 PM, Ganesh Ajjanagadde  wrote:
> On Sat, Sep 26, 2015 at 10:32 PM, Ronald S. Bultje  wrote:
>> Hi,
>>
>> On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde 
>> wrote:
>>
>>> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer 
>>> 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 
>>> >> ---
>>> >>  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).
>
> What I have done is manually audit this code to ensure this
> transformation is safe, and rewritten the expression to match what the
> compiler anyway transforms it into. This thereby suppresses the
> warning.

Just checked: it is something triggered at -O3 but not at -O2.
However, this does not change the key points made above.

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

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

[Ganesh Ajjanagadde]

On Sat, Sep 26, 2015 at 10:32 PM, Ronald S. Bultje  wrote:
> Hi,
>
> On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde 
> wrote:
>
>> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer 
>> 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 
>> >> ---
>> >>  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).

What I have done is manually audit this code to ensure this
transformation is safe, and rewritten the expression to match what the
compiler anyway transforms it into. This thereby suppresses the
warning.

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

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

[Ganesh Ajjanagadde]

On Fri, Sep 18, 2015 at 5:15 PM, 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 
> ---
>  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)
>  return 0;
>
>  if (cluster_idx + 1 == track->entry)
> --
> 2.5.2
>

ping
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

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 
---
 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)
 return 0;
 
 if (cluster_idx + 1 == track->entry)
-- 
2.5.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel