code review 35790044: liblink: create new library based on linker code

liblink: create new library based on linker code

There is an enormous amount of code moving around in this CL,
but the code is the same, and it is invoked in the same ways.
This CL is preparation for the new linker structure, not the new
structure itself.

The new library's definition is in include/link.h.

The main change is the use of a Link structure to hold all the
linker-relevant state, replacing the smattering of global variables.
The Link structure should both make it clearer which state must
be carried around and make it possible to parallelize more easily
later.

The main body of the linker has moved into the architecture-independent
cmd/ld directory. That includes the list of known header types, so the
distinction between Hplan9x32 and Hplan9x64 is removed (no other
header type distinguished 32- and 64-bit formats), and code for unused
formats such as ipaq kernels has been deleted.

The code being deleted from 5l, 6l, and 8l reappears in liblink or in ld.
Because multiple files are being merged in the liblink directory,
it is not possible to show the diffs nicely in hg.

The Prog and Addr structures have been unified into an
architecture-independent form and moved to link.h, where they will
be shared by all tools: the assemblers, the compilers, and the linkers.
The unification makes it possible to write architecture-independent
traversal of Prog lists, among other benefits.

The Sym structures cannot be unified: they are too fundamentally
different between the linker and the compilers. Instead, liblink defines
an LSym - a linker Sym - to be used in the Prog and Addr structures,
and the linker now refers exclusively to LSyms. The compilers will
keep using their own syms but will fill out the corresponding LSyms in
the Prog and Addr structures.

Although code from 5l, 6l, and 8l is now in a single library, the
code has been arranged so that only one architecture needs to
be linked into a particular program: 5l will not contain the code
needed for x86 instruction layout, for example.

The object file writing code in liblink/obj.c is from cmd/gc/obj.c.

Preparation for golang.org/s/go13linker work.

This CL does not build by itself. It depends on 35740044
and will be submitted at the same time.

[iant, 2013-12-04 19:14:12]

Comment just from reading link.h.

https://codereview.appspot.com/35790044/diff/20001/include/link.h
File include/link.h (right):

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode46
include/link.h:46: struct	Addr
It would be nice to have some comments at some point.  I guess it doesn't have
to be now.  It's not like there are any comments today.  Sigh.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode52
include/link.h:52: char	sval[8];
No vval?

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode70
include/link.h:70: uint8	etype;
It's nice to have etype down here, but if you pulled it up to after class you
would reduce the size of this struct by 8 bytes.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode91
include/link.h:91: uchar	reg;
Comment reg and scond as only for 5g?

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode255
include/link.h:255: char *file;		// object file
This is your chance to remove that extra tab.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode305
include/link.h:305: struct	Link
This is a new struct, it could get a comment even if nothing else does.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode310
include/link.h:310: int	HEADTYPE;
Should really be headtype, I think.  It's not a constant any more, if it ever
was.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode356
include/link.h:356: int		rexflag;
Looks like an extra tab here and on next line.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode369
include/link.h:369: Library*	library;
Extra tabs here and next three lines?

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode384
include/link.h:384: Hist2 *histcopy;
Here you seem to switch from tabs to spaces.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode393
include/link.h:393: struct LinkArch
Comment for struct?

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode408
include/link.h:408: int	PtrSize;
Do these field names really need capital letters?

[rsc, 2013-12-04 19:23:34]

Hello iant, iant@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[iant, 2013-12-05 02:20:06]

https://codereview.appspot.com/35790044/diff/40001/src/cmd/5l/asm.c
File src/cmd/5l/asm.c (right):

https://codereview.appspot.com/35790044/diff/40001/src/cmd/5l/asm.c#newcode426
src/cmd/5l/asm.c:426: addgotsyminternal(Link *ctxt, LSym *s)
What is your guideline for whether a function should take a ctxt argument or
not?  This file has references to the global ctxt variable.  Then it has static
functions like this with a ctxt parameter.

https://codereview.appspot.com/35790044/diff/40001/src/cmd/ld/pobj.c
File src/cmd/ld/pobj.c (right):

https://codereview.appspot.com/35790044/diff/40001/src/cmd/ld/pobj.c#newcode168
src/cmd/ld/pobj.c:168: ctxt->linkmode = linkmode;
Setting the linkmode field is also in archinit in 6l/obj.c and 8l/obj.c for some
reason.

[rsc, 2013-12-05 02:59:07]

PTAL

Thanks.

https://codereview.appspot.com/35790044/diff/20001/include/link.h
File include/link.h (right):

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode46
include/link.h:46: struct	Addr
On 2013/12/04 19:14:13, iant wrote:
> It would be nice to have some comments at some point.  I guess it doesn't have
> to be now.  It's not like there are any comments today.  Sigh.

I agree it would be nice. These structures have the meaning they had in the
other programs, for now. I do intend to add comments throughout as part of
cleaning up the code base after the refactoring is done.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode52
include/link.h:52: char	sval[8];
On 2013/12/04 19:14:13, iant wrote:
> No vval?

Turns out no. Integer constants use offset.
And branch is on the chopping block too
(it is redundant with p->pcond), but after everything else.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode70
include/link.h:70: uint8	etype;
On 2013/12/04 19:14:13, iant wrote:
> It's nice to have etype down here, but if you pulled it up to after class you
> would reduce the size of this struct by 8 bytes.

Done.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode91
include/link.h:91: uchar	reg;
On 2013/12/04 19:14:13, iant wrote:
> Comment reg and scond as only for 5g?

Done.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode255
include/link.h:255: char *file;		// object file
On 2013/12/04 19:14:13, iant wrote:
> This is your chance to remove that extra tab.

done.
(it was aligned in the variable-width font i use.)

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode310
include/link.h:310: int	HEADTYPE;
On 2013/12/04 19:14:13, iant wrote:
> Should really be headtype, I think.  It's not a constant any more, if it ever
> was.

Yeah. It never was, but I preserved all the capitalization in a misguided
attempt to reduce the diff size (ha!). Fixed.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode356
include/link.h:356: int		rexflag;
On 2013/12/04 19:14:13, iant wrote:
> Looks like an extra tab here and on next line.

Done.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode369
include/link.h:369: Library*	library;
On 2013/12/04 19:14:13, iant wrote:
> Extra tabs here and next three lines?

This one is unavoidable because "Library*" is too long, but the others are
fixed.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode384
include/link.h:384: Hist2 *histcopy;
On 2013/12/04 19:14:13, iant wrote:
> Here you seem to switch from tabs to spaces.

Done.

https://codereview.appspot.com/35790044/diff/20001/include/link.h#newcode408
include/link.h:408: int	PtrSize;
On 2013/12/04 19:14:13, iant wrote:
> Do these field names really need capital letters?

PtrSize and MINLC no. The other ones (D_XXX, AXXX) I'd really like to keep
capitalized. They won't be here for long. Once everything is switched over I am
going to define a set of constants shared across all architectures, and these
will go away. I've added a TODO about that.

https://codereview.appspot.com/35790044/diff/40001/src/cmd/5l/asm.c
File src/cmd/5l/asm.c (right):

https://codereview.appspot.com/35790044/diff/40001/src/cmd/5l/asm.c#newcode426
src/cmd/5l/asm.c:426: addgotsyminternal(Link *ctxt, LSym *s)
On 2013/12/05 02:20:07, iant wrote:
> What is your guideline for whether a function should take a ctxt argument or
> not?  This file has references to the global ctxt variable.  Then it has
static
> functions like this with a ctxt parameter.

These don't strictly need to, but they got caught up in a global search and
replace at some point and I didn't bother to undo it. When we move this to Go,
I'd like to eliminate the globals entirely so this will need to be passed around
in some form (as a receiver, probably). So it's not too big a deal either way.

https://codereview.appspot.com/35790044/diff/40001/src/cmd/ld/pobj.c
File src/cmd/ld/pobj.c (right):

https://codereview.appspot.com/35790044/diff/40001/src/cmd/ld/pobj.c#newcode168
src/cmd/ld/pobj.c:168: ctxt->linkmode = linkmode;
On 2013/12/05 02:20:07, iant wrote:
> Setting the linkmode field is also in archinit in 6l/obj.c and 8l/obj.c for
some
> reason.

Fixed.

[iant, 2013-12-05 04:27:31]

LGTM

[rsc, 2013-12-09 03:49:42]

*** Submitted as https://code.google.com/p/go/source/detail?r=00fa28348495 ***

liblink: create new library based on linker code

There is an enormous amount of code moving around in this CL,
but the code is the same, and it is invoked in the same ways.
This CL is preparation for the new linker structure, not the new
structure itself.

The new library's definition is in include/link.h.

The main change is the use of a Link structure to hold all the
linker-relevant state, replacing the smattering of global variables.
The Link structure should both make it clearer which state must
be carried around and make it possible to parallelize more easily
later.

The main body of the linker has moved into the architecture-independent
cmd/ld directory. That includes the list of known header types, so the
distinction between Hplan9x32 and Hplan9x64 is removed (no other
header type distinguished 32- and 64-bit formats), and code for unused
formats such as ipaq kernels has been deleted.

The code being deleted from 5l, 6l, and 8l reappears in liblink or in ld.
Because multiple files are being merged in the liblink directory,
it is not possible to show the diffs nicely in hg.

The Prog and Addr structures have been unified into an
architecture-independent form and moved to link.h, where they will
be shared by all tools: the assemblers, the compilers, and the linkers.
The unification makes it possible to write architecture-independent
traversal of Prog lists, among other benefits.

The Sym structures cannot be unified: they are too fundamentally
different between the linker and the compilers. Instead, liblink defines
an LSym - a linker Sym - to be used in the Prog and Addr structures,
and the linker now refers exclusively to LSyms. The compilers will
keep using their own syms but will fill out the corresponding LSyms in
the Prog and Addr structures.

Although code from 5l, 6l, and 8l is now in a single library, the
code has been arranged so that only one architecture needs to
be linked into a particular program: 5l will not contain the code
needed for x86 instruction layout, for example.

The object file writing code in liblink/obj.c is from cmd/gc/obj.c.

Preparation for golang.org/s/go13linker work.

This CL does not build by itself. It depends on 35740044
and will be submitted at the same time.

R=iant
CC=golang-dev
https://codereview.appspot.com/35790044