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

cmd/link: unable to ingest ARM objects and resources on Windows #42866

Closed
zx2c4 opened this issue Nov 28, 2020 · 26 comments
Closed

cmd/link: unable to ingest ARM objects and resources on Windows #42866

zx2c4 opened this issue Nov 28, 2020 · 26 comments

Comments

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Nov 28, 2020

There are a number of issues/bugs in the linker that prevent it from being able to ingest PE objects for arm, or prevent it from being able to ingest PE resource objects as produced by clang, which is the only compiler we've got for windows/arm.

I've fixed these issues already, so this issue is to coordinate the inclusion of those CLs into Go.

I'm hoping this can be done for 1.16.

CC @cherrymui @ianlancetaylor @bradfitz @alexbrainman @aclements @rsc

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 28, 2020

Change https://golang.org/cl/268237 mentions this issue: cmd/link: recognize arm header of PE objects

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 28, 2020

Change https://golang.org/cl/268238 mentions this issue: cmd/link: deal with ADDR32NB relocations the same way as ADDR32 on arm

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 28, 2020

Change https://golang.org/cl/268239 mentions this issue: cmd/link: ignore SEH marking on PE objects

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 28, 2020

Change https://golang.org/cl/268258 mentions this issue: cmd/link: do not mark resource section as writable

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 28, 2020

Change https://golang.org/cl/268337 mentions this issue: cmd/link: handle grouped resource sections

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Nov 28, 2020

Gopher Bot has now added links for all 5 patches.

@aclements
Copy link
Member

@aclements aclements commented Dec 7, 2020

@zx2c4 , I'm a little unclear on which release we should be targeting these CLs for. Is there a subset that fixes important bugs while also being low risk? Are you thinking they all need to go in to get windows/arm usable?

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 7, 2020

I was thinking they all go in now. They're all pretty low risk and none are really invasive in the least. Without it, it's hard to make a real windows/arm app. And then others in that series are just straightforward bug fixes of things that are definitely a mistake. The series is very tame, and I've been running these atop 1.15 for a while now.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Dec 8, 2020

This is about linking against PE objects generated by a C toolchain, right? Could you use -ldflags=-linkmode=external to let the C linker handle them? (And it should default to external linking, why not?)

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 8, 2020

This is about linking against PE objects generated by a C toolchain, right? Could you use -ldflags=-linkmode=external to let the C linker handle them? (And it should default to external linking, why not?)

External linking doesn't work with the toolchains available for windows/arm. I was thinking of getting windows/clang cgo support squared away at some point, maybe for 1.17, but that seems like a way larger project than fixing a few small things here and there.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Dec 8, 2020

External linking doesn't work with the toolchains available for windows/arm.

Sorry I'm a bit confused. Are you saying that the C toolchain can produce PE objects but cannot link them to executables? How would the C toolchain work, then? Or you mean the Go linker doesn't invoke the C linker correctly?

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 8, 2020

I mean that the Go toolchain doesn't invoke the C linker correctly. I haven't started very far down that path, but I suspect the first thing involved in repairing that will be conditionalizing the linker script that we're currently passing to gcc (p := writeGDBLinkerScript(), argv = append(argv, "-Wl,-T,"+p)). This doesn't seem overly difficult of a project, but it does seem like a project, and I haven't really started on it yet or looked too deeply into what's required.

@aclements
Copy link
Member

@aclements aclements commented Dec 8, 2020

Sorry, I think we're all still a little fuzzy on this.

Just to clarify, is it accurate to say the goal here is to support internal linking with C objects on windows/arm because external linking doesn't currently work?

Given that windows/arm doesn't have cgo support right now, where are these C objects coming from? How does external linking without cgo even come up?

Did this previously work and something changed or broke, or is this something that never worked on windows/arm?

In general, all other platforms have quite limited support for internally linking with C objects. Really, it's just meant to be enough to support the limited use of C in std itself, and the moment user code reaches for C we switch to external linking. Is this just about the C code in std itself (I'm honestly not sure what there is on Windows), or user C code?

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 8, 2020

Just to clarify, is it accurate to say the goal here is to support internal linking with C objects on windows/arm because external linking doesn't currently work?

No. I have no desire at all to use external linking. I thought cherry was suggesting that as some kind of temporary hack. But I would really rather not, as internal linking is faster and doesn't require me to haul a large toolchain around.

I'm not using cgo at all. I actually spent a bunch of time reworking C code into Go (before, after) so that I wouldn't need Cgo, due to windows/arm's lack of support for it, and now that it's gone, I'm very happy to have everything in pure Go.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Dec 8, 2020

No, I did not suggest external linking as a temporary hack. In general, cgo is supported with external linking. Internal linking is used by default in only limited cases. You can opt in internal linking, but it is not a general solution, because we don't want to reimplement a complete C/C++ linker.

As you noted, cgo is not yet supported on windows/arm. What C objects are you trying to link against? Since cgo is not supported, it wouldn't work to call into C code anyway.

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 8, 2020

Oh, okay. I'm not trying to link against "C objects". Rather, I'm just linking against simple resource objects -- ones with .rsrc sections and nothing else -- which the Go linker already mostly supports, with a few caveats that my series fixes up.

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 8, 2020

Overview of the series, which lets .rsrc objects work with windows/arm:

  1. https://golang.org/cl/268237 -- cmd/link: recognize arm header of PE objects

We recognize magic bytes for 386 and amd64 objects but not arm objects, so this adds the magic detection.

  1. https://golang.org/cl/268238 -- cmd/link: deal with ADDR32NB relocations the same way as ADDR32 on arm

Existing code forgot to handle a common relocation type, used in .rsrc objects.

  1. https://golang.org/cl/268239 -- cmd/link: ignore SEH marking on PE objects

We can ignore a marking on PE objects instead of erroring out, also used in .rsrc objects.

  1. https://golang.org/cl/268258 -- cmd/link: do not mark resource section as writable

This fixes what was probably a typo. Nobody expects for the resource section to be writable at runtime!

  1. https://golang.org/cl/268337 -- cmd/link: handle grouped resource sections

This lets lets the linker handle .rsrc objects that are split into .rsrc$01 and .rsrc$02 sections, as is very commonly the case (and the only case with the tools available on windows/arm).

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Dec 9, 2020

Oh, okay. I'm not trying to link against "C objects". Rather, I'm just linking against simple resource objects -- ones with .rsrc sections and nothing else

@aclements think of icons stored in executable file. If Windows find these icons in your executable file, it will display them when you look at your executable. These icons are converted into an object file by external tools like windres (see comment in src/cmd/link/testdata/testPErsrc-complex/rsrc.syso in CL 268337). And then Go linker reads these object files and adds them as .rsrc PE section to a built executable.

Go linker was always able to handle resources generated by GCC version of windres. But according to Jason GCC windres is not available on Windows arm. BTW, @zx2c4 what about https://github.com/akavel/rsrc ? Why couldn't we use that instead?

Alex

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 9, 2020

@aclements think of icons stored in executable file. If Windows find these icons in your executable file, it will display them when you look at your executable. These icons are converted into an object file by external tools like windres (see comment in src/cmd/link/testdata/testPErsrc-complex/rsrc.syso in CL 268337). And then Go linker reads these object files and adds them as .rsrc PE section to a built executable.

Right. The important takeaway here being that the Go linker already has special support for resource sections, and the existing test case uses an object that was generated with the "windres" tool. The "windres" tool available with arm, however, does not work, and that's what my patches fix.

Go linker was always able to handle resources generated by GCC version of windres. But according to Jason GCC windres is not available on Windows arm. BTW, @zx2c4 what about https://github.com/akavel/rsrc ? Why couldn't we use that instead?

Extremely limited and incomplete. If we're taking that route, I'll just write my own tool instead. And if it's between writing my own tool and patching the linker with a few totally trivial patches to make things work fine with the "windres" that works on arm, then I'll go with the patches.

In other words, the tiny series is the most straightforward way to accomplish the goal of shipping windows/arm software, and fits in with already existing capabilities (ability to ingest .rsrc files). I'm confused why fixing a few things to work with arm would require so much discussion.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 9, 2020

Side question: why doesn't windres work on Windows ARM? When I wrote the first version decades ago I don't recall any platform dependencies.

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 9, 2020

Side question: why doesn't windres work on Windows ARM? When I wrote the first version decades ago I don't recall any platform dependencies.

Simply because there's no binutils or gcc for windows/arm, and attempts on the mailing list in the past have fallen on the floor. So the only mingw compiler on windows/arm is from clang's toolchain, whose windres outputs more standard resource objects.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 10, 2020

The binutils appear to support arm-pe. I was just able to do

../configure --target=arm-pe
make all-binutils

and I got a windres program. But I haven't tried using it. And perhaps there are problems building it natively on Windows ARM, i don't know.

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 10, 2020

Last I looked, the gnu toolchain would produce objects for old wince arm, but not new nt armv7.

Edit: yes, just checked again, it definitely doesn't produce usable objects.

@aclements
Copy link
Member

@aclements aclements commented Dec 14, 2020

Oh, okay. I'm not trying to link against "C objects". Rather, I'm just linking against simple resource objects -- ones with .rsrc sections and nothing else -- which the Go linker already mostly supports, with a few caveats that my series fixes up.

Thanks. This was the critical context I was really missing on this CL series.

I'm confused why fixing a few things to work with arm would require so much discussion.

It wasn't clear to any of us until your message I quoted above that what you were trying to accomplish was quite limited in scope. Your other messages had mentioned both resources and other PE objects, and that seemed to run against the design of the Go linker on other platforms and like far too much to land during the freeze (irrespective of the size of the CLs themselves). All this discussion was to get to the point where the scope and intent were clear.

Now that we understand, I think we should go ahead and land these CLs for 1.16. I don't know that we really need a freeze exception for this, but I'll write one up.

@aclements
Copy link
Member

@aclements aclements commented Dec 14, 2020

Filed #43182 for a freeze exception.

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 14, 2020

It wasn't clear to any of us until your message I quoted above that what you were trying to accomplish was quite limited in scope.

Ahh okay. Sorry for being muddled in my descriptions, but glad we found clarity. Thank you for filing the freeze exception.

@gopherbot gopherbot closed this in c9fb4eb Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants