-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
debug/pe: add Machine type with String method #60198
Comments
Change https://go.dev/cl/508401 mentions this issue: |
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 |
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 |
Machine
type with a corresponding String()
method
There's not really a larger interface, just a different integer type. So you'd have to say |
Interesting. Any suggestions for how we can move forwards? |
I believe the proposal is: In package pe, add:
similar to elf.Machine. The existing
|
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())
} Test output:
|
Based on the discussion above, this proposal seems like a likely accept. In package pe, add:
similar to elf.Machine. The existing
|
No change in consensus, so accepted. 🎉 In package pe, add:
similar to elf.Machine. The existing
|
Awesome 👏🏽 for next steps, is it a case of the linked changeset getting reviewed when the core team have a chance? |
When using the
debug/pe
package to look up information about binaries, I noticed that there's a subtle API difference compared todebug/macho
anddebug/elf
.For instance, in the
debug/elf
package,Machine
is defined as being able to call:Whereas when using the
debug/pe
package,Machine
has no such definition in theFileHeader
.It would be good to align the API of this package alongside how we do it in the other packages:
Machine
String()
methodGoString()
methodThe text was updated successfully, but these errors were encountered: