code review 6281048: ld: add .gcdata and .gcbss sections

ld: add .gcdata and .gcbss sections

[atom, 2012-06-04 12:39:18]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-06-11 12:41:10]

On Mon, Jun 11, 2012 at 8:33 AM, ⚛ <0xe2.0x9a.0x9b@gmail.com> wrote:
> I would like to know about the progress of this codereview. Thanks.

I haven't forgotten about it. I just have other work pending too (much
of which is not visible on the mailing list) and I haven't had time to
finish this review yet. Thanks for your patience.

Russ

[atom, 2012-06-18 14:04:11]

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

Please take another look.

[rsc, 2012-08-05 21:39:28]

This looks good but it needs to stand alone before I can submit it.

cmd/6l
/Users/rsc/g/go/src/cmd/6l/../ld/data.c:37:36: error: ../../pkg/runtime/mgc0.h:
No such file or directory
/Users/rsc/g/go/src/cmd/6l/../ld/data.c: In function 'gcaddsym':
/Users/rsc/g/go/src/cmd/6l/../ld/data.c:873: error: 'GC_CALL' undeclared (first
use in this function)
/Users/rsc/g/go/src/cmd/6l/../ld/data.c:873: error: (Each undeclared identifier
is reported only once
/Users/rsc/g/go/src/cmd/6l/../ld/data.c:873: error: for each function it appears
in.)
/Users/rsc/g/go/src/cmd/6l/../ld/data.c:879: error: 'GC_APTR' undeclared (first
use in this function)
/Users/rsc/g/go/src/cmd/6l/../ld/data.c: In function 'dodata':
/Users/rsc/g/go/src/cmd/6l/../ld/data.c:1005: error: 'GC_END' undeclared (first
use in this function)
go tool dist: FAILED: gcc -Wall -Wno-sign-compare -Wno-missing-braces
-Wno-parentheses -Wno-unknown-pragmas -Wno-switch -Wno-comment -Werror
-fno-common -ggdb -O2 -c -m64 -I /Users/rsc/g/go/include -I
/Users/rsc/g/go/src/cmd/6l -o /Users/rsc/g/go/pkg/obj/cmd/6l/data.o
/Users/rsc/g/go/src/cmd/6l/../ld/data.c

http://codereview.appspot.com/6281048/diff/4001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

http://codereview.appspot.com/6281048/diff/4001/src/cmd/ld/data.c#newcode847
src/cmd/ld/data.c:847: int32 t = alignsymsize(s->size);
Please declare variables at top of function.

http://codereview.appspot.com/6281048/diff/4001/src/cmd/ld/data.c#newcode870
src/cmd/ld/data.c:870: Sym *gotype = s->gotype;
Please declare variables at top of function.

[atom, 2012-09-12 17:48:12]

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

Please take another look.

[atom, 2012-09-12 17:48:36]

https://codereview.appspot.com/6281048/diff/4001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

https://codereview.appspot.com/6281048/diff/4001/src/cmd/ld/data.c#newcode847
src/cmd/ld/data.c:847: int32 t = alignsymsize(s->size);
On 2012/08/05 21:39:28, rsc wrote:
> Please declare variables at top of function.

Done.

https://codereview.appspot.com/6281048/diff/4001/src/cmd/ld/data.c#newcode870
src/cmd/ld/data.c:870: Sym *gotype = s->gotype;
On 2012/08/05 21:39:28, rsc wrote:
> Please declare variables at top of function.

Done.

[rsc, 2012-09-13 18:49:10]

LGTM

[rsc, 2012-09-13 19:59:37]

*** Submitted as http://code.google.com/p/go/source/detail?r=910a3f16c063 ***

ld: add .gcdata and .gcbss sections

R=rsc
CC=golang-dev
http://codereview.appspot.com/6281048

Committer: Russ Cox <rsc@golang.org>

[rsc, 2012-09-14 03:08:47]

Any guesses about the 386 build breakages?
I can't reproduce the darwin one locally. I suspect ld/decodesym.c is not quite
right for 32-bit (I don't think dwarf gets used for windows or arm) but I cannot
reproduce it, nor can I see the problem.

It is possible that the StructType and its fields are not always contiguous in
memory, and that decodetype_structfieldname and decodetype_structfieldtype need
to load the pointer from the slice header and then walk from there.

[atom, 2012-09-14 11:19:22]

On 2012/09/14 03:08:47, rsc wrote:
> Any guesses about the 386 build breakages?
> I can't reproduce the darwin one locally. I suspect ld/decodesym.c is not
quite
> right for 32-bit (I don't think dwarf gets used for windows or arm) but I
cannot
> reproduce it, nor can I see the problem.

I am unable to reproduce the issue on my linux/386 machine.

[dave_cheney.net, 2012-09-14 11:58:58]

I can reproduce the problem on ubuntu 12.04.1, gcc 4.6.3, ld 2.22. This is
an amd64 host cross compiling with GOARCH=386.

On Fri, Sep 14, 2012 at 9:19 PM, <0xE2.0x9A.0x9B@gmail.com> wrote:

> On 2012/09/14 03:08:47, rsc wrote:
>
>> Any guesses about the 386 build breakages?
>> I can't reproduce the darwin one locally. I suspect ld/decodesym.c is
>>
> not quite
>
>> right for 32-bit (I don't think dwarf gets used for windows or arm)
>>
> but I cannot
>
>> reproduce it, nor can I see the problem.
>>
>
> I am unable to reproduce the issue on my linux/386 machine.
>
>
https://codereview.appspot.**com/6281048/<https://codereview.appspot.com/6281...
>

[rsc, 2012-09-14 12:20:27]

Aha.

Usually when I build for 386 on my 64-bit machine I use GOARCH=386
GOHOSTARCH=386. But if you only do GOARCH=386 (GOHOSTARCH=amd64 by
default) then you get the failure.

Russ

[dave_cheney.net, 2012-09-14 13:01:38]

I believe this is how the builders build, ie, they do not set GOHOST*

On Fri, Sep 14, 2012 at 10:20 PM, Russ Cox <rsc@golang.org> wrote:

> Aha.
>
> Usually when I build for 386 on my 64-bit machine I use GOARCH=386
> GOHOSTARCH=386. But if you only do GOARCH=386 (GOHOSTARCH=amd64 by
> default) then you get the failure.
>
> Russ
>

[atom, 2012-09-15 08:06:25]

On 2012/09/14 12:20:27, rsc wrote:
> Aha.
> 
> Usually when I build for 386 on my 64-bit machine I use GOARCH=386
> GOHOSTARCH=386. But if you only do GOARCH=386 (GOHOSTARCH=amd64 by
> default) then you get the failure.
> 
> Russ

Fix: https://codereview.appspot.com/6493123/

[rsc, 2012-09-17 20:44:14]

Thanks!

[minux1, 2012-09-20 17:33:44]

Hi,

With this change, Linux/arm binaries are much bigger (size doubled at
least).
For example, .gcbss section of go_bootstrap alone is 8MiB.
Is this expected?

FYI, you can just do this on linux to verify the problem without an arm
machine:
GOARCH=arm GOHOSTARCH=arm ./make.bash
ignore the build error and look at the size of go_bootstrap.

[0xe2.0x9a.0x9b_gmail.com, 2012-09-20 18:20:44]

On Thu, Sep 20, 2012 at 7:33 PM, minux <minux.ma@gmail.com> wrote:
> Hi,
>
> With this change, Linux/arm binaries are much bigger (size doubled at
> least).
> For example, .gcbss section of go_bootstrap alone is 8MiB.
> Is this expected?
>
> FYI, you can just do this on linux to verify the problem without an arm
> machine:
> GOARCH=arm GOHOSTARCH=arm ./make.bash
> ignore the build error and look at the size of go_bootstrap.

The cause of the problem is that .bss section on ARM is substantially
larger than .bss on 386 (see the two attached files). Most of the data
in ARM's .bss section should be in .noptrbss section, like on 386.

[minux1, 2012-09-21 05:39:05]

On Friday, September 21, 2012, ⚛ <0xe2.0x9a.0x9b@gmail.com> wrote:
>
> The cause of the problem is that .bss section on ARM is substantially
> larger than .bss on 386 (see the two attached files). Most of the data
> in ARM's .bss section should be in .noptrbss section, like on 386.
>
Yeah, somehow runtime.mheap is placed in .bss instead of .noptrbss,
could be a regression. I will investigate it.

BTW, 8 MiB of GC data for a ~4MiB struct seems not very memory
efficient. Is this expected?

[atom, 2012-09-21 06:19:35]

On 2012/09/21 05:39:05, minux wrote:
> BTW, 8 MiB of GC data for a ~4MiB struct seems not very memory
> efficient. Is this expected?

One reason why the GC data consumed so much space is that all integers (even
small ones) need to be aligned to a word boundary because of performance
reasons.

It is possible that the encoding can be made more space efficient. I am now
trying to split CL 6114046 into smaller parts and send those parts for inclusion
in Go. I have decided not to attempt any further optimizations until the whole
CL 6114046 is submitted. My 4 month old comment in
https://codereview.appspot.com/6114046/#msg16 says that "There will be no
changes to this patch set, ..." - I am strictly holding to this rule even if it
partially seems irrational to do so.

[minux1, 2012-09-21 07:15:55]

On Fri, Sep 21, 2012 at 2:19 PM, <0xE2.0x9A.0x9B@gmail.com> wrote:

> On 2012/09/21 05:39:05, minux wrote:
>
>> BTW, 8 MiB of GC data for a ~4MiB struct seems not very memory
>> efficient. Is this expected?
>
> One reason why the GC data consumed so much space is that all integers
> (even small ones) need to be aligned to a word boundary because of
> performance reasons.
>
i see.

>
> It is possible that the encoding can be made more space efficient.

hope you have plan for this :)
because i have a project where memory is a very scarce resource so
i'd really like to use your precise GC enhancements.

thank you for your wonderful work.