Skip to content
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

Jump to bottom

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

Open
quasilyte opened this issue Jan 9, 2018 · 9 comments
Open

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

quasilyte opened this issue Jan 9, 2018 · 9 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@quasilyte
Copy link
Contributor

quasilyte commented Jan 9, 2018 •
edited

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
Copy link
Contributor Author

quasilyte commented Jan 9, 2018 •
edited

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
Copy link
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
Copy link
Contributor Author

quasilyte commented Jan 10, 2018 •
edited

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 the NeedsFix The path to resolution is known, but the work has not been done. label Mar 28, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 28, 2018
@quasilyte
Copy link
Contributor Author

quasilyte commented May 21, 2018 •
edited

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
Copy link
Contributor Author

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

@mmcloughlin mmcloughlin mentioned this issue May 29, 2018
Open
@ianlancetaylor
Copy link
Contributor

CC @cherrymui

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

@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
Copy link
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
Copy link

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

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@rsc @andybons @dgryski @ianlancetaylor @quasilyte @gopherbot @TocarIP