[PATCH] parse_line: simplify signed byte value check
|
View:
New views
6 Messages
—
Rating Filter:
Alert me
|
 |
[PATCH] parse_line: simplify signed byte value check
by Cyrill Gorcunov
::
Rate this Message:
Please review, it seems http://repo.or.cz/ just hanging on.
No project here, all is dangling :( -- Cyrill --- From: Cyrill Gorcunov <gorcunov@...> Date: Sun, 1 Nov 2009 01:28:49 +0300 Subject: [PATCH] parse_line: simplify signed byte value check Immediate value get scanned to int64_t so if the value is in range [-128; 127] we may just set all SBYTE64|32|16 instead of converting it to int32_t and int16_t and check step-by-step. Signed-off-by: Cyrill Gorcunov <gorcunov@...> --- parser.c | 14 +++----------- 1 files changed, 3 insertions(+), 11 deletions(-) diff --git a/parser.c b/parser.c index 411d171..d6868ac 100644 --- a/parser.c +++ b/parser.c @@ -855,17 +855,9 @@ is_expression: if (optimizing >= 0 && !(result->oprs[operand].type & STRICT)) { int64_t v64 = reloc_value(value); - int32_t v32 = (int32_t)v64; - int16_t v16 = (int16_t)v32; - - if (v64 >= -128 && v64 <= 127) - result->oprs[operand].type |= SBYTE64; - if (!overflow_signed(v64, sizeof(v32))) - if (v32 >= -128 && v32 <= 127) - result->oprs[operand].type |= SBYTE32; - if (!overflow_signed(v64, sizeof(v16))) - if (v16 >= -128 && v16 <= 127) - result->oprs[operand].type |= SBYTE16; + + if (v64 >= -128 && v64 <= 127) /* we fit signed byte */ + result->oprs[operand].type |= SBYTE64 | SBYTE32 | SBYTE16; } } } else { /* it's a register */ -- 1.6.5 ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Nasm-devel mailing list Nasm-devel@... https://lists.sourceforge.net/lists/listinfo/nasm-devel |
|
|
Re: [PATCH] parse_line: simplify signed byte value check
by H. Peter Anvin
::
Rate this Message:
This one is wrong.
Consider the value 65535; this value is equivalent to -1 if encoded as a 16-bit value, and so can be SBYTE16, but it is most definitely not SBYTE32 or SBYTE64. Furthermore, even positive values can be affected: consider 65537. Although it will generate a warning, it is equivalent to 1 for 16-bit values, and so is SBYTE16, but isn't SBYTE32 or SBYTE64. The tests for overflow_signed() are wrong too. Those are relevant for if a warning should be issued, but not for how the value should be encoded in the byte stream. -hpa On 11/01/2009 07:46 AM, Cyrill Gorcunov wrote: > Please review, it seems http://repo.or.cz/ just hanging on. > No project here, all is dangling :( > > -- Cyrill > --- > From: Cyrill Gorcunov<gorcunov@...> > Date: Sun, 1 Nov 2009 01:28:49 +0300 > Subject: [PATCH] parse_line: simplify signed byte value check > > Immediate value get scanned to int64_t so if > the value is in range [-128; 127] we may > just set all SBYTE64|32|16 instead of converting > it to int32_t and int16_t and check step-by-step. > > Signed-off-by: Cyrill Gorcunov<gorcunov@...> > --- > parser.c | 14 +++----------- > 1 files changed, 3 insertions(+), 11 deletions(-) > > diff --git a/parser.c b/parser.c > index 411d171..d6868ac 100644 > --- a/parser.c > +++ b/parser.c > @@ -855,17 +855,9 @@ is_expression: > if (optimizing>= 0&& > !(result->oprs[operand].type& STRICT)) { > int64_t v64 = reloc_value(value); > - int32_t v32 = (int32_t)v64; > - int16_t v16 = (int16_t)v32; > - > - if (v64>= -128&& v64<= 127) > - result->oprs[operand].type |= SBYTE64; > - if (!overflow_signed(v64, sizeof(v32))) > - if (v32>= -128&& v32<= 127) > - result->oprs[operand].type |= SBYTE32; > - if (!overflow_signed(v64, sizeof(v16))) > - if (v16>= -128&& v16<= 127) > - result->oprs[operand].type |= SBYTE16; > + > + if (v64>= -128&& v64<= 127) /* we fit signed byte */ > + result->oprs[operand].type |= SBYTE64 | SBYTE32 | SBYTE16; > } > } > } else { /* it's a register */ ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Nasm-devel mailing list Nasm-devel@... https://lists.sourceforge.net/lists/listinfo/nasm-devel |
|
|
Re: [PATCH] parse_line: simplify signed byte value check
by Cyrill Gorcunov
::
Rate this Message:
[H. Peter Anvin - Sun, Nov 01, 2009 at 08:07:46AM +0900]
> This one is wrong. > > Consider the value 65535; this value is equivalent to -1 if encoded as a > 16-bit value, and so can be SBYTE16, but it is most definitely not > SBYTE32 or SBYTE64. Oh my! Peter, now I see what we're trying to achieve! > > Furthermore, even positive values can be affected: consider 65537. > Although it will generate a warning, it is equivalent to 1 for 16-bit > values, and so is SBYTE16, but isn't SBYTE32 or SBYTE64. This looks like a screwed approach for me. If one used 65537 in immediate field -- we should never strip off the higher bit just to fit 16 bits limit. > > The tests for overflow_signed() are wrong too. Those are relevant for > if a warning should be issued, but not for how the value should be > encoded in the byte stream. Yes :( I'll revert or fix it. Thanks a lot, Peter! > > -hpa > > > On 11/01/2009 07:46 AM, Cyrill Gorcunov wrote: >> Please review, it seems http://repo.or.cz/ just hanging on. >> No project here, all is dangling :( >> >> -- Cyrill >> --- >> From: Cyrill Gorcunov<gorcunov@...> >> Date: Sun, 1 Nov 2009 01:28:49 +0300 >> Subject: [PATCH] parse_line: simplify signed byte value check >> >> Immediate value get scanned to int64_t so if >> the value is in range [-128; 127] we may >> just set all SBYTE64|32|16 instead of converting >> it to int32_t and int16_t and check step-by-step. >> >> Signed-off-by: Cyrill Gorcunov<gorcunov@...> >> --- >> parser.c | 14 +++----------- >> 1 files changed, 3 insertions(+), 11 deletions(-) >> >> diff --git a/parser.c b/parser.c >> index 411d171..d6868ac 100644 >> --- a/parser.c >> +++ b/parser.c >> @@ -855,17 +855,9 @@ is_expression: >> if (optimizing>= 0&& >> !(result->oprs[operand].type& STRICT)) { >> int64_t v64 = reloc_value(value); >> - int32_t v32 = (int32_t)v64; >> - int16_t v16 = (int16_t)v32; >> - >> - if (v64>= -128&& v64<= 127) >> - result->oprs[operand].type |= SBYTE64; >> - if (!overflow_signed(v64, sizeof(v32))) >> - if (v32>= -128&& v32<= 127) >> - result->oprs[operand].type |= SBYTE32; >> - if (!overflow_signed(v64, sizeof(v16))) >> - if (v16>= -128&& v16<= 127) >> - result->oprs[operand].type |= SBYTE16; >> + >> + if (v64>= -128&& v64<= 127) /* we fit signed byte */ >> + result->oprs[operand].type |= SBYTE64 | SBYTE32 | SBYTE16; >> } >> } >> } else { /* it's a register */ > ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Nasm-devel mailing list Nasm-devel@... https://lists.sourceforge.net/lists/listinfo/nasm-devel |
|
|
Re: [PATCH] parse_line: simplify signed byte value check
by H. Peter Anvin
::
Rate this Message:
On 11/01/2009 04:23 PM, Cyrill Gorcunov wrote:
>> >> Furthermore, even positive values can be affected: consider 65537. >> Although it will generate a warning, it is equivalent to 1 for 16-bit >> values, and so is SBYTE16, but isn't SBYTE32 or SBYTE64. > > This looks like a screwed approach for me. If one used 65537 in immediate > field -- we should never strip off the higher bit just to fit 16 bits > limit. > No, we should, because the instruction still has the same effect. Consider that the programmer may have intentionally caused wraparound and disabled the warning. We will (or should - if we don't there is a bug) still get the warning for overflow during the code-generation phase (byte code handling), but we shouldn't worry about that during the pattern-matching phase. -hpa ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Nasm-devel mailing list Nasm-devel@... https://lists.sourceforge.net/lists/listinfo/nasm-devel |
|
|
Re: [PATCH] parse_line: simplify signed byte value check
by Cyrill Gorcunov
::
Rate this Message:
[H. Peter Anvin - Sun, Nov 01, 2009 at 05:04:18PM +0900]
> On 11/01/2009 04:23 PM, Cyrill Gorcunov wrote: >>> >>> Furthermore, even positive values can be affected: consider 65537. >>> Although it will generate a warning, it is equivalent to 1 for 16-bit >>> values, and so is SBYTE16, but isn't SBYTE32 or SBYTE64. >> >> This looks like a screwed approach for me. If one used 65537 in immediate >> field -- we should never strip off the higher bit just to fit 16 bits >> limit. >> > > No, we should, because the instruction still has the same effect. > > Consider that the programmer may have intentionally caused wraparound > and disabled the warning. > > We will (or should - if we don't there is a bug) still get the warning > for overflow during the code-generation phase (byte code handling), but > we shouldn't worry about that during the pattern-matching phase. > > -hpa OK, here is a few patches I would like to be reviewed. Thanks! --- From: Cyrill Gorcunov <gorcunov@...> Date: Sun, 1 Nov 2009 23:16:01 +0300 Subject: [PATCH 1/2] matches: simplify check operand size actions We may throw out j variable (since we break anyway) and don't assign asize for free (since we don't use it after). Signed-off-by: Cyrill Gorcunov <gorcunov@...> --- assemble.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/assemble.c b/assemble.c index 5fceea3..d792f12 100644 --- a/assemble.c +++ b/assemble.c @@ -2136,12 +2136,11 @@ static enum match_result matches(const struct itemplate *itemp, */ if (itemp->flags & (IF_SM | IF_SM2)) { oprs = (itemp->flags & IF_SM2 ? 2 : itemp->operands); - asize = 0; for (i = 0; i < oprs; i++) { - if ((asize = itemp->opd[i] & SIZE_MASK) != 0) { - int j; - for (j = 0; j < oprs; j++) - size[j] = asize; + asize = itemp->opd[i] & SIZE_MASK; + if (asize) { + for (i = 0; i < oprs; i++) + size[i] = asize; break; } } -- From: Cyrill Gorcunov <gorcunov@...> Date: Mon, 2 Nov 2009 00:15:23 +0300 Subject: [PATCH 2/2] insns.dat: fixup sbyteX usage Some sbyteX do not correlate with size of second/third operand which leads to a screwed template and code generation with -Ox passed. This should fix BR 2887108 Signed-off-by: Cyrill Gorcunov <gorcunov@...> --- insns.dat | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/insns.dat b/insns.dat index f03e5a8..2685826 100644 --- a/insns.dat +++ b/insns.dat @@ -583,19 +583,19 @@ IMUL reg16,mem,sbyte16 \320\1\x6B\110\16 186,SM,ND IMUL reg16,mem,imm16 \320\1\x69\110\32 186,SM IMUL reg16,mem,imm \320\146\x69\110\142 186,SM,ND IMUL reg16,reg16,imm8 \320\1\x6B\110\16 186 -IMUL reg16,reg16,sbyte32 \320\1\x6B\110\16 186,SM,ND +IMUL reg16,reg16,sbyte16 \320\1\x6B\110\16 186,SM,ND IMUL reg16,reg16,imm16 \320\1\x69\110\32 186 IMUL reg16,reg16,imm \320\146\x69\110\142 186,SM,ND IMUL reg32,mem,imm8 \321\1\x6B\110\16 386,SM -IMUL reg32,mem,sbyte64 \321\1\x6B\110\16 386,SM,ND +IMUL reg32,mem,sbyte32 \321\1\x6B\110\16 386,SM,ND IMUL reg32,mem,imm32 \321\1\x69\110\42 386,SM IMUL reg32,mem,imm \321\156\x69\110\152 386,SM,ND IMUL reg32,reg32,imm8 \321\1\x6B\110\16 386 -IMUL reg32,reg32,sbyte16 \321\1\x6B\110\16 386,SM,ND +IMUL reg32,reg32,sbyte32 \321\1\x6B\110\16 386,SM,ND IMUL reg32,reg32,imm32 \321\1\x69\110\42 386 IMUL reg32,reg32,imm \321\156\x69\110\152 386,SM,ND IMUL reg64,mem,imm8 \324\1\x6B\110\16 X64,SM -IMUL reg64,mem,sbyte32 \324\1\x6B\110\16 X64,SM,ND +IMUL reg64,mem,sbyte64 \324\1\x6B\110\16 X64,SM,ND IMUL reg64,mem,imm32 \324\1\x69\110\42 X64,SM IMUL reg64,mem,imm \324\156\x69\110\252 X64,SM,ND IMUL reg64,reg64,imm8 \324\1\x6B\110\16 X64 -- 1.6.5 ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Nasm-devel mailing list Nasm-devel@... https://lists.sourceforge.net/lists/listinfo/nasm-devel |
|
|
Re: [PATCH] parse_line: simplify signed byte value check
by H. Peter Anvin
::
Rate this Message:
On 11/01/2009 01:25 PM, Cyrill Gorcunov wrote:
> > OK, here is a few patches I would like to be reviewed. > Thanks! > --- > From: Cyrill Gorcunov <gorcunov@...> > Date: Sun, 1 Nov 2009 23:16:01 +0300 > Subject: [PATCH 1/2] matches: simplify check operand size actions > > We may throw out j variable (since we break anyway) > and don't assign asize for free (since we don't > use it after). > > Signed-off-by: Cyrill Gorcunov <gorcunov@...> > --- > assemble.c | 9 ++++----- > 1 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/assemble.c b/assemble.c > index 5fceea3..d792f12 100644 > --- a/assemble.c > +++ b/assemble.c > @@ -2136,12 +2136,11 @@ static enum match_result matches(const struct itemplate *itemp, > */ > if (itemp->flags & (IF_SM | IF_SM2)) { > oprs = (itemp->flags & IF_SM2 ? 2 : itemp->operands); > - asize = 0; > for (i = 0; i < oprs; i++) { > - if ((asize = itemp->opd[i] & SIZE_MASK) != 0) { > - int j; > - for (j = 0; j < oprs; j++) > - size[j] = asize; > + asize = itemp->opd[i] & SIZE_MASK; > + if (asize) { > + for (i = 0; i < oprs; i++) > + size[i] = asize; > break; > } > } Looks okay to me. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Nasm-devel mailing list Nasm-devel@... https://lists.sourceforge.net/lists/listinfo/nasm-devel |
| Free embeddable forum powered by Nabble | Forum Help |
