New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Quasilyte opened this Issue Jan 9, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@Quasilyte
Contributor

Quasilyte commented Jan 9, 2018

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?

  1. Assemble this file by go tool asm fsave.s:
TEXT main·fsave(SB),0,$0
        FSAVE (AX)
        RET
  1. 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			
  1. 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

This comment has been minimized.

Contributor

Quasilyte commented Jan 9, 2018

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

This comment has been minimized.

Contributor

dgryski commented Jan 10, 2018

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

@Quasilyte

This comment has been minimized.

Contributor

Quasilyte commented Jan 10, 2018

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

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 28, 2018

@Quasilyte

This comment has been minimized.

Contributor

Quasilyte commented May 21, 2018

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

This comment has been minimized.

Contributor

Quasilyte commented May 21, 2018

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

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 23, 2018

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 23, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Dec 7, 2018

@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.)

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Go1.13 Dec 7, 2018

@TocarIP

This comment has been minimized.

Contributor

TocarIP commented Dec 7, 2018

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

This comment has been minimized.

gopherbot commented Dec 7, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment