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

cmd/internal/obj/x86: using AH in instruction with REX prefix substitutes SPL without an error #45511

Open
TACIXAT opened this issue Apr 12, 2021 · 4 comments
Labels
NeedsInvestigation
Milestone

Comments

@TACIXAT
Copy link

@TACIXAT TACIXAT commented Apr 12, 2021

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

$ go version
go version go1.15.8 windows/amd64

Does this issue reproduce with the latest release?

Didn't check.

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

windows, amd64

What did you do?

Accessing obj via package here.

Repro -

package main

import (
	asm "github.com/twitchyliquid64/golang-asm"
	"github.com/twitchyliquid64/golang-asm/obj"
	"github.com/twitchyliquid64/golang-asm/obj/x86"
	"log"
)

func main() {
	b, err := asm.NewBuilder("amd64", 64)
	if err != nil {
		log.Fatal(err)
	}

	p := b.NewProg()
	p.As = x86.AMOVB
	p.To.Type = obj.TYPE_REG
	p.To.Reg = x86.REG_AH
	p.From.Type = obj.TYPE_MEM
	p.From.Reg = x86.REG_R13
	p.From.Offset = 2 * 8

	b.AddInstruction(p)

	log.Printf("%x", b.Assemble())
}
λ go run assemble.go
2021/04/11 18:21:36 418a6510

What did you expect to see?

An error.

What did you see instead?

0:  41 8a 65 10             mov    spl,BYTE PTR [r13+0x10] 
@TACIXAT
Copy link
Author

@TACIXAT TACIXAT commented Apr 12, 2021

Would be happy to help fix this if anyone would like to give pointers of where to look. I see the optab and ymovb. Not sure where these registers translate though.

@mknyszek mknyszek changed the title obj/x86 - Using AH in instruction with REX prefix substitutes SPL without an error cmd/internal/obj/x86: using AH in instruction with REX prefix substitutes SPL without an error Apr 12, 2021
@mknyszek mknyszek added this to the Backlog milestone Apr 12, 2021
@mknyszek mknyszek added the NeedsInvestigation label Apr 12, 2021
@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Apr 12, 2021

CC @randall77 @dr2chase @cherrymui @martisch @josharian via https://dev.golang.org/owners (I used cmd/compile/x86 which doesn't seem to exist anymore... that needs an update, probably, post-refactoring.)

@cherrymui cherrymui removed this from the Backlog milestone Apr 12, 2021
@cherrymui cherrymui added this to the Unplanned milestone Apr 12, 2021
@quasilyte
Copy link
Contributor

@quasilyte quasilyte commented Nov 29, 2021

@TACIXAT, ytab is like a signature, it describes the set of operands the instruction can use.

If you want to restrict the instruction arguments, you need to use a Y-constant (like Yi8 is for int8) that includes everything that is legal (GPR registers minus AH?). I have a hunch that we already have that operand class, but it probably has some cryptic name.

All in all, you need to:

  1. Locate the AMOVB optab (asm6.go)
  2. See which ytab it's using
  3. If it's the sole user of that ytab, fix the ytab object itself. Otherwise, create a new ytab and use it instead.
  4. Add a test to $GOROOT/src/cmd/asm/internal/asm/testdata/amd64error.s

I would actually start from (4) so you have a failing test to begin with. Then you try to make the tests pass. You don't need to rebuild the entire toolchain to test a new assembler, it's probably enough to do a go install cmd/asm and then run the tests like go test cmd/asm/internal/asm.

IIRC, REX prefix can be validated for some Y operand classes in ad-hoc way, but you're better off reading the code closely (maybe start by looking at rexflag usages?).

My memories are quite hazy on this, but I could make a partial review if you send a CL (I can't do +2, but at least I can ease the review for the maintainers).

@randall77
Copy link
Contributor

@randall77 randall77 commented Nov 29, 2021

Dup of #14288 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

5 participants