diff mbox

[FFmpeg-devel,3/3] avcodec/x86/lossless_videoencdsp: Fix warning: signed dword value exceeds bounds

Message ID 20170929225829.2890-3-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Sept. 29, 2017, 10:58 p.m. UTC
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/x86/lossless_videoencdsp.asm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ronald S. Bultje Sept. 30, 2017, 12:50 p.m. UTC | #1
Hi,

On Fri, Sep 29, 2017 at 6:58 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/x86/lossless_videoencdsp.asm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/x86/lossless_videoencdsp.asm
> b/libavcodec/x86/lossless_videoencdsp.asm
> index a9c7a0a73c..89f20dc4e2 100644
> --- a/libavcodec/x86/lossless_videoencdsp.asm
> +++ b/libavcodec/x86/lossless_videoencdsp.asm
> @@ -45,7 +45,7 @@ cglobal diff_bytes, 4,5,2, dst, src1, src2, w
>  ; labels to jump to if w < regsize and w < 0
>  %macro DIFF_BYTES_LOOP_PREP 2
>      mov                i, wq
> -    and                i, -2 * regsize
> +    and                i, -(2 * regsize)
>          js            %2
>          jz            %1
>      add             dstq, i
> --
> 2.14.2


OK.

(Is this a particular version of an assembler and maybe something that
should be reported upstream?)

Ronald
Michael Niedermayer Sept. 30, 2017, 10:17 p.m. UTC | #2
On Sat, Sep 30, 2017 at 08:50:57AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, Sep 29, 2017 at 6:58 PM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:
> 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/x86/lossless_videoencdsp.asm | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/x86/lossless_videoencdsp.asm
> > b/libavcodec/x86/lossless_videoencdsp.asm
> > index a9c7a0a73c..89f20dc4e2 100644
> > --- a/libavcodec/x86/lossless_videoencdsp.asm
> > +++ b/libavcodec/x86/lossless_videoencdsp.asm
> > @@ -45,7 +45,7 @@ cglobal diff_bytes, 4,5,2, dst, src1, src2, w
> >  ; labels to jump to if w < regsize and w < 0
> >  %macro DIFF_BYTES_LOOP_PREP 2
> >      mov                i, wq
> > -    and                i, -2 * regsize
> > +    and                i, -(2 * regsize)
> >          js            %2
> >          jz            %1
> >      add             dstq, i
> > --
> > 2.14.2
> 
> 
> OK.
> 
> (Is this a particular version of an assembler and maybe something that
> should be reported upstream?)

this report from 2011 sounds similar:
https://bugzilla.nasm.us/show_bug.cgi?id=3311133
the issue is still open


NASM version 2.10.09 compiled on Dec 29 2013
and
NASM version 2.11.08
are both affected


[...]
Henrik Gramner Oct. 1, 2017, 12:10 a.m. UTC | #3
On Sat, Sep 30, 2017 at 12:58 AM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> -    and                i, -2 * regsize
> +    and                i, -(2 * regsize)

regsize is defined to mmsize / 2 in the relevant case so the
expression resolves to -2 * 16 / 2

In nasm integers are 64-bit and / is unsigned division (// is signed
division). When you perform unsigned division on a negative number you
get a large number that doesn't fit inside a 32-bit immediate. E.g.
the warning is working as intended.

A better fix is to either use parentheses in the regsize definition
(e.g. what you'd normally do in an equivalent C define), or use
%assign instead of %define. Using // is kind of ugly so I rather avoid
that.
Michael Niedermayer Oct. 4, 2017, 10:50 p.m. UTC | #4
On Sun, Oct 01, 2017 at 02:10:41AM +0200, Henrik Gramner wrote:
> On Sat, Sep 30, 2017 at 12:58 AM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> > -    and                i, -2 * regsize
> > +    and                i, -(2 * regsize)
> 
> regsize is defined to mmsize / 2 in the relevant case so the
> expression resolves to -2 * 16 / 2
> 
> In nasm integers are 64-bit and / is unsigned division (// is signed
> division). When you perform unsigned division on a negative number you
> get a large number that doesn't fit inside a 32-bit immediate. E.g.
> the warning is working as intended.
> 

> A better fix is to either use parentheses in the regsize definition
> (e.g. what you'd normally do in an equivalent C define), or use

yes, thats better, ill push a fix with that

thanks


> %assign instead of %define. Using // is kind of ugly so I rather avoid
> that.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavcodec/x86/lossless_videoencdsp.asm b/libavcodec/x86/lossless_videoencdsp.asm
index a9c7a0a73c..89f20dc4e2 100644
--- a/libavcodec/x86/lossless_videoencdsp.asm
+++ b/libavcodec/x86/lossless_videoencdsp.asm
@@ -45,7 +45,7 @@  cglobal diff_bytes, 4,5,2, dst, src1, src2, w
 ; labels to jump to if w < regsize and w < 0
 %macro DIFF_BYTES_LOOP_PREP 2
     mov                i, wq
-    and                i, -2 * regsize
+    and                i, -(2 * regsize)
         js            %2
         jz            %1
     add             dstq, i