debug/pe: add Machine type with String method

[jamietanna]

When using the debug/pe package to look up information about binaries, I noticed that there's a subtle API difference compared to debug/macho and debug/elf.

For instance, in the debug/elf package, Machine is defined as being able to call:

f.Machine.String()

Whereas when using the debug/pe package, Machine has no such definition in the FileHeader.

It would be good to align the API of this package alongside how we do it in the other packages:

• Extract a type for Machine
• Add a String() method
• Add a GoString() method

[gopherbot]

Change https://go.dev/cl/508401 mentions this issue: debug/pe: addMachinetype to align API with debug/elf and debug/macho

[jamietanna]

Hey folks, just wondering if there's anything I can do to help this along? Appreciate it's only a small proposal and there are a lot of the bigger ones to go through at the moment!

[qmuntal]

Hi @jamietanna. I'm afraid this is a backwards incompatible change, as you are changing the definition of pe.FileHeader.

[jamietanna]

That's fair - I'm sure there's a way we can make this non-breaking, while still allowing for this new metadata to be usable?

[josharian]

Could we expose a new method in debug/* to get at the Machine? (What would it be called?)

[rsc]

We can add the type, but if we can't use it anywhere in the API, is there much value?

[josharian]

Folks could type assert to the larger interface?

[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]

There's not really a larger interface, just a different integer type. So you'd have to say pe.Machine(fh.Machine).String(). I guess that's fine but it's a little weird.

[jamietanna]

Interesting. Any suggestions for how we can move forwards?

[rsc]

I believe the proposal is:

In package pe, add:

type Machine uint16
func (m Machine) String() string
func (m Machine) GoString() string

similar to elf.Machine. The existing Machine uint16 in type FileHeader would not change, for compatibility. Users who want to print a file header’s machine need to use

fmt.Println(pe.Machine(fh.Machine))

[jamietanna]

Gotcha, that makes sense, thanks!

Would we also be able to make the following change:

-   IMAGE_FILE_MACHINE_UNKNOWN     = 0x0
+   IMAGE_FILE_MACHINE_UNKNOWN     Machine = 0x0

Or would this introduce a breaking change? I think probably yes it would be a breaking change?

[rsc]

@jamietanna Yes, that would be a breaking change, and we can't do that.

[jamietanna]

Gotcha, thanks!

I've amended the proposed code to fit within this, which should now no longer be a breaking change. Thanks for the feedback and suggestions folks.

I've also verified it with the following test:

```go package pe import ( "fmt" "testing" ) func TestMachineToString(t *testing.T) { c := IMAGE_FILE_MACHINE_WCEMIPSV2 fmt.Printf("c: %v\n", c) fmt.Printf("Machine(c): %v\n", Machine(c)) fmt.Printf("Machine(c).GoString(): %v\n", Machine(c).GoString()) c = 0 fmt.Printf("c: %v\n", c) fmt.Printf("Machine(c): %v\n", Machine(c)) fmt.Printf("Machine(c).GoString(): %v\n", Machine(c).GoString()) c = 0x5128 fmt.Printf("c: %v\n", c) fmt.Printf("Machine(c): %v\n", Machine(c)) fmt.Printf("Machine(c).GoString(): %v\n", Machine(c).GoString()) } ``` Test output: ``` === RUN TestMachineToString c: 361 Machine(c): IMAGE_FILE_MACHINE_WCEMIPSV2 Machine(c).GoString(): pe.IMAGE_FILE_MACHINE_WCEMIPSV2 c: 0 Machine(c): IMAGE_FILE_MACHINE_UNKNOWN Machine(c).GoString(): pe.IMAGE_FILE_MACHINE_UNKNOWN c: 20776 Machine(c): IMAGE_FILE_MACHINE_RISCV128 Machine(c).GoString(): pe.IMAGE_FILE_MACHINE_RISCV128 ```

[rsc]

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

In package pe, add:

type Machine uint16
func (m Machine) String() string
func (m Machine) GoString() string

similar to elf.Machine. The existing Machine uint16 in type FileHeader would not change, for compatibility. Users who want to print a file header’s machine need to use

fmt.Println(pe.Machine(fh.Machine))

[rsc]

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

In package pe, add:

type Machine uint16
func (m Machine) String() string
func (m Machine) GoString() string

similar to elf.Machine. The existing Machine uint16 in type FileHeader would not change, for compatibility. Users who want to print a file header’s machine need to use

fmt.Println(pe.Machine(fh.Machine))

[jamietanna]

Awesome 👏🏽 for next steps, is it a case of the linked changeset getting reviewed when the core team have a chance?