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/objdump: always assume 64 bit mode even if it is 32 bit for x86 CPU #22093

Closed
hirochachacha opened this issue Sep 30, 2017 · 11 comments
Closed

cmd/objdump: always assume 64 bit mode even if it is 32 bit for x86 CPU #22093

hirochachacha opened this issue Sep 30, 2017 · 11 comments
Labels
FrozenDueToAge

Comments

@hirochachacha
Copy link
Contributor

See following code.

go/src/cmd/internal/objfile/disasm.go

Lines 298 to 317 in ee4fbbc

func disasm_386(code []byte, pc uint64, lookup lookupFunc, _ binary.ByteOrder) (string, int) {
return disasm_x86(code, pc, lookup, 32)
}
func disasm_amd64(code []byte, pc uint64, lookup lookupFunc, _ binary.ByteOrder) (string, int) {
return disasm_x86(code, pc, lookup, 64)
}
func disasm_x86(code []byte, pc uint64, lookup lookupFunc, arch int) (string, int) {
inst, err := x86asm.Decode(code, 64)
var text string
size := inst.Len
if err != nil || size == 0 || inst.Op == 0 {
size = 1
text = "?"
} else {
text = x86asm.GoSyntax(inst, pc, lookup)
}
return text, size
}

disasm_386 and disasm_amd64 pass its own CPU mode to disasm_x86, however disasm_x86 always use the hard coded value 64.

I'm not familiar with CPUs, so I cannot provide minimal example. But this is probably a bug.
@dougpuob
Copy link

dougpuob commented Sep 30, 2017 •
edited

@hirochachacha
I think that the x86asm means 80x86, including i386 and amd64. And the second parameter of the function description of x86asm.Decode() handles 16, 32, 64 bit processor mode.

// Decode decodes the leading bytes in src as a single instruction. The mode arguments specifies
// the assumed processor mode: 16, 32, or 64 for 16-, 32-, and 64-bit execution modes.
func Decode(src []byte, mode int) (inst Inst, err error)

x86asm.Decode()
x86asm.decode1()

@hirochachacha
Copy link
Contributor Author

@dougpuob yes, so, what conclusion are you implying? Assuming 64-bit execution mode for the binary that is created for 32 bit CPU is right?

@dougpuob
Copy link

dougpuob commented Oct 1, 2017 •
edited

@hirochachacha
According to the second parameter of the x86asm.decode1() function treats input binary array, src, as i386 or amd64 opcode then decodes a single instruction (assembly code) of i386 or amd64.

x86asm.Decode(i386_binary_stream, 86); // decodes a single instruction for i386
x86asm.Decode(amd64_binary_stream, 64);// decodes a single instruction for amd64

See the return data type of Decode() function is Inst, Inst.Op is the opcode of the single instruction; Inst.Len is how many byte combine the single instruction from src.

// *src/cmd/vendor/golang.org/x/arch/x86/x86asm/inst.go*
type Inst struct {
	Prefix   Prefixes // Prefixes applied to the instruction.
	Op       Op       // Opcode mnemonic
	Opcode   uint32   // Encoded opcode bits, left aligned (first byte is Opcode>>24, etc)
	Args     Args     // Instruction arguments, in Intel order
	Mode     int      // processor mode in bits: 16, 32, or 64
	AddrSize int      // address size in bits: 16, 32, or 64
	DataSize int      // operand size in bits: 16, 32, or 64
	MemBytes int      // size of memory argument in bytes: 1, 2, 4, 8, 16, and so on.
	Len      int      // length of encoded instruction in bytes
	PCRel    int      // length of PC-relative address in instruction encoding
	PCRelOff int      // index of start of PC-relative address in instruction encoding
}

@hirochachacha
Copy link
Contributor Author

@dougpuob So, what's your conclusion?

@dougpuob
Copy link

dougpuob commented Oct 1, 2017 •
edited

Hi @hirochachacha. I think it is not about assuming 64-bit or 32-bit execution mode, input array is 64-bit data and you indicate 64, then function return a 64-bit instruction, so and 32-bit. My conclusion is the function returns what you told him, didn't assume. If my conception is incorrect please let me know.

@randall77
Copy link
Contributor

It certainly sounds like a bug. Does it actually mis-disassemble anything though?
Maybe disassemble a Go binary with and without the obvious fix and diff them.

@hirochachacha
Copy link
Contributor Author

z.go

package main

import "fmt"

func main() {
	fmt.Print("Hello World!")
}

patch

diff --git a/src/cmd/internal/objfile/disasm.go b/src/cmd/internal/objfile/disasm.go
index ede1141a3e..c99c34b179 100644
--- a/src/cmd/internal/objfile/disasm.go
+++ b/src/cmd/internal/objfile/disasm.go
@@ -304,7 +304,7 @@ func disasm_amd64(code []byte, pc uint64, lookup lookupFunc, _ binary.ByteOrder)
 }

 func disasm_x86(code []byte, pc uint64, lookup lookupFunc, arch int) (string, int) {
-       inst, err := x86asm.Decode(code, 64)
+       inst, err := x86asm.Decode(code, arch)
        var text string
        size := inst.Len
        if err != nil || size == 0 || inst.Op == 0 {

diff (before patch - after patch for z.o which is created by GOARCH=386 go tool compile z.go)

--- z.txt	2017-10-02 07:41:32.000000000 +0900
+++ z2.txt	2017-10-02 07:42:32.000000000 +0900
@@ -1,13 +1,13 @@
 TEXT %22%22.main(SB) gofile../Users/hiro/z.go
-  z.go:5		0x3bb			658b0d00000000		MOVL GS:0(IP), CX	[3:7]R_TLS_LE
+  z.go:5		0x3bb			658b0d00000000		MOVL GS:0, CX		[3:7]R_TLS_LE
   z.go:5		0x3c2			3b6108			CMPL 0x8(CX), SP
   z.go:5		0x3c5			7647			JBE 0x40e
   z.go:5		0x3c7			83ec20			SUBL $0x20, SP
   z.go:6		0x3ca			c744241800000000	MOVL $0x0, 0x18(SP)
   z.go:6		0x3d2			c744241c00000000	MOVL $0x0, 0x1c(SP)
-  z.go:6		0x3da			8d0500000000		LEAL 0(IP), AX		[2:6]R_ADDR:type.string
+  z.go:6		0x3da			8d0500000000		LEAL 0, AX		[2:6]R_ADDR:type.string
   z.go:6		0x3e0			89442418		MOVL AX, 0x18(SP)
-  z.go:6		0x3e4			8d0500000000		LEAL 0(IP), AX		[2:6]R_ADDR:%22%22.statictmp_0
+  z.go:6		0x3e4			8d0500000000		LEAL 0, AX		[2:6]R_ADDR:%22%22.statictmp_0
   z.go:6		0x3ea			8944241c		MOVL AX, 0x1c(SP)
   z.go:6		0x3ee			8d442418		LEAL 0x18(SP), AX
   z.go:6		0x3f2			890424			MOVL AX, 0(SP)
@@ -20,19 +20,19 @@
   z.go:5		0x413			eba6			JMP %22%22.main(SB)

 TEXT %22%22.init(SB) gofile..<autogenerated>
-  gofile..<autogenerated>:1	0x432			658b0d00000000		MOVL GS:0(IP), CX	[3:7]R_TLS_LE
+  gofile..<autogenerated>:1	0x432			658b0d00000000		MOVL GS:0, CX		[3:7]R_TLS_LE
   gofile..<autogenerated>:1	0x439			3b6108			CMPL 0x8(CX), SP
   gofile..<autogenerated>:1	0x43c			762a			JBE 0x468
-  gofile..<autogenerated>:1	0x43e			0fb60500000000		MOVZX 0(IP), AX		[3:7]R_ADDR:%22%22.initdone·
+  gofile..<autogenerated>:1	0x43e			0fb60500000000		MOVZX 0, AX		[3:7]R_ADDR:%22%22.initdone·
   gofile..<autogenerated>:1	0x445			80f801			CMPL $0x1, AL
   gofile..<autogenerated>:1	0x448			7601			JBE 0x44b
   gofile..<autogenerated>:1	0x44a			c3			RET
   gofile..<autogenerated>:1	0x44b			7507			JNE 0x454
   gofile..<autogenerated>:1	0x44d			e800000000		CALL 0x452		[1:5]R_CALL:runtime.throwinit
   gofile..<autogenerated>:1	0x452			0f0b			UD2
-  gofile..<autogenerated>:1	0x454			c6050000000001		MOVB $0x1, 0(IP)	[2:6]R_ADDR:%22%22.initdone·
+  gofile..<autogenerated>:1	0x454			c6050000000001		MOVB $0x1, 0		[2:6]R_ADDR:%22%22.initdone·
   gofile..<autogenerated>:1	0x45b			e800000000		CALL 0x460		[1:5]R_CALL:fmt.init
-  gofile..<autogenerated>:1	0x460			c6050000000002		MOVB $0x2, 0(IP)	[2:6]R_ADDR:%22%22.initdone·
+  gofile..<autogenerated>:1	0x460			c6050000000002		MOVB $0x2, 0		[2:6]R_ADDR:%22%22.initdone·
   gofile..<autogenerated>:1	0x467			c3			RET
   gofile..<autogenerated>:1	0x468			e800000000		CALL 0x46d		[1:5]R_CALL:runtime.morestack_noctxt
   gofile..<autogenerated>:1	0x46d			ebc3			JMP %22%22.init(SB)

It looks like this is a root cause of #19988.

@hirochachacha
Copy link
Contributor Author

@dougpuob

My conclusion is the function returns what you told him, didn't assume.

I'm saying that disasm_x86 don't pass the correct argument to x86asm.Decode.

@randall77
Copy link
Contributor

Yes, that looks like using arch instead of 64 in disasm_x86 is correct.
@hirochachacha Can you send me a CL with your fix?

@hirochachacha
Copy link
Contributor Author

Sure.

@gopherbot
Copy link

Change https://golang.org/cl/67450 mentions this issue: cmd/objdump: pass the correct execution mode to x86asm.Decode in disasm_x86 on 386

@golang golang locked and limited conversation to collaborators Nov 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@hirochachacha @dougpuob @randall77 @gopherbot