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: using AH in instruction with REX prefix substitutes SPL without an error #45511

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

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

TACIXAT opened this issue Apr 12, 2021 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@TACIXAT
Copy link

TACIXAT commented Apr 12, 2021 •
edited

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 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 Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 12, 2021
@mknyszek
Copy link
Contributor

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 modified the milestones: Backlog, Unplanned Apr 12, 2021
@quasilyte
Copy link
Contributor

quasilyte commented Nov 29, 2021 •
edited

@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

Dup of #14288 ?

@randall77 randall77 mentioned this issue Apr 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@TACIXAT @mknyszek @quasilyte @randall77 @cherrymui