proposal: x/arch/arm64/arm64asm: all fields of potential Args structs should be exported

[fabled]

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

$ go version
go1.17.7

Does this issue reproduce with the latest release?

Yes.

What did you do?

I would like to programmatically access all fields of the arm64 disassembler Instruction Args.

	code := []byte{0x01,0xd4,0x43,0xf9} // LDR X1, [X0,#1960]
	inst, _ := arm64asm.Decode(code)
	fmt.Printf("opcode: %#v\n", inst)
	// arm64asm.Inst{Op:0xb1, Enc:0xf943d401,
	//	Args:arm64asm.Args{0x21, arm64asm.MemImmediate{Base:0x20, Mode:0x3, imm:1960},
	//	arm64asm.Arg(nil), arm64asm.Arg(nil), arm64asm.Arg(nil)}}

	// like to extract the offset
	arg := inst.Args[1].(arm64asm.MemImmediate)
	fmt.Printf("argument: %#v\n", arg)
	// arm64asm.MemImmediate{Base:0x20, Mode:0x3, imm:1960}
	//fmt.Printf("offset: %x\n", arg.imm) // error: "cannot refer to unexported field or method imm"

What did you expect to see?

To be able to access programmatically instruction arguments. This would be consistent with the other disassemblers: x86, arm32 and ppc. All of them have all Args field exported (or be typedef to a basic type). It seems arm64 is the only exception where some fields are not exported.

What did you see instead?

Build error, as several fields are unexported.

Change https://go.dev/cl/388714 contains suggested change.

[ianlancetaylor]

CC @cherrymui

[erifan]

I'm not against exporting these fields, but I think it's better for you to define a set of data structures yourself to represent Instructions and parameters, because the format of Instructions won't change, but these internal data structures may.

[fabled]

I believe Instructions and the Args are direct representation of the instruction set encoding. I don't see these data structures changing unless someone changes the ARM opcode encoding.

[rsc]

FWIW it was definitely the intent of the other disassemblers to have these be "pure data" that could be directly manipulated. Is there a reason not to do the same for arm64?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

Does anyone object to accepting this proposal?

[erifan]

I'm still a little concerned about this. There are two points:

1. I think some data structures of the current disassembler are not well designed enough. For example,
   Go assembly instruction MRS ELR_EL1, R8, encoded as 0xd5384028.
   Go disassembles 0xd5384028 as Go assembly MRS $16897, R8, GNU assembly mrs x8, s3_0_c4_c0_1
   However GCC disassembles the binary as mrs x8, elr_el1
   From the point of view of the Arm specification, mrs x8, s3_0_c4_c0_1 and mrs x8, elr_el1 are both valid, we can use the system register name (elr_el1 ) or the combination of fields (s3_0_c4_c0_1). Currently we don't have any specification saying that which one should we use. If we go the same way as GCC does with system register names, then the data structures defined for system registers today are obviously not good enough, see https://github.com/golang/arch/blob/b76863e36670e165c85261bc41fabaf345376022/arm64/arm64asm/inst.go#L809
   In addition, if we introduce a new arm instruction, maybe we can reuse the current data structure with slight changes, but with exported data structure, we will have to define a new data structure.

2. I actually don't understand why the disassembler is a separate project, leading to the assembler and disassembler are often not synchronized enough. When I was refactoring the arm64 assembler, I found that they can actually share a lot of things, such as instruction tables, data structures, etc. This gives me a strong drive to do some refactoring work on disassembler to make the two synchronize better, if this is really necessary, then some data structures and tables will inevitably be modified. I don't want the refactoring to be limited by these exported data structures.

[cherrymui]

I think it is fine to export the fields. But we probably want to polish the names a little bit. For example, currently we have

type RegisterWithArrangement struct {
	r   Reg
	a   Arrangement
	cnt uint8
}

When exported, maybe they should be Reg, Arrangement, and Count, instead of R, A, and Cnt.

