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

Jump to bottom

cmd/link: code cleanup #22095

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

cmd/link: code cleanup #22095

crawshaw opened this issue Sep 30, 2017 · 19 comments
Labels
FrozenDueToAge

Comments

@crawshaw
Copy link
Member

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
Copy link
Member Author

cc @ianlancetaylor

@gopherbot
Copy link

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

@crawshaw
Copy link
Member Author

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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
@crawshaw
cmd/link: remove SysArch global variable
5d95de2 
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
@crawshaw
cmd/link: remove coutbuf global variable
5fe9bbc 
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
@crawshaw
cmd/link: convert symbol Add* functions to methods
0346664 
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
@crawshaw
cmd/link: remove ctxt from objfile reader
c80338a 
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
Copy link

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

@gopherbot
Copy link

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
@crawshaw
cmd/link: put symbol data types in new package
475d92b 
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
@crawshaw
cmd/link: move Library type to sym package
a910fe2 
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
Copy link

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
@crawshaw
cmd/link: give the object reader its own package
9f9bb97 
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
@crawshaw
cmd/link: move build/link mode globals into ctxt
2e85455 
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
Copy link

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

@gopherbot
Copy link

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
@crawshaw
cmd/link: move ldmacho to its own package
f7ad3a0 
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
Copy link

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
@stemar94 @crawshaw
cmd/link/internal/loadmacho: reduce scope of local declarations
da4d740 
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
@crawshaw
cmd/link: move ELF reader to its own package
ecfa737 
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
Copy link

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
@crawshaw
cmd/link: split PE loader into its own package
d06815b 
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
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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
@crawshaw
cmd/link: move FlagLinkshared global to ctxt
6738c49 
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
@crawshaw
cmd/link: move Iself global to ctxt
d05f82a 
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
@crawshaw
cmd/link: move Headtype global to ctxt
38cfeb9 
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
Copy link
Contributor

I am going to declare the code clean for now.

@gopherbot
Copy link

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
@quasilyte @bradfitz
cmd/link/internal/ld: make Thearch unexported
da02dcd 
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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@crawshaw @ianlancetaylor @gopherbot