code review 13241050: cmd/6l, cmd/8l: fix MOVL MOVQ optab

cmd/6l, cmd/8l: fix MOVL MOVQ optab

The entry for LEAL/LEAQ in these optabs was listed as having
two data bytes in the y array. In fact they had and expect no data
bytes. However, the general loop expects to be able to look at at
least one data byte, to make sure it is not 0x0f. So give them each
a single data byte set to 0 (not 0x0f).

Since the MOV instructions have the largest optab cases, this
requires growing the size of the data array.

Clang found this bug because the general o->op[z] == 0x0f
test was using z == 22, which was out of bounds.

In practice the next byte in memory was probably not 0x0f
so it wasn't truly broken. But might as well be clean.

Update issue 5764

[rsc, 2013-09-10 18:53:39]

Hello ken2 (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[rsc, 2013-09-10 18:53:44]

*** Submitted as https://code.google.com/p/go/source/detail?r=66a62b7700b9 ***

cmd/6l, cmd/8l: fix MOVL MOVQ optab

The entry for LEAL/LEAQ in these optabs was listed as having
two data bytes in the y array. In fact they had and expect no data
bytes. However, the general loop expects to be able to look at at
least one data byte, to make sure it is not 0x0f. So give them each
a single data byte set to 0 (not 0x0f).

Since the MOV instructions have the largest optab cases, this
requires growing the size of the data array.

Clang found this bug because the general o->op[z] == 0x0f
test was using z == 22, which was out of bounds.

In practice the next byte in memory was probably not 0x0f
so it wasn't truly broken. But might as well be clean.

Update issue 5764

R=ken2
CC=golang-dev
https://codereview.appspot.com/13241050

[dave_cheney.net, 2013-09-10 22:25:30]

Nice. I'm glad clang's pedantry found something useful. 

On 11/09/2013, at 4:53, rsc@golang.org wrote:

> Reviewers: ken2,
> 
> Message:
> Hello ken2 (cc: golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go/
> 
> 
> Description:
> cmd/6l, cmd/8l: fix MOVL MOVQ optab
> 
> The entry for LEAL/LEAQ in these optabs was listed as having
> two data bytes in the y array. In fact they had and expect no data
> bytes. However, the general loop expects to be able to look at at
> least one data byte, to make sure it is not 0x0f. So give them each
> a single data byte set to 0 (not 0x0f).
> 
> Since the MOV instructions have the largest optab cases, this
> requires growing the size of the data array.
> 
> Clang found this bug because the general o->op[z] == 0x0f
> test was using z == 22, which was out of bounds.
> 
> In practice the next byte in memory was probably not 0x0f
> so it wasn't truly broken. But might as well be clean.
> 
> Update issue 5764
> 
> Please review this at https://codereview.appspot.com/13241050/
> 
> Affected files (+19, -8 lines):
>  M src/cmd/6l/l.h
>  M src/cmd/6l/optab.c
>  M src/cmd/8l/l.h
>  M src/cmd/8l/optab.c
> 
> 
> Index: src/cmd/6l/l.h
> ===================================================================
> --- a/src/cmd/6l/l.h
> +++ b/src/cmd/6l/l.h
> @@ -193,7 +193,7 @@
>    short    as;
>    uchar*    ytab;
>    uchar    prefix;
> -    uchar    op[22];
> +    uchar    op[23];
> };
> struct    Movtab
> {
> Index: src/cmd/6l/optab.c
> ===================================================================
> --- a/src/cmd/6l/optab.c
> +++ b/src/cmd/6l/optab.c
> @@ -913,7 +913,7 @@
>    { AMOVHLPS,    yxr,    Pm, 0x12 },
>    { AMOVHPD,    yxmov,    Pe, 0x16,0x17 },
>    { AMOVHPS,    yxmov,    Pm, 0x16,0x17 },
> -    { AMOVL,    ymovl,    Px,
0x89,0x8b,0x31,0xb8,0xc7,(00),0x6e,0x7e,Pe,0x6e,Pe,0x7e },
> +    { AMOVL,    ymovl,    Px,
0x89,0x8b,0x31,0xb8,0xc7,(00),0x6e,0x7e,Pe,0x6e,Pe,0x7e,0 },
>    { AMOVLHPS,    yxr,    Pm, 0x16 },
>    { AMOVLPD,    yxmov,    Pe, 0x12,0x13 },
>    { AMOVLPS,    yxmov,    Pm, 0x12,0x13 },
> @@ -925,7 +925,7 @@
>    { AMOVNTPD,    yxr_ml,    Pe, 0x2b },
>    { AMOVNTPS,    yxr_ml,    Pm, 0x2b },
>    { AMOVNTQ,    ymr_ml,    Pm, 0xe7 },
> -    { AMOVQ,    ymovq,    Pw, 0x89, 0x8b, 0x31, 0xc7,(00), 0xb8, 0xc7,(00),
0x6f, 0x7f, 0x6e, 0x7e, Pf2,0xd6, Pf3,0x7e, Pe,0xd6, Pe,0x6e, Pe,0x7e },
> +    { AMOVQ,    ymovq,    Pw, 0x89, 0x8b, 0x31, 0xc7,(00), 0xb8, 0xc7,(00),
0x6f, 0x7f, 0x6e, 0x7e, Pf2,0xd6, Pf3,0x7e, Pe,0xd6, Pe,0x6e, Pe,0x7e,0 },
>    { AMOVQOZX,    ymrxr,    Pf3, 0xd6,0x7e },
>    { AMOVSB,    ynone,    Pb, 0xa4 },
>    { AMOVSD,    yxmov,    Pf2, 0x10,0x11 },
> @@ -935,7 +935,7 @@
>    { AMOVSW,    ynone,    Pe, 0xa5 },
>    { AMOVUPD,    yxmov,    Pe, 0x10,0x11 },
>    { AMOVUPS,    yxmov,    Pm, 0x10,0x11 },
> -    { AMOVW,    ymovw,    Pe, 0x89,0x8b,0x31,0xb8,0xc7,(00) },
> +    { AMOVW,    ymovw,    Pe, 0x89,0x8b,0x31,0xb8,0xc7,(00),0 },
>    { AMOVWLSX,    yml_rl,    Pm, 0xbf },
>    { AMOVWLZX,    yml_rl,    Pm, 0xb7 },
>    { AMOVWQSX,    yml_rl,    Pw, 0x0f,0xbf },
> Index: src/cmd/8l/l.h
> ===================================================================
> --- a/src/cmd/8l/l.h
> +++ b/src/cmd/8l/l.h
> @@ -175,7 +175,7 @@
>    short    as;
>    uchar*    ytab;
>    uchar    prefix;
> -    uchar    op[12];
> +    uchar    op[13];
> };
> 
> enum
> Index: src/cmd/8l/optab.c
> ===================================================================
> --- a/src/cmd/8l/optab.c
> +++ b/src/cmd/8l/optab.c
> @@ -152,6 +152,17 @@
>    Yi32,    Ymb,    Zibo_m,    2,
>    0
> };
> +uchar    ymovw[] =
> +{
> +    Yrl,    Yml,    Zr_m,    1,
> +    Yml,    Yrl,    Zm_r,    1,
> +    Yi0,    Yrl,    Zclr,    1+2,
> +//    Yi0,    Yml,    Zibo_m,    2,    // shorter but slower AND $0,dst
> +    Yi32,    Yrl,    Zil_rp,    1,
> +    Yi32,    Yml,    Zilo_m,    2,
> +    Yiauto,    Yrl,    Zaut_r,    1,
> +    0
> +};
> uchar    ymovl[] =
> {
>    Yrl,    Yml,    Zr_m,    1,
> @@ -162,7 +173,7 @@
>    Yi32,    Yml,    Zilo_m,    2,
>    Yml,    Yxr,    Zm_r_xm,    2,    // XMM MOVD (32 bit)
>    Yxr,    Yml,    Zr_m_xm,    2,    // XMM MOVD (32 bit)
> -    Yiauto,    Yrl,    Zaut_r,    2,
> +    Yiauto,    Yrl,    Zaut_r,    1,
>    0
> };
> uchar    ymovq[] =
> @@ -592,8 +603,8 @@
>    { ALSLL,    yml_rl,    Pm, 0x03  },
>    { ALSLW,    yml_rl,    Pq, 0x03  },
>    { AMOVB,    ymovb,    Pb, 0x88,0x8a,0xb0,0xc6,(00) },
> -    { AMOVL,    ymovl,    Px,
0x89,0x8b,0x31,0x83,(04),0xb8,0xc7,(00),Pe,0x6e,Pe,0x7e },
> -    { AMOVW,    ymovl,    Pe, 0x89,0x8b,0x31,0x83,(04),0xb8,0xc7,(00) },
> +    { AMOVL,    ymovl,    Px,
0x89,0x8b,0x31,0x83,(04),0xb8,0xc7,(00),Pe,0x6e,Pe,0x7e,0 },
> +    { AMOVW,    ymovw,    Pe, 0x89,0x8b,0x31,0x83,(04),0xb8,0xc7,(00),0 },
>    { AMOVQ,    ymovq,    Pf3, 0x7e },
>    { AMOVBLSX,    ymb_rl,    Pm, 0xbe },
>    { AMOVBLZX,    ymb_rl,    Pm, 0xb6 },
> 
> 
> -- 
> 
> ---You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.