cmd/internal/obj/x86: FSAVE assembled as FNSAVE

quasilyte commented 6 years ago

What version of Go are you using (`go version`)?

go version go1.9.2 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (`go env`)?

GOARCH="amd64" GOBIN="" GOEXE="" GOHOSTARCH="amd64" GOHOSTOS="linux" GOOS="linux" GOPATH="/home/quasilyte/CODE/intel/avxgen" GORACE="" GOROOT="/usr/local/go" GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64" GCCGO="gccgo" CC="gcc" GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build350631872=/tmp/go-build -gno-record-gcc-switches" CXX="g++" CGO_ENABLED="1" CGO_CFLAGS="-g -O2" CGO_CPPFLAGS="" CGO_CXXFLAGS="-g -O2" CGO_FFLAGS="-g -O2" CGO_LDFLAGS="-g -O2" PKG_CONFIG="pkg-config"

What did you do?

Assemble this file by go tool asm fsave.s:

TEXT main·fsave(SB),0,$0
    FSAVE (AX)
    RET

Check output by go tool objdump fsave.o

TEXT main.fsave(SB) gofile../home/quasilyte/CODE/asm/fsave.s
fsave.s:2     0x94            dd30            FNSAVE 0(AX)        
fsave.s:3     0x96            c3          RET

Note that disassembler prints "FNSAVE" (it's correct). You may also check it with external disassemblers like binutils objdump and Intel XED.

$ ./obj/examples/xed -64 -d dd30
DD30
ICLASS: FNSAVE   CATEGORY: X87_ALU   EXTENSION: X87  IFORM: FNSAVE_MEMmem108   ISA_SET: X87
SHORT: fnsave ptr [rax]

What did you expect to see?

FSAVE instruction is assembled into FSAVE. Result encoding is "9bdd30".

What did you see instead?

FSAVE instruction is assembled into FNSAVE. Result encoding is "dd30".

Additional notes

It's not only FSAVE/FNSAVE, there are more such instructions. I can provide more-or-less complete list later, potentially in a separate issue.
It's impossible to select datasize as there is no FNSAVEL/FNSAVES variants, only FNSAVE (which is FSAVE right now). x86csv makes me believe that Go asm should have either suffixed version to make datasize selection possible.

$ cat src/x86/x86.csv | egrep 'fnsave|fsave'
"FNSAVE m94/108byte","FNSAVES/FNSAVEL m94/108byte","fnsaves/fnsavel m94/108byte","DD /6","V","V","","","w","",""
"FSAVE m94/108byte","FSAVE m94/108byte","fsave m94/108byte","9B DD /6","V","V","","pseudo","w","",""

I can send a CL that fixes this. The only trouble is backwards-compatibility: if FSAVE becomes FNSAVE, what to do with existing code?

Note that FSAVE is really a combination of FWAIT and FNSAVE.

quasilyte commented 6 years ago

If this issue is never fixed, here is advice for programmers from the future: use FWAIT+FSAVE to get proper FSAVE.

And if FWAIT is not implemented at the moment you read this, try:

        BYTE $0x9b // FWAIT
        FSAVE (AX)

dgryski commented 6 years ago

I wonder if it would be worth fuzzing the assembler with valid but odd instructions to find any other mismatches.

quasilyte commented 6 years ago

I may be wrong (not fuzzing expert), but maybe validation against external assemblers/disassemblers is more appropriate?

I did some of it using XED for AVX2 and above (including AVX512 with most of it's extensions), so only legacy instructions need heavy testing.

Updated x86.csv can be useful as a foundation for tests code generation (it was used for existing amd64enc.s suite and disassembler "map"). Not sure it can be merged until code freeze is gone though.

In theory, when x86.csv is updated, we need to re-generate disassembler from it and check the assembler by tests based on it (this part is mostly done, but there is a room for improvements).

But anyway, I think fuzzing can still find places where x86 asm crashes.. It can be more overall useful to run next fuzzing after AVX512 is implemented, which will open whole new horizons for input corpus diversity (suffixes, register ranges, new registers and different internal changes like updated opcode lookup).

(By the way, there are still open issues from previous fuzz attack.)

quasilyte commented 6 years ago

More complete list of instructions that assembled in incorrect way:

FCLEX   = FWAIT + FNCLEX
FINIT   = FWAIT + FNINI
FSAVES  = FWAIT + FNSAVES
FSAVEL  = FWAIT + FNSAVEL (L size suffix is optional)
FSTCW   = FWAIT + FNSTCW
FSTENVS = FWAIT + FNSTENVS
FSTENVL = FWAIT + FNSTENVL (L size suffix is optional)
FSTSW   = FWAIT + FNSTSW

Since current uses of all instructions from the left imply FWAIT (WAIT), but in reality they assembled into forms without WAIT, it could be acceptable to forbid "pseudo" WAIT+op forms and add instructions without WAIT, so programmer can use both forms. This way, invalid code will not be assembled anymore. Probably can be fixed automatically with go fix.

The other way is to change FSAVE to proper encoding, but that will change code behavior silently.

We're also missing operand size override for FSAVE and FSTENV. From gas:

        FSTENVS (%rax)
        FSTENV (%rax)
        FSTENVL (%rax)
=>
   0:   66 d9 30                fnstenvs (%rax)
   3:   d9 30                   fnstenv (%rax)
   5:   d9 30                   fnstenv (%rax)

My proposed solution is described above, but I can't make the final decision here.

quasilyte commented 6 years ago

CC @TocarIP (not urgent, adding to copy just in case)

ianlancetaylor commented 6 years ago

CC @cherrymui

ianlancetaylor commented 5 years ago

@TocarIP What do you think of the suggestion above?

(Personally I'm not worried about backward compatibility for these instructions, and I think it would be OK to just fix them. But I also don't know much about this.)

TocarIP commented 5 years ago

I'm Ok with fixing FSAVE. Quick search shows that it isn't encountered at all in github data-set (https://cloud.google.com/bigquery/public-data/), so breakage would be minimal.

gopherbot commented 5 years ago

Change https://golang.org/cl/153217 mentions this issue: cmd/asm,runtime,math: rename x87 insts without FWAIT

golang / go