code review 48870044: cmd/link: intial skeleton of linker written in Go

cmd/link: intial skeleton of linker written in Go

[rsc, 2014-01-08 05:00:21]

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

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

[iant, 2014-01-08 21:14:58]

LGTM

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/layout.go
File src/cmd/link/layout.go (right):

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/layout.go#new...
src/cmd/link/layout.go:14: // final executable. Go binaries only have a fixed
set of possible
A fixed set of possible sections ignoring contributions from non-Go objects when
using internal linking.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/layout.go#new...
src/cmd/link/layout.go:118: const segAlign = 4096
This alignment is determined by the processor--it's the page size--so ultimately
it needs to come from something processor-specific.  Also someone is eventually
going to want a command line option for it, to support things like hugepages.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/layout.go#new...
src/cmd/link/layout.go:126: addr := p.UnmappedSize
Unless generating a PIE or a shared library.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/load.go
File src/cmd/link/load.go (right):

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/load.go#newco...
src/cmd/link/load.go:18: seg.Data = make([]byte, seg.FileSize)
It's nice to mmap the output file and just store the data in place.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/load.go#newco...
src/cmd/link/load.go:42: _, err := f.ReadAt(data, sym.Data.Offset)
Argh, too slow.  Even before going to mmap it would be a win to read the whole
file into memory and just copy the data.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/load.go#newco...
src/cmd/link/load.go:87: binary.LittleEndian.PutUint32(frag, uint32(val))
// TODO: Bigendian?

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/prog.go
File src/cmd/link/prog.go (right):

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/prog.go#newco...
src/cmd/link/prog.go:116: // TODO(rsc): Provide a way to redirect.
Seems like p.Error is the way to redirect, you just need to call it.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/prog.go#newco...
src/cmd/link/prog.go:171: // goosFormat records the default format each known
GOOS value.
s/format each/format for each/

[rsc, 2014-01-09 22:00:50]

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/layout.go
File src/cmd/link/layout.go (right):

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/layout.go#new...
src/cmd/link/layout.go:14: // final executable. Go binaries only have a fixed
set of possible
On 2014/01/08 21:14:58, iant wrote:
> A fixed set of possible sections ignoring contributions from non-Go objects
when
> using internal linking.

Actually, it's just a fixed set of possible sections. Even contributions from
non-Go objects end up in these sections, not in their own sections. At least
that's what the current linker does.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/layout.go#new...
src/cmd/link/layout.go:118: const segAlign = 4096
On 2014/01/08 21:14:58, iant wrote:
> This alignment is determined by the processor--it's the page size--so
ultimately
> it needs to come from something processor-specific.  Also someone is
eventually
> going to want a command line option for it, to support things like hugepages.

Done.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/layout.go#new...
src/cmd/link/layout.go:126: addr := p.UnmappedSize
On 2014/01/08 21:14:58, iant wrote:
> Unless generating a PIE or a shared library.

Done.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/load.go
File src/cmd/link/load.go (right):

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/load.go#newco...
src/cmd/link/load.go:18: seg.Data = make([]byte, seg.FileSize)
On 2014/01/08 21:14:58, iant wrote:
> It's nice to mmap the output file and just store the data in place.

Done (added TODO).

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/load.go#newco...
src/cmd/link/load.go:42: _, err := f.ReadAt(data, sym.Data.Offset)
On 2014/01/08 21:14:58, iant wrote:
> Argh, too slow.  Even before going to mmap it would be a win to read the whole
> file into memory and just copy the data.

Added two TODOs.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/load.go#newco...
src/cmd/link/load.go:87: binary.LittleEndian.PutUint32(frag, uint32(val))
On 2014/01/08 21:14:58, iant wrote:
> // TODO: Bigendian?

Done.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/prog.go
File src/cmd/link/prog.go (right):

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/prog.go#newco...
src/cmd/link/prog.go:116: // TODO(rsc): Provide a way to redirect.
On 2014/01/08 21:14:58, iant wrote:
> Seems like p.Error is the way to redirect, you just need to call it.

Done.

https://codereview.appspot.com/48870044/diff/20001/src/cmd/link/prog.go#newco...
src/cmd/link/prog.go:171: // goosFormat records the default format each known
GOOS value.
On 2014/01/08 21:14:58, iant wrote:
> s/format each/format for each/

Done.

[rsc, 2014-01-10 00:29:19]

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

cmd/link: intial skeleton of linker written in Go

R=iant
CC=golang-codereviews
https://codereview.appspot.com/48870044

[u, 2014-01-11 13:44:28]

The skeleton structure for the new linker is really clean. Thank you very much
for starting this work!

https://codereview.appspot.com/48870044/diff/40001/src/cmd/link/layout.go
File src/cmd/link/layout.go (right):

https://codereview.appspot.com/48870044/diff/40001/src/cmd/link/layout.go#new...
src/cmd/link/layout.go:27: {Segment: "text", Section: "text", Kind:
goobj.STEXT},
It seems like Section is always a string interpretation of Kind. Would it be
possible to redefine the String method of goobj.SymKind to return "text", "data"
instead of "STEXT", "SDATA", etc?

https://codereview.appspot.com/48870044/diff/40001/src/cmd/link/layout.go#new...
src/cmd/link/layout.go:46: var layoutByKind []*layoutSection
Are lookup time in layoutByKind a bottle neck? If not a map from goobj.SymKind
to *layoutSection would be intuitive and simplify code such as "if kind < 0 ||
int(kind) >= len(layoutByKind) || layoutByKind[kind] == nil {" to "if
layoutByKind[kind] == nil {". It would also simplify init.

https://codereview.appspot.com/48870044/diff/40001/src/cmd/link/scan.go
File src/cmd/link/scan.go (right):

https://codereview.appspot.com/48870044/diff/40001/src/cmd/link/scan.go#newco...
src/cmd/link/scan.go:84: if p.Syms[r.Sym] != nil {
p.Syms is a map from SymID to *Sym. My suggestion is to rename the Sym member of
goobj.Reloc to SymID. This would make it consistent with "p.Syms[gs.SymID]". If
such a change is made goobj.FuncData should also use SymID instead of Sym.