code review 7399045: cmd/5g, cmd/5l, cmd/6l, cmd/8l, cmd/gc, cmd/ld, runtime...

cmd/5g, cmd/5l, cmd/6l, cmd/8l, cmd/gc, cmd/ld, runtime: accurate args and locals information

Previously, the func structure contained an inaccurate value for
the args member and a 0 value for the locals member.

This change populates the func structure with args and locals
values computed by the compiler.  The number of args was
already available in the ATEXT instruction.  The number of
locals is now passed through in the new ALOCALS instruction.

This change also switches the unit of args and locals to be
bytes, just like the frame member, instead of 32-bit words.

[cshapiro, 2013-02-21 01:17:45]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-02-21 01:21:46]

On Wed, Feb 20, 2013 at 5:17 PM, <cshapiro@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> cmd/5g, cmd/5l, cmd/6l, cmd/8l, cmd/gc, cmd/ld, runtime: accurate args
> and locals information
>
> Previously, the func structure contained an inaccurate value for
> the args member and a 0 value for the locals member.
>
> This change populates the func structure with args and locals
> values computed by the compiler.  The number of args was
> already available in the ATEXT instruction.  The number of
> locals is not passed through in the new ALOCALS instruction.
>

s/not/now/ ?

[cshapiro1, 2013-02-21 01:25:02]

On 2013/02/21 01:21:46, bradfitz wrote:
> s/not/now/ ?

Fixed.

[dave_cheney.net, 2013-02-21 01:25:59]

Adding more members to {5,6,8}l.Sym makes me worry about the RSS of
the linker. Are there any options to use a union to reduce the cost of
adding those members to Sym ?

On Thu, Feb 21, 2013 at 12:21 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>
> On Wed, Feb 20, 2013 at 5:17 PM, <cshapiro@golang.org> wrote:
>>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go
>>
>>
>> Description:
>> cmd/5g, cmd/5l, cmd/6l, cmd/8l, cmd/gc, cmd/ld, runtime: accurate args
>> and locals information
>>
>> Previously, the func structure contained an inaccurate value for
>> the args member and a 0 value for the locals member.
>>
>> This change populates the func structure with args and locals
>> values computed by the compiler.  The number of args was
>> already available in the ATEXT instruction.  The number of
>> locals is not passed through in the new ALOCALS instruction.
>
>
> s/not/now/ ?
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[cshapiro, 2013-02-21 01:38:05]

On Wed, Feb 20, 2013 at 5:25 PM, Dave Cheney <dave@cheney.net> wrote:

> Adding more members to {5,6,8}l.Sym makes me worry about the RSS of
> the linker. Are there any options to use a union to reduce the cost of
> adding those members to Sym ?
>

This sounds like a valid concern.  However, there appear to be preexisting
limited use members in the Sym structure.

Are you suggesting that the new should overlap their storage with
preexisting members?  (That would not be hard to do but it would touch a
lot of unrelated code.)

[dave_cheney.net, 2013-02-21 01:39:52]

I'm always concerned about memory usage by the compiler/linker, but
you shouldn't let that be a blocker :)

If you want to tackle rearranging the fields in a followup CL that
would be fantastic.

On Thu, Feb 21, 2013 at 12:37 PM, Carl Shapiro <cshapiro@golang.org> wrote:
> On Wed, Feb 20, 2013 at 5:25 PM, Dave Cheney <dave@cheney.net> wrote:
>>
>> Adding more members to {5,6,8}l.Sym makes me worry about the RSS of
>> the linker. Are there any options to use a union to reduce the cost of
>> adding those members to Sym ?
>
>
> This sounds like a valid concern.  However, there appear to be preexisting
> limited use members in the Sym structure.
>
> Are you suggesting that the new should overlap their storage with
> preexisting members?  (That would not be hard to do but it would touch a lot
> of unrelated code.)

[rsc, 2013-02-21 02:37:29]

LGTM

We typically add new instructions to the end of the enumeration.
Please move ALOCALS to the end of the instruction lists, just before ALAST.
That will require moving the line in the optab tables too.

https://codereview.appspot.com/7399045/diff/9001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/7399045/diff/9001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:132: func[nfunc-1].locals = sym->value/4;
Can we drop the /4 and just store as bytes all the time?

[rsc, 2013-02-21 02:38:25]

Please don't worry about the size of Sym in the linker. It's not the
bottleneck today.

Russ

[cshapiro1, 2013-02-21 20:47:29]

https://codereview.appspot.com/7399045/diff/9001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/7399045/diff/9001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:132: func[nfunc-1].locals = sym->value/4;
Gladly.  Fixed.

[cshapiro, 2013-02-21 20:52:29]

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

cmd/5g, cmd/5l, cmd/6l, cmd/8l, cmd/gc, cmd/ld, runtime: accurate args and
locals information

Previously, the func structure contained an inaccurate value for
the args member and a 0 value for the locals member.

This change populates the func structure with args and locals
values computed by the compiler.  The number of args was
already available in the ATEXT instruction.  The number of
locals is now passed through in the new ALOCALS instruction.

This change also switches the unit of args and locals to be
bytes, just like the frame member, instead of 32-bit words.

R=golang-dev, bradfitz, cshapiro, dave, rsc
CC=golang-dev
https://codereview.appspot.com/7399045