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: need to add buildids to wasm modules #25910

Closed
bradfitz opened this issue Jun 15, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@bradfitz
Copy link
Member

commented Jun 15, 2018

dist is failing for js/wasm right now (when run via make.bash, etc) due to missing buildids:

https://build.golang.org/log/4dfe5bd474996d98e85d77bfa4f63b9713b2ccbe

Repro:

$ GOOS=js GOARCH=wasm ./make.bash

@rsc says:

the reason things look stale is that cmd/go recomputes what the expected build id is and then looks in the binary to see if that's the recorded buildid. if so, not stale. if not, then it's stale
go tool buildid $GOROOT/pkg/tool/js_wasm/dist
prints an empty string
looks like the build id is not in the binary
...
what we do on systems with inflexible object formats (like darwin and windows) is arrange for the build id to appear early in the binary (in teh first few kilobytes) as string data
and we search for a magic indicator to find it.
src/cmd/link/internal/ld/data.go textbuildid
so wasm should do something like that too
we almost certainly want build ids generally. it's not just about dist
...
it's going to just keep rebuilding because it looks out of date
i mean, i don't think we'll run any of the binaries in pkg/js_wasm/tool
but still

/cc @neelance @ianlancetaylor

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2018

The linker's Link.textbuildid code is running and prepending to ctx.Textp, and then cmd/link/internal/wasm/asm.go has code that looks for it:

        for i, fn := range ctxt.Textp {
                wfn := new(bytes.Buffer)
                if fn.Name == "go.buildid" {
                        writeUleb128(wfn, 0) // number of sets of locals                                                                                           
                        writeI32Const(wfn, 0)
                        wfn.WriteByte(0x0b) // end                                                                                                                 

                } else {

I think we need to note the string value there and add a wasm data section early in the module. Not sure it's valid to have multiple data sections, though. (I'd considered a name, but names must be UTF-8, and the buildid is intentionally invalid UTF-8)

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2018

Ah, we can use a custom section. Perfect.

CL forthcoming.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 15, 2018

Change https://golang.org/cl/119175 mentions this issue: cmd/link: add buildid to wasm modules

@gopherbot gopherbot closed this in 3b0b3a0 Jun 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.