Alternatively we can keep fields unexported but add accessor methods (it doesn't seem necessary to set the fields in user code, just read them). But that doesn't sound very Go-like.

[rsc]

It sounds like we want to work on field names and maybe also introduce some new registers for things like ELR_EL1.
Do I have that right?

[rsc]

If the new API will take a while to design, we can always put this on hold until it is ready.

[rsc]

Should we put this on hold?

[fabled]

Is anything needed on my part? Or is this waiting still internal decision?

[cherrymui]

I think we need to put up the names for exported types/fields/methods/etc. @fabled feel free to propose a list. Thanks.

[rsc]

Will put this on hold; feel free to take off hold once there is a concrete proposal for the new fields. Thanks!

[rsc]

Placed on hold.
— rsc for the proposal review group

[davidben]

Exporting these would be very useful for some tool I'm toying with to test BoringSSL's assembly code. (I'm more using it as an instruction decoder than a disassembler, but it seems to do the trick!) For now I'm just parsing the string representation of the unexported structs back out, which is rather tedious. Beyond field names, the ExtShift constants also aren't exported.

From the discussion, it sounds like there's some question as to whether the system registers are done right, and also issues with some names. The system registers are less commonly used (and, not coincidentally, I'm less familiar with them 😄 ), so perhaps we should leave those unexported in the first iteration and leave that for later? I would then propose:

• Export ImmShift fields as-is: Imm and Shift. Imm is consistent with Imm64.Imm and Shift is a fine name.
• Export ExtShift constants as all-caps with no decoration. So UXTB, etc. ExtShiftUXTB would also work, but this is consistent with X0, etc. It's inconsistent with Arrangement8B, but those would otherwise begin with digits and needed some prefix.
• Export RegExtshiftAmount fields as-is: Reg, ExtShift, and Amount. Leave show_zero alone for now, but probably rename it to showZero for style consistency. (If I'm understanding that structure correctly, show_zero is not necessary for interpreting the instruction, just stringifying it canonically.) Or we could call it ShowZero if folks want.
• Export MemImmediate.imm as Imm
• Leave Systemreg alone for now, per questions in proposal: x/arch/arm64/arm64asm: all fields of potential Args structs should be exported #51517 (comment)
• Export Imm_fp fields as Sign, Exp, and Mantissa, respectively.
• Export RegisterWithArrangement fields as Reg, Arrangement, and Count, respectively
• Export RegisterWithArrangementAndIndex fields as Reg, Arrangement, Index, and Count, respectively

How does that sound to folks?

[fabled]

Sounds good to me. I'll update the draft PR accordingly to get things forward.

[fabled]

• Export ExtShift constants as all-caps with no decoration. So UXTB, etc. ExtShiftUXTB would also work, but this is consistent with X0, etc. It's inconsistent with Arrangement8B, but those would otherwise begin with digits and needed some prefix.

This does not work:

arm64/arm64asm/tables.go:29:2: ASR redeclared in this block
arm64/arm64asm/inst.go:411:2: other declaration of ASR
arm64/arm64asm/tables.go:210:2: LSL redeclared in this block
arm64/arm64asm/inst.go:409:2: other declaration of LSL
arm64/arm64asm/tables.go:212:2: LSR redeclared in this block
arm64/arm64asm/inst.go:410:2: other declaration of LSR
arm64/arm64asm/tables.go:250:2: ROR redeclared in this block
arm64/arm64asm/inst.go:412:2: other declaration of ROR
arm64/arm64asm/tables.go:387:2: SXTB redeclared in this block
arm64/arm64asm/inst.go:405:2: other declaration of SXTB
arm64/arm64asm/tables.go:388:2: SXTH redeclared in this block
arm64/arm64asm/inst.go:406:2: other declaration of SXTH
arm64/arm64asm/tables.go:391:2: SXTW redeclared in this block
arm64/arm64asm/inst.go:407:2: other declaration of SXTW
arm64/arm64asm/tables.go:465:2: UXTB redeclared in this block
arm64/arm64asm/inst.go:401:2: other declaration of UXTB
arm64/arm64asm/tables.go:466:2: UXTH redeclared in this block
arm64/arm64asm/inst.go:402:2: other declaration of UXTH

[fabled]

Other parts updated to https://go-review.googlesource.com/c/arch/+/388714/. I also changed Imm_fp.Sign to a bool.

[davidben]

This does not work: [...]

Oh right, those are also instructions! Alright, how about ExtShiftLSL, etc?

[fabled]

Oh right, those are also instructions! Alright, how about ExtShiftLSL, etc?

Sounds good. Patch updated accordingly.

@rsc could you remove the Hold label, and have this reviewed? Thanks!

[davidben]

Ah I guess one interesting point about MemImmediate.imm is that it's not quite an immediate. It's actually either an immediate or a register. See this code:
https://cs.opensource.google/go/x/arch/+/fc48f9fe:arm64/arm64asm/inst.go;drc=fc48f9fe4c157e3ed95b38adbda9b9fe5a31cf03;l=525

Should there be separate Imm and Reg fields with the AddrPostReg option using Reg instead of Imm? If not, we at least should document that AddrPostReg's second register is stored in Imm, as an offset from X0.

[davidben]

Or perhaps s/Reg/Index/ to match MemExtend, if we go with the separate variable option.

[fabled]

@rsc Gentle ping. Any suggestions how to get this forward?

[erifan]

Hi @fabled , I am refactoring the arm64 assembler, see #44734, and I also plan to refactor the disassembler based on the new instruction table, so many data structures may be changed, and more argument structs will be supported. In the new code I will export all fields. But I don’t have a clear timeline yet.