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.

Sign up for GitHub

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/objdump: bad disassembly for 8-bit operations #15595

Closed
randall77 opened this issue May 7, 2016 · 3 comments
Closed

cmd/objdump: bad disassembly for 8-bit operations #15595

randall77 opened this issue May 7, 2016 · 3 comments
Labels
FrozenDueToAge help wanted
Milestone
Go1.8

Comments

@randall77
Copy link
Contributor

The disassembly for some 8-bit operations is wrong. (The code is correct, just the disassembler is wrong.)

Test file:

package main

//go:noinline
func f(a, b uint8) uint8 {
    return a + b
}
func main() {
    println(f(3, 5))
}

Then run:

go build tmp.go
go tool objdump tmp

gives:

TEXT main.f(SB) /Users/khr/go/tmp.go
        tmp.go:5       0x2040  0fb6442409      MOVZX 0x9(SP), AX       
        tmp.go:5       0x2045  0fb64c2408      MOVZX 0x8(SP), CX       
        tmp.go:5       0x204a  01c8            ADDL CX, AX             
        tmp.go:5       0x204c  88442410        MOVL AL, 0x10(SP)       
        tmp.go:5       0x2050  c3              RET

The MOVL at the end should be a MOVB. Notice that the register is named AL (correctly? AX would also a fine name there).

The bug doesn't happen for 16-bit stores, it generates MOVW AX, 0x10(SP) correctly.
@randall77 randall77 added this to the Go1.8 milestone May 7, 2016
@feiling
Copy link

feiling commented May 23, 2016

I looked into the problem a little bit and this is what I found:

  • The instructions are decoded by function decode1() in file src/cmd/internal/unvendor/golang.org/x/arch/x86/x86asm/decode.go. It returns one decoded instruction, whose type is Inst. The type contains members Op, which is the opcode mnemonic, and DataSize, which is the size of the operand.
  • For opcode 0x88, it's decoded into a Inst whose Op is MOV, and whose DataSize is 32. When printed out, it's shown as "MOVL". Of course the correct DataSize in this case should be 8, not 32. In decoder1(), during decoding, the size of the operand is calculated and saved into variable dataMode. But searching the code, you can see dataMode is never assigned 8.
  • For some x86 opcodes, the operand size is always 8-bit. For example, in file https://github.com/rsc/x86/blob/master/x86.csv, you can see opcode 0x88 matches instruction "MOV r/m8, r8", whose operand size is always 8-bit.
  • decoder1() uses tables defined in tables.go to decode x86 instructions. A simple fix to the bug can be:
    • Change https://github.com/rsc/x86/blob/master/x86map/map.go so that it generates a map containing all opcodes whose operand size is always 8-bit.
    • Change decoder1() so that inst.DataSize is set to 8 when the opcode is contained in the map. Otherwise, inst.DataSize is calculated the same way as today.

@appleby
Copy link
Contributor

appleby commented Sep 7, 2016

I am not able to reproduce this on either linux/amd64 or linux/386 using go1.7 (or tip). I was able to repro using go1.6.3.

Here is the output of my various attempts in case I've missed something. Note that go1.6.3 decodes the instruction as MOVL, whereas go1.7 and tip both decode as MOVB.

linux/amd64

go version go1.6.3 linux/amd64
TEXT main.f(SB) /tmp/objdump/tmp.go
        tmp.go:5        0x401000        0fb65c2408      MOVZX 0x8(SP), BX
        tmp.go:5        0x401005        0fb66c2409      MOVZX 0x9(SP), BP
        tmp.go:5        0x40100a        4801eb          ADDQ BP, BX
        tmp.go:5        0x40100d        885c2410        MOVL BL, 0x10(SP)
        tmp.go:5        0x401011        c3              RET
        tmp.go:5        0x401012        cc              INT $0x3


go version go1.7 linux/amd64
TEXT main.f(SB) /tmp/objdump/tmp.go
        tmp.go:5        0x401000        0fb6442409      MOVZX 0x9(SP), AX
        tmp.go:5        0x401005        0fb64c2408      MOVZX 0x8(SP), CX
        tmp.go:5        0x40100a        01c8            ADDL CX, AX
        tmp.go:5        0x40100c        88442410        MOVB AL, 0x10(SP)
        tmp.go:5        0x401010        c3              RET


go version devel +8259cf3 Tue Sep 6 21:05:58 2016 +0000 linux/amd64
TEXT main.f(SB) /tmp/objdump/tmp.go
        tmp.go:5        0x401000        0fb6442409      MOVZX 0x9(SP), AX
        tmp.go:5        0x401005        0fb64c2408      MOVZX 0x8(SP), CX
        tmp.go:5        0x40100a        01c8            ADDL CX, AX
        tmp.go:5        0x40100c        88442410        MOVB AL, 0x10(SP)
        tmp.go:5        0x401010        c3              RET

linux/386

go version go1.6.3 linux/386
TEXT main.f(SB) /home/vagrant/objdump/tmp.go
        tmp.go:4        0x8049000       658b0d00000000  GS MOVL GS:0(IP), CX
        tmp.go:4        0x8049007       8b89fcffffff    MOVL 0xfffffffc(CX), CX
        tmp.go:4        0x804900d       3b6108          CMPL 0x8(CX), SP
        tmp.go:4        0x8049010       760e            JBE 0x8049020
        tmp.go:5        0x8049012       0fb65c2404      MOVZX 0x4(SP), BX
        tmp.go:5        0x8049017       025c2405        ADDL 0x5(SP), BL
        tmp.go:5        0x804901b       885c2408        MOVL BL, 0x8(SP)
        tmp.go:5        0x804901f       c3              RET
        tmp.go:4        0x8049020       e85b0f0400      CALL runtime.morestack_noctxt(SB)


go version go1.7 linux/386
TEXT main.f(SB) /home/vagrant/objdump/tmp.go
        tmp.go:4        0x8049000       658b0d00000000  GS MOVL GS:0(IP), CX
        tmp.go:4        0x8049007       8b89fcffffff    MOVL 0xfffffffc(CX), CX
        tmp.go:4        0x804900d       3b6108          CMPL 0x8(CX), SP
        tmp.go:4        0x8049010       760e            JBE 0x8049020
        tmp.go:5        0x8049012       0fb65c2404      MOVZX 0x4(SP), BX
        tmp.go:5        0x8049017       025c2405        ADDB 0x5(SP), BL
        tmp.go:5        0x804901b       885c2408        MOVB BL, 0x8(SP)
        tmp.go:5        0x804901f       c3              RET
        tmp.go:4        0x8049020       e8fb450400      CALL runtime.morestack_noctxt(SB)


go version devel +0cff219 Wed Sep 7 10:43:13 2016 +0000 linux/386
TEXT main.f(SB) /home/vagrant/objdump/tmp.go
        tmp.go:4        0x8049000       658b0d00000000  GS MOVL GS:0(IP), CX
        tmp.go:4        0x8049007       8b89fcffffff    MOVL 0xfffffffc(CX), CX
        tmp.go:4        0x804900d       3b6108          CMPL 0x8(CX), SP
        tmp.go:4        0x8049010       7611            JBE 0x8049023
        tmp.go:5        0x8049012       0fb6442405      MOVZX 0x5(SP), AX
        tmp.go:5        0x8049017       0fb64c2404      MOVZX 0x4(SP), CX
        tmp.go:5        0x804901c       01c8            ADDL CX, AX
        tmp.go:5        0x804901e       88442408        MOVB AL, 0x8(SP)
        tmp.go:5        0x8049022       c3              RET

@randall77
Copy link
Contributor Author

I concur. This looks fixed.

@golang golang locked and limited conversation to collaborators Sep 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
4 participants
@appleby @feiling @randall77 @gopherbot