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

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!

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

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?

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

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

Folks could type assert to the larger interface?

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

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.

Interesting. Any suggestions for how we can move forwards?

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))

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?

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

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:

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())
}

=== 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

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))

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))

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