code review 11406044: runtime: use funcdata to supply garbage collection info...

runtime: use funcdata to supply garbage collection information

This CL introduces a FUNCDATA number for runtime-specific
garbage collection metadata, changes the C and Go compilers
to emit that metadata, and changes the runtime to expect it.

The old pseudo-instructions that carried this information
are gone, as is the linker code to process them.

[rsc, 2013-07-18 19:20:35]

Hello golang-dev@googlegroups.com,

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

[rsc, 2013-07-18 19:37:40]

I updated the CL to bring the C compiler back to where it was, emitting the
count for the local variable section. Unfortunately, that count is
incorrect, but we'll have to leave that for another day.

Russ

[dvyukov, 2013-07-19 12:15:02]

LGTM for runtime part

[rsc, 2013-07-19 15:22:03]

Carl, do you want to look at this?

Russ

[cshapiro1, 2013-07-19 17:00:39]

I'll take a look at it shortly.


On Fri, Jul 19, 2013 at 8:22 AM, Russ Cox <rsc@golang.org> wrote:

> Carl, do you want to look at this?
>
> Russ
>

[cshapiro1, 2013-07-19 17:42:04]

lgtm to me, just a few minor questions and comments

[cshapiro1, 2013-07-19 17:43:51]

https://codereview.appspot.com/11406044/diff/14001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/11406044/diff/14001/src/cmd/gc/pgen.c#newcode96
src/cmd/gc/pgen.c:96: snprint(namebuf, sizeof namebuf, "gc·%d", ngcsym++);
I am not familiar with the motivation or convention for these names (assuming
they exists) but it would seem preferable to me to have the name of the function
being compiled added to the name rather than an integer.  This would make
locating the metadata in the output simpler as you could search for the gc
symbol directly as opposed to first having to locate the function to find out
the name of its symbol.

https://codereview.appspot.com/11406044/diff/14001/src/cmd/gc/pgen.c#newcode185
src/cmd/gc/pgen.c:185: int ot;
What does ot stand for?

https://codereview.appspot.com/11406044/diff/14001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/11406044/diff/14001/src/pkg/runtime/mgc0.c#new...
src/pkg/runtime/mgc0.c:1408: gcf = (GCFunc*)runtime·funcdata(f, FUNCDATA_GC);
Is this cast needed?

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

https://codereview.appspot.com/11406044/diff/14001/src/pkg/runtime/runtime.h#...
src/pkg/runtime/runtime.h:416: int32	x1;	// locals size
Is there any reason now to leave these x variables in the Func structure?

[rsc, 2013-07-19 17:54:34]

https://codereview.appspot.com/11406044/diff/14001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/11406044/diff/14001/src/cmd/gc/pgen.c#newcode96
src/cmd/gc/pgen.c:96: snprint(namebuf, sizeof namebuf, "gc·%d", ngcsym++);
On 2013/07/19 17:43:52, cshapiro1 wrote:
> I am not familiar with the motivation or convention for these names (assuming
> they exists) but it would seem preferable to me to have the name of the
function
> being compiled added to the name rather than an integer.  This would make
> locating the metadata in the output simpler as you could search for the gc
> symbol directly as opposed to first having to locate the function to find out
> the name of its symbol.

They just need to be unique within this package (they are already package
scoped). In general the function names can get quite long and this seemed
simpler. I should really merge them all into a single symbol in the final symbol
table in the executable anyway, and then it won't matter.

https://codereview.appspot.com/11406044/diff/14001/src/cmd/gc/pgen.c#newcode185
src/cmd/gc/pgen.c:185: int ot;
On 2013/07/19 17:43:52, cshapiro1 wrote:
> What does ot stand for?

The o is offset. I don't know about the t. It's the name that very similar code
in reflect.c uses. Maybe off would be better.

https://codereview.appspot.com/11406044/diff/14001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/11406044/diff/14001/src/pkg/runtime/mgc0.c#new...
src/pkg/runtime/mgc0.c:1408: gcf = (GCFunc*)runtime·funcdata(f, FUNCDATA_GC);
On 2013/07/19 17:43:52, cshapiro1 wrote:
> Is this cast needed?

Not anymore. When I wrote it funcdata returned uintptr not void*.

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

https://codereview.appspot.com/11406044/diff/14001/src/pkg/runtime/runtime.h#...
src/pkg/runtime/runtime.h:416: int32	x1;	// locals size
On 2013/07/19 17:43:52, cshapiro1 wrote:
> Is there any reason now to leave these x variables in the Func structure?

Only that the CL is already pretty large. Deleting these fields requires
changing code here, in the linker, in libmach, and in debug/gosym, all together.
By postponing these to a later CL I avoid having to pull those last two into
this one.

[cshapiro1, 2013-07-19 18:39:10]

sgtm

[rsc, 2013-07-19 20:04:12]

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

runtime: use funcdata to supply garbage collection information

This CL introduces a FUNCDATA number for runtime-specific
garbage collection metadata, changes the C and Go compilers
to emit that metadata, and changes the runtime to expect it.

The old pseudo-instructions that carried this information
are gone, as is the linker code to process them.

R=golang-dev, dvyukov, cshapiro
CC=golang-dev
https://codereview.appspot.com/11406044