code review 11085043: cmd/ld, runtime: new in-memory symbol table format

cmd/ld, runtime: new in-memory symbol table format

Design at http://golang.org/s/go12symtab.

This enables some cleanup of the garbage collector metadata
that will be done in future CLs.

This CL does not move the old symtab and pclntab back into
an unmapped section of the file. That's a bit tricky and will be
done separately.

Fixes issue 4020.

[rsc, 2013-07-11 03:18:40]

Hello golang-dev@googlegroups.com,

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

[rsc, 2013-07-11 03:19:12]

As usual there is something not 100% right on arm. 386 and amd64 are fine.

[nigeltao, 2013-07-11 05:12:50]

https://codereview.appspot.com/11085043/diff/11001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/11085043/diff/11001/src/cmd/ld/lib.c#newcode1370
src/cmd/ld/lib.c:1370: if(0 && debug['O'])
Revert.

Similarly below.

[rsc, 2013-07-12 03:53:28]

PTAL

Now arm works a lot better, though still not 100%. Funny story: 5l inserts calls
to special routines called _div, _divu, _mod, and _modu to implement the DIV and
MOD pseudo-instructions. Those calls used to be mentioned by the arm traceback
routines, because the frame size was slightly off during those calls. Now that
the frame offset is recorded correctly, that special case is gone from the
traceback, so nothing was holding up those routines to keep them in the binary,
yet the linker was still trying to use them. Now it knows to keep them from the
start.

[rsc, 2013-07-12 04:27:49]

PTAL

arm is at 100% now. The final problem was the traceback didn't know to stop
at runtime.mcall, which can be at the top of an m stack.

[dave_cheney.net, 2013-07-13 14:36:09]

On 2013/07/12 04:27:49, rsc wrote:
> PTAL
> 
> arm is at 100% now. The final problem was the traceback didn't know to stop
> at runtime.mcall, which can be at the top of an m stack.

Compilation failure on my arm chromebook.

pkg/runtime
warning: /home/dfc/go/src/pkg/runtime/traceback_arm.c:99 format mismatch S IND
STRUCT String, arg 2
/home/dfc/go/src/pkg/runtime/traceback_arm.c:210 not a member of struct/union:
src
warning: /home/dfc/go/src/pkg/runtime/traceback_arm.c:209 set and not used: line
go tool dist: FAILED: /home/dfc/go/pkg/tool/linux_arm/5c -F -V -w -I $WORK -I
/home/dfc/go/pkg/linux_arm -D GOOS_linux -D GOARCH_arm -o $WORK/traceback_arm.o
/home/dfc/go/src/pkg/runtime/traceback_arm.c

[dave_cheney.net, 2013-07-15 06:03:05]

On 2013/07/13 14:36:09, dfc wrote:
> On 2013/07/12 04:27:49, rsc wrote:
> > PTAL
> > 
> > arm is at 100% now. The final problem was the traceback didn't know to stop
> > at runtime.mcall, which can be at the top of an m stack.
> 
> Compilation failure on my arm chromebook.
> 
> pkg/runtime
> warning: /home/dfc/go/src/pkg/runtime/traceback_arm.c:99 format mismatch S IND
> STRUCT String, arg 2
> /home/dfc/go/src/pkg/runtime/traceback_arm.c:210 not a member of struct/union:
> src
> warning: /home/dfc/go/src/pkg/runtime/traceback_arm.c:209 set and not used:
line
> go tool dist: FAILED: /home/dfc/go/pkg/tool/linux_arm/5c -F -V -w -I $WORK -I
> /home/dfc/go/pkg/linux_arm -D GOOS_linux -D GOARCH_arm -o
$WORK/traceback_arm.o
> /home/dfc/go/src/pkg/runtime/traceback_arm.c

The compilation errors were just debug prints, after fixing them ./all.bash
built find on linux/arm

[rsc, 2013-07-15 21:04:49]

Ping.

[dave_cheney.net, 2013-07-15 23:44:36]

Two compile errors on arm. Sorry I cannot review further.

https://codereview.appspot.com/11085043/diff/23001/src/pkg/runtime/traceback_...
File src/pkg/runtime/traceback_arm.c (right):

https://codereview.appspot.com/11085043/diff/23001/src/pkg/runtime/traceback_...
src/pkg/runtime/traceback_arm.c:99: runtime·printf("runtime: unknown argument
frame size for %S\n", f->name);
s/f->name/*f->name/

https://codereview.appspot.com/11085043/diff/23001/src/pkg/runtime/traceback_...
src/pkg/runtime/traceback_arm.c:210: runtime·printf("\t%S:%d", f->src, line);
I think has to be *f->src as well.

[cshapiro1, 2013-07-15 23:49:22]

https://codereview.appspot.com/11085043/diff/23001/src/cmd/5l/5.out.h
File src/cmd/5l/5.out.h (right):

https://codereview.appspot.com/11085043/diff/23001/src/cmd/5l/5.out.h#newcode204
src/cmd/5l/5.out.h:204: // TODO: Remove these.
Why isn't the ATYPE value in this list?  Does anything use it now?  It is not
needed for GC.

https://codereview.appspot.com/11085043/diff/23001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/11085043/diff/23001/src/pkg/runtime/runtime.h#...
src/pkg/runtime/runtime.h:417: int32	pcsp;
Having comments for all of these members would be helpful.

[iant, 2013-07-15 23:59:07]

LGTM

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/go.c
File src/cmd/ld/go.c (right):

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/go.c#newcode804
src/cmd/ld/go.c:804: // NOTE: Keep in sync with ../gc/go.c:/^Zconv.
s/go.c/fmt.c/

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode1459
src/cmd/ld/lib.c:1459: pctofileline(Sym *sym, int32 oldval, Prog *p, int32
phase, int32 arg)
Would it be consistent with the commenting practices of this program to have a
comment explaining the phase and arg parameters?

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode1492
src/cmd/ld/lib.c:1492: return p->to.offset;
Check for overflow?

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode2425
src/cmd/ld/lib.c:2425: npcdata = p->from.offset+1;
Check for overflow?

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode2558
src/cmd/ld/lib.c:2558: setaddrplus(ftab, f->value*PtrSize, defgostring(f->name),
0);
Can just use setaddr.

[r, 2013-07-16 00:23:42]

code seems fine but commentary is required

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode1459
src/cmd/ld/lib.c:1459: pctofileline(Sym *sym, int32 oldval, Prog *p, int32
phase, int32 arg)
ha ha you jester

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode1492
src/cmd/ld/lib.c:1492: return p->to.offset;
On 2013/07/15 23:59:08, iant wrote:
> Check for overflow?

previous functions too?

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode2170
src/cmd/ld/lib.c:2170: struct Hist
some commentary about the format, the location in the object file, the decoding,
how it works in memory, etc. would be most welcome

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode2389
src/cmd/ld/lib.c:2389: return setuint32(f, off, start);
overflow?

[rsc, 2013-07-16 00:58:13]

Hello golang-dev@googlegroups.com, dave@cheney.net, cshapiro@google.com,
iant@golang.org, r@golang.org (cc: golang-dev@googlegroups.com,
nigeltao@golang.org),

Please take another look.

[rsc, 2013-07-16 00:58:34]

PTAL

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode1459
src/cmd/ld/lib.c:1459: pctofileline(Sym *sym, int32 oldval, Prog *p, int32
phase, int32 arg)
On 2013/07/15 23:59:08, iant wrote:
> Would it be consistent with the commenting practices of this program to have a
> comment explaining the phase and arg parameters?

Done.

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode1492
src/cmd/ld/lib.c:1492: return p->to.offset;
On 2013/07/15 23:59:08, iant wrote:
> Check for overflow?

Done.

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode2170
src/cmd/ld/lib.c:2170: struct Hist
On 2013/07/16 00:23:43, r wrote:
> some commentary about the format, the location in the object file, the
decoding,
> how it works in memory, etc. would be most welcome

Done.

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode2389
src/cmd/ld/lib.c:2389: return setuint32(f, off, start);
On 2013/07/16 00:23:43, r wrote:
> overflow?

Done.

https://codereview.appspot.com/11085043/diff/23001/src/cmd/ld/lib.c#newcode2558
src/cmd/ld/lib.c:2558: setaddrplus(ftab, f->value*PtrSize, defgostring(f->name),
0);
On 2013/07/15 23:59:08, iant wrote:
> Can just use setaddr.

Done.

[rsc, 2013-07-16 01:00:56]

https://codereview.appspot.com/11085043/diff/23001/src/cmd/5l/5.out.h
File src/cmd/5l/5.out.h (right):

https://codereview.appspot.com/11085043/diff/23001/src/cmd/5l/5.out.h#newcode204
src/cmd/5l/5.out.h:204: // TODO: Remove these.
On 2013/07/15 23:49:22, cshapiro1 wrote:
> Why isn't the ATYPE value in this list?  Does anything use it now?  It is not
> needed for GC.

TYPE instructions are the only ones that specify types for local variables.
Without them, the DWARF generation code will not have the right information to
send to gdb.

https://codereview.appspot.com/11085043/diff/23001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/11085043/diff/23001/src/pkg/runtime/runtime.h#...
src/pkg/runtime/runtime.h:417: int32	pcsp;
On 2013/07/15 23:49:22, cshapiro1 wrote:
> Having comments for all of these members would be helpful.

I added a link to the doc.

[r, 2013-07-16 04:24:00]

LGTM

https://codereview.appspot.com/11085043/diff/54001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/11085043/diff/54001/src/cmd/ld/lib.c#newcode1546
src/cmd/ld/lib.c:1546: // transmitted in zig-zag form, where the sign bit is
placed in bit 0,
it's not the sign bit, it's the complement bit, isn't it?
otherwise it would save no space.

[rsc, 2013-07-16 13:41:50]

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

cmd/ld, runtime: new in-memory symbol table format

Design at http://golang.org/s/go12symtab.

This enables some cleanup of the garbage collector metadata
that will be done in future CLs.

This CL does not move the old symtab and pclntab back into
an unmapped section of the file. That's a bit tricky and will be
done separately.

Fixes issue 4020.

R=golang-dev, dave, cshapiro, iant, r
CC=golang-dev, nigeltao
https://codereview.appspot.com/11085043

[brainman, 2013-07-17 01:40:02]

FYI. This broke windows build

# ..\misc\cgo\test
# testmain
history stack phase error: bad offset in pop
FAIL	_/c_/gobuilder/windows-amd64-8b12d6f12155/go/misc/cgo/test [build failed]
Build complete, duration 8m55.837s. Result: error: exit status 1

Alex