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: code cleanup #22095

Closed
crawshaw opened this issue Sep 30, 2017 · 19 comments

Comments

Projects
None yet
3 participants
@crawshaw
Copy link
Contributor

commented Sep 30, 2017

General tracking bug for cmd/link code cleanup.

This code base was converted from C it shows. Several contributors have made attempts at improving various parts of it, though there is more to be done.

In particular, there are a lot of global variables and a lack of documentation. In discussion on previous cleanup CLs the general agreement is that the global variables should become parameters passed to the parts of the linker that need them. To facilitate this, a lot of variables have been dumped into the *ld.Link structure, and it has been plumbed everywhere as ctxt. Over time, it should be decomposed into just the parameters the particular component needs.

There is also some duplication that needs dealing with (some of which is my fault). For example, there is a global variable SysArch which is the same as ctxt.Arch. I believe we should switch to using a passed version of the architecture structure where possible, and remove SysArch.

@crawshaw

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Sep 30, 2017

Change https://golang.org/cl/67313 mentions this issue: cmd/link: convert symbol Add* functions to methods

@crawshaw

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2017

It would be nice to replace the concept of Library with Package if possible.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 30, 2017

Change https://golang.org/cl/67314 mentions this issue: cmd/link: remove ctxt from objfile reader

@gopherbot

This comment has been minimized.

Copy link

commented Sep 30, 2017

Change https://golang.org/cl/67315 mentions this issue: cmd/link: give the object reader its own package

@gopherbot

This comment has been minimized.

Copy link

commented Sep 30, 2017

Change https://golang.org/cl/67317 mentions this issue: cmd/link: remove SysArch global variable

@gopherbot

This comment has been minimized.

Copy link

commented Oct 1, 2017

Change https://golang.org/cl/67318 mentions this issue: cmd/link: remove coutbuf global variable

gopherbot pushed a commit that referenced this issue Oct 3, 2017

cmd/link: remove SysArch global variable
For #22095

Change-Id: I9d1f0d93f8fd701a24af826dc903eea2bc235de2
Reviewed-on: https://go-review.googlesource.com/67317
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot pushed a commit that referenced this issue Oct 4, 2017

cmd/link: remove coutbuf global variable
Begin passing coutbuf by as a parameter. To make the initial plumbing
pass easier, it is also a field in the standard ctxt parameter.

Consolidate the byte writing functions into the OutBuf object.
The result is less architecture-dependent initialization.

To avoid plumbing out everywhere we want to report an error, move
handling of out file deletion to an AtExit function.

For #22095

Change-Id: I0863695241562e0662ae3669666c7922b8c846f9
Reviewed-on: https://go-review.googlesource.com/67318
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot pushed a commit that referenced this issue Oct 4, 2017

cmd/link: convert symbol Add* functions to methods
Also reduce the passed context from *Link to *sys.Arch, so fewer
data dependencies need to be wired through all the code dealing
with symbols.

For #22095

Change-Id: I50969405d6562c5152bd1a3c443b72413e9b70bc
Reviewed-on: https://go-review.googlesource.com/67313
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot pushed a commit that referenced this issue Oct 4, 2017

cmd/link: remove ctxt from objfile reader
Preparation for moving the object file reader to its own package.

For #22095

Change-Id: I31fe4a10a2c465f8ea4bf548f40918807e4ec6b5
Reviewed-on: https://go-review.googlesource.com/67314
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Oct 4, 2017

Change https://golang.org/cl/68331 mentions this issue: cmd/link: put symbol data types in new package

@gopherbot

This comment has been minimized.

Copy link

commented Oct 4, 2017

Change https://golang.org/cl/68332 mentions this issue: cmd/link: move Library type to sym package

gopherbot pushed a commit that referenced this issue Oct 5, 2017

cmd/link: put symbol data types in new package
For #22095

Change-Id: I07c288208d94dabae164c2ca0a067402a8e5c2e6
Reviewed-on: https://go-review.googlesource.com/68331
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot pushed a commit that referenced this issue Oct 5, 2017

cmd/link: move Library type to sym package
For #22095

Change-Id: I2cb0d3e0aaf9f97952cf8dda0e99a4379e275020
Reviewed-on: https://go-review.googlesource.com/68332
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dave Cheney <dave@cheney.net>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Oct 5, 2017

Change https://golang.org/cl/68334 mentions this issue: cmd/link: move build/link mode globals into ctxt

gopherbot pushed a commit that referenced this issue Oct 6, 2017

cmd/link: give the object reader its own package
For #22095

Change-Id: Ie9ae84c758af99ac7daed26d0b3e3b0a47599edd
Reviewed-on: https://go-review.googlesource.com/67315
Run-TryBot: David Crawshaw <crawshaw@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot pushed a commit that referenced this issue Oct 6, 2017

cmd/link: move build/link mode globals into ctxt
Replace Buildmode with BuildMode and Linkmode with LinkMode.

For #22095

Change-Id: I51a6f5719d107727bca29ec8e68e3e9d87e31e33
Reviewed-on: https://go-review.googlesource.com/68334
Run-TryBot: David Crawshaw <crawshaw@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Oct 6, 2017

Change https://golang.org/cl/68930 mentions this issue: cmd/link: move ldmacho to its own package

@gopherbot

This comment has been minimized.

Copy link

commented Oct 6, 2017

Change https://golang.org/cl/69013 mentions this issue: cmd/link: move ELF reader to its own package

gopherbot pushed a commit that referenced this issue Oct 6, 2017

cmd/link: move ldmacho to its own package
For #22095

Change-Id: I660080279692b74669c45f42c28cccff71bd33b5
Reviewed-on: https://go-review.googlesource.com/68930
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Oct 7, 2017

Change https://golang.org/cl/69110 mentions this issue: cmd/link/internal/loadmacho: reduce scope of local declarations

gopherbot pushed a commit that referenced this issue Oct 7, 2017

cmd/link/internal/loadmacho: reduce scope of local declarations
Move some local declarations closer to their use, reducing their
respective lifetimes, also improve few error messages.
Follow up of CL 67370.

Updates #22095

Change-Id: I6131159ae8de571015ef5459b33d5c186e543a37
Reviewed-on: https://go-review.googlesource.com/69110
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

gopherbot pushed a commit that referenced this issue Oct 11, 2017

cmd/link: move ELF reader to its own package
Along the way, switch to using relocation constants from debug/elf.

For #22095

Change-Id: I1a64353619f95dde5aa39060c4b9d001af7dc1e4
Reviewed-on: https://go-review.googlesource.com/69013
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Oct 12, 2017

Change https://golang.org/cl/70310 mentions this issue: cmd/link: split PE loader into its own package

gopherbot pushed a commit that referenced this issue Oct 12, 2017

cmd/link: split PE loader into its own package
For #22095

Change-Id: I8f48fce571b69a7e8edf2ad7733ffdfd38676e63
Reviewed-on: https://go-review.googlesource.com/70310
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Oct 14, 2017

Change https://golang.org/cl/70833 mentions this issue: cmd/link: move FlagLinkshared global to ctxt

@gopherbot

This comment has been minimized.

Copy link

commented Oct 14, 2017

Change https://golang.org/cl/70835 mentions this issue: cmd/link: move Headtype global to ctxt

@gopherbot

This comment has been minimized.

Copy link

commented Oct 14, 2017

Change https://golang.org/cl/70834 mentions this issue: cmd/link: move Iself global to ctxt

gopherbot pushed a commit that referenced this issue Oct 20, 2017

cmd/link: move FlagLinkshared global to ctxt
For #22095

Change-Id: Ica6b3391541fe5a0355620d7c4a5107cf53eee82
Reviewed-on: https://go-review.googlesource.com/70833
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot pushed a commit that referenced this issue Oct 20, 2017

cmd/link: move Iself global to ctxt
For #22095

Change-Id: Iba3dffc782cecc15ea0e90a971a2734729984945
Reviewed-on: https://go-review.googlesource.com/70834
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot pushed a commit that referenced this issue Oct 21, 2017

cmd/link: move Headtype global to ctxt
For #22095

Change-Id: Idcfdfe8a94db8626392658bb93429454238f648a
Reviewed-on: https://go-review.googlesource.com/70835
Run-TryBot: David Crawshaw <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

I am going to declare the code clean for now.

@gopherbot

This comment has been minimized.

Copy link

commented Mar 31, 2018

Change https://golang.org/cl/103878 mentions this issue: cmd/link/internal/ld: make Thearch unexported

gopherbot pushed a commit that referenced this issue Apr 2, 2018

cmd/link/internal/ld: make Thearch unexported
s/Thearch/thearch/

This reduces the amount of exported global variables,
which in turn could make it easier to refactor them later.

Also updated somewhat vague comment about ld.Thearch.
There is no need for Thearch to be exported as Archinit is
called by ld.Main.

Updates #22095

Change-Id: I266b291f6eac0165f70c51964738206e066cea08
Reviewed-on: https://go-review.googlesource.com/103878
Run-TryBot: Iskander Sharipov <iskander.sharipov@intel.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@golang golang locked and limited conversation to collaborators Mar 31, 2019

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