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: package versioning for shared libraries is incompatible with linkobj builds #24512

Open
mdempsky opened this Issue Mar 23, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@mdempsky
Member

mdempsky commented Mar 23, 2018

During linkobj builds, __.PKGDEF files are still included in the linker object files, but they're empty stub files that only contain information already present in the .o file, so I'm looking at getting rid of them in that case.

Working on cmd/link, I notice that genhash (used for generating package versions from the __.PKGDEF files for shared library builds) makes two assumptions that are incompatible with this:

  1. They assume __.PKGDEF files are the first file in the .a. This is currently true because loadobjfile requires it to be the case, but won't be if we get rid of the __.PKGDEF files from linkobj files (as I intend to do).

  2. In linkobj builds, the __.PKGDEF file is largely useless for versioning because it doesn't actually record anything about the package. So the versioning is broken.

My plan here is:

  1. Near-term: change cmd/link to not require __.PKGDEF files for non-shared builds, change genhash to error if the archive is missing a __.PKGDEF file, and change cmd/compile to stop producing useless __.PKGDEF files in linkobj files.

    This means shared library, linkobj builds will now explicitly fail, whereas before they would work but be subtly incorrect due to useless package version files.

  2. Intermediate-term: have cmd/compile compute the package version hash and include it directly in the link object, so that cmd/link doesn't need the __.PKGDEF at all.

/cc @ianlancetaylor @mwhudson

@mdempsky mdempsky added the NeedsFix label Mar 23, 2018

@mdempsky mdempsky added this to the Go1.11 milestone Mar 23, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Mar 23, 2018

Change https://golang.org/cl/102236 mentions this issue: cmd/compile: always write pack files

@gopherbot

This comment has been minimized.

gopherbot commented Mar 23, 2018

Change https://golang.org/cl/102281 mentions this issue: cmd/link: skip __.PKGDEF in archives

@gopherbot

This comment has been minimized.

gopherbot commented Mar 23, 2018

Change https://golang.org/cl/102280 mentions this issue: cmd/link: make sure we're hashing __.PKGDEF in genhash

gopherbot pushed a commit that referenced this issue Mar 23, 2018

cmd/link: make sure we're hashing __.PKGDEF in genhash
This is currently always the case because loadobjfile complains if
it's not, but that will be changed soon.

Updates #24512.

Change-Id: I262daca765932a0f4cea3fcc1cc80ca90de07a59
Reviewed-on: https://go-review.googlesource.com/102280
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot pushed a commit that referenced this issue Mar 24, 2018

cmd/link: skip __.PKGDEF in archives
The __.PKGDEF file is a compiler object file only intended for other
compilers. Also, for build systems that use -linkobj, all of the
information it contains is present within the linker object files
already, so look for it there instead.

This requires a little bit of code reorganization. Significantly,
previously when loading an archive file, the __.PKGDEF file was
authoritative on whether the package was "main" and/or "safe". Now
that we're using the Go object files instead, there's the issue that
there can be multiple Go object files in an archive (because when
using assembly, each assembly file becomes its own additional object
file).

The solution taken here is to check if any object file within the
package declares itself as "main" and/or "safe".

Updates #24512.

Change-Id: I70243a293bdf34b8555c0bf1833f8933b2809449
Reviewed-on: https://go-review.googlesource.com/102281
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot pushed a commit that referenced this issue Mar 24, 2018

cmd/compile: always write pack files
By always writing out pack files, the object file format can be
simplified somewhat. In particular, the export data format will no
longer require escaping, because the pack file provides appropriate
framing.

This CL does not affect build systems that use -pack, which includes
all major Go build systems (cmd/go, gb, bazel).

Also, existing package import logic already distinguishes pack/object
files based on file contents rather than file extension.

The only exception is cmd/pack, which specially handled object files
created by cmd/compile when used with the 'c' mode. This mode is
extended to now recognize the pack files produced by cmd/compile and
handle them as before.

Passes toolstash-check.

Updates #21705.
Updates #24512.

Change-Id: Idf131013bfebd73a5cde7e087eb19964503a9422
Reviewed-on: https://go-review.googlesource.com/102236
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 23, 2018

@mdempsky Do you know what the status of this is?

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 29, 2018

@mdempsky

This comment has been minimized.

Member

mdempsky commented Jul 2, 2018

@ianlancetaylor The "near-term" steps I outlined are done. The "intermediate-term" steps are not done.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment