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.

Sign up for GitHub

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

Jump to bottom

x/tools/go/gcexportdata: simplify implementation assuming go >= 1.21 #70651

Closed
timothy-king opened this issue Dec 3, 2024 · 14 comments
Closed

x/tools/go/gcexportdata: simplify implementation assuming go >= 1.21 #70651

timothy-king opened this issue Dec 3, 2024 · 14 comments
Assignees
@timothy-king
Labels
Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@timothy-king
Copy link
Contributor

timothy-king commented Dec 3, 2024 •
edited
Loading

This is a tracking issue to simplify the implementation of x/tools/{go/gcexportdata,internal/gcimporter} since the acceptance of proposal #68898. These can now assume exportdata only comes from go versions >= 1.21.
@timothy-king timothy-king self-assigned this Dec 3, 2024
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Dec 3, 2024
@gopherbot gopherbot added this to the Unreleased milestone Dec 3, 2024
@gabyhelp
Copy link

gabyhelp commented Dec 3, 2024

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633115 mentions this issue: internal/gcimporter: require archive files

gopherbot pushed a commit to golang/tools that referenced this issue Dec 3, 2024
@timothy-king
internal/gcimporter: require archive files
7a4f3b0 
Proposal golang/go#68898 was accepted. gcexportdata.Read now supports
tip and the latest two releases. FindExportData is needed only
to support Read and can thus have the same support policy.

Since go1.21, the only files the compiler produces with
the header "go object " are either in archive files or do
not contain exportdata (cmd/asm). It is therefore safe for
FindExportData to require the files to be ar files.

Updates golang/go#70651

Change-Id: Iec6703da6768198524e174824b0b05f95b96db90
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633115
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Commit-Queue: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633279 mentions this issue: internal/gcimporter: require binary export data header

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633278 mentions this issue: internal/gcimporter: update FindExportData to return a non-negative size

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633296 mentions this issue: internal/gcimporter: remove end-of-section marker from size

gopherbot pushed a commit to golang/tools that referenced this issue Dec 3, 2024
@timothy-king
internal/gcimporter: update FindExportData to return a non-negative size
399ee16 
Updates FindExportData to return a non-negative size when there is no
error. This can only happen if the archive file is malformed.

Updates golang/go#70651

Change-Id: Ia422651cd31fba83a260cf1be600ffd134012a01
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633278
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Dec 3, 2024
@timothy-king
internal/gcimporter: require binary export data header
c01eead 
FindExportData now returns an error if the header indicating
the beginning of the export data is not "$$B\n". (Historically
"$$\n" was also used for non-binary export data.) The
signature of FindExportData no longer returns hdr as it is
redundant.

This also simplifies internal/gcimporter.Import using this
new assumption.

Updates golang/go#70651

Change-Id: I0d136d6c474ae6a7bfc2baeb33c22fd412711452
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633279
Reviewed-by: Robert Griesemer <gri@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: Tim King <taking@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633435 mentions this issue: internal/gcimporter: remove indexed support from Import

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633708 mentions this issue: internal/gcimporter: require archive files

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633716 mentions this issue: internal/gcimporter: require binary export data header

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633712 mentions this issue: internal/gcimporter: update FindExportData to return a non-negative size

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633659 mentions this issue: internal/gcimporter: copy GOROOT/internal/exportdata functions

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633657 mentions this issue: internal/gcimporter: reuse archive.ReadHeader implementation

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633658 mentions this issue: internal/gcimporter: synchronizing FindPkg implementations

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633660 mentions this issue: interna/gcimporter: synchronize with internal/exportdata

gopherbot pushed a commit to golang/tools that referenced this issue Dec 9, 2024
@timothy-king
internal/gcimporter: remove end-of-section marker from size
c2df0ef 
Remove the bytes for the end-of-section marker "\n$$\n" from
the end of the exportdata by descreasing the returned size
appropriately. This marker is not a part of the export data
so it should be removed. Updates the unified IR reader to not
expect the marker to be present.

Updates golang/go#70651

Change-Id: Id3324ff21162d80f8e3d1c9792e9157b01ae1a3f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633296
Reviewed-by: Robert Findley <rfindley@google.com>
Commit-Queue: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit to golang/tools that referenced this issue Dec 9, 2024
@timothy-king
internal/gcimporter: remove indexed support from Import
6ebf95a 
Removes support for the indexed binary format from the test
only function Import.

Additionally makes Import more similar to go/internal/gcimporter.Import
by taking a token.FileSet argument.

Updates golang/go#70651

Change-Id: I39f120adc554278adb3d114194c27fd5906985a6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633435
Commit-Queue: Tim King <taking@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Dec 9, 2024
@timothy-king
internal/gcimporter: reuse archive.ReadHeader implementation
3689f0b 
Adds an internal copy of the GOROOT/src/cmd/internal/archive.ReadHeader
to x/tools. This replaces readGopackHeader. The result is one less
different implementation of reading an archive header between GOROOT
and x/tools.

Updates golang/go#70651

Change-Id: I69028da6472d6b06a613b554158301894326e735
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633657
Commit-Queue: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/634518 mentions this issue: internal/gcimporter: moving FindPkg

gopherbot pushed a commit to golang/tools that referenced this issue Dec 10, 2024
@timothy-king
internal/gcimporter: moving FindPkg
ab5f969 
Moves FindPkg and related variables from gcimporter.go to
exportdata.go. This is a part of minimizing the code delta
between internal/gcimporter and GOROOT/src/internal/exportdata.

Updates golang/go#70651

Change-Id: I74d2ba2085d0b034ff9f98f7cae17a2231ae3a6c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/634518
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Commit-Queue: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Dec 11, 2024
@timothy-king
internal/gcimporter: synchronizing FindPkg implementations
47df42f 
Synchronizes the implementation of FindPkg with the implementation
of FindPkg in $GOROOT/src/internal/exportdata/exportdata.go.
This adds an error return value.

Updates golang/go#70651

Change-Id: If0295b6d396ffca30ee75d958a50aa7f5b93848e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633658
Commit-Queue: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Dec 11, 2024
@timothy-king
internal/gcimporter: copy GOROOT/internal/exportdata functions
aca81ce 
Copies functions from GOROOT/internal/exportdata as they are.
Only local modification is to avoid saferio.

Updates golang/go#70651

Change-Id: If2f4e503f69897c51b708f995dad2720b9f59e8d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633659
Reviewed-by: Robert Findley <rfindley@google.com>
Commit-Queue: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@timothy-king timothy-king

Labels
Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@timothy-king @gopherbot @gabyhelp