code review 35980043: liblink, cmd/ld, cmd/6l, cmd/8l: add support for GOOS=s...

liblink, cmd/ld, cmd/6l, cmd/8l: add support for GOOS=solaris

[aram, 2013-12-10 20:39:45]

Hello golang-dev@googlegroups.com,

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

[jsing, 2013-12-11 14:08:31]

https://codereview.appspot.com/35980043/diff/170001/src/cmd/6l/obj.c
File src/cmd/6l/obj.c (right):

https://codereview.appspot.com/35980043/diff/170001/src/cmd/6l/obj.c#newcode115
src/cmd/6l/obj.c:115: case Hsolaris:	/* solaris */
Please align comment with above lines.

[rsc, 2013-12-11 15:15:05]

Thanks but the C part of the tree is still frozen. There are more linker
changes on the way. I'd rather wait until they are done before starting to
merge significant amounts of new code.

Russ

[aram, 2013-12-11 15:22:31]

On Wed, Dec 11, 2013 at 4:15 PM, Russ Cox <rsc@golang.org> wrote:
> Thanks but the C part of the tree is still frozen. There are more linker
> changes on the way. I'd rather wait until they are done before starting to
> merge significant amounts of new code.

Thanks, I uploaded these CLs to give people time to review them, they
don't have to get in right now. They can wait, and I will update them
as the linker changes come in making sure they always work.

That being said, there's very little code in this CL. Many files, but
most changes are trivial (add Hsolaris in case statements), only two
changes are more significant.

Thanks,

-- 
Aram Hăvărneanu

[minux1, 2013-12-13 07:11:25]

it seems we have to wait more time before we can
submit changes to the linker. so i will also defer
reviewing the code until then.

https://codereview.appspot.com/35980043/diff/210001/src/cmd/ld/doc.go
File src/cmd/ld/doc.go (right):

https://codereview.appspot.com/35980043/diff/210001/src/cmd/ld/doc.go#newcode46
src/cmd/ld/doc.go:46: -H solaris (only in 6l)
more spaces before left paren.

[gobot, 2013-12-20 16:26:11]

Replacing golang-dev with golang-codereviews.

[aram, 2013-12-21 21:33:08]

https://codereview.appspot.com/35980043/diff/170001/src/cmd/6l/obj.c
File src/cmd/6l/obj.c (right):

https://codereview.appspot.com/35980043/diff/170001/src/cmd/6l/obj.c#newcode115
src/cmd/6l/obj.c:115: case Hsolaris:	/* solaris */
On 2013/12/11 14:08:31, jsing wrote:
> Please align comment with above lines.

Done.

https://codereview.appspot.com/35980043/diff/210001/src/cmd/ld/doc.go
File src/cmd/ld/doc.go (right):

https://codereview.appspot.com/35980043/diff/210001/src/cmd/ld/doc.go#newcode46
src/cmd/ld/doc.go:46: -H solaris (only in 6l)
On 2013/12/13 07:11:25, minux wrote:
> more spaces before left paren.

Done.

[aram, 2013-12-29 17:44:24]

Hello golang-codereviews@googlegroups.com, jsing@google.com, rsc@golang.org,
minux.ma@gmail.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[gobot, 2014-01-07 11:13:35]

R=rsc@golang.org (assigned by aram@mgk.ro)

[jsing, 2014-01-07 13:28:46]

R=rsc

LGTM, but wait for rsc or iant due to the more intrusive linker changes.

https://codereview.appspot.com/35980043/diff/310001/src/cmd/ld/symtab.c
File src/cmd/ld/symtab.c (right):

https://codereview.appspot.com/35980043/diff/310001/src/cmd/ld/symtab.c#newco...
src/cmd/ld/symtab.c:201: putelfsyment(putelfstr(s->type == SDYNIMPORT ?
s->extname:s->name), 0, 0, (STB_GLOBAL<<4)|STT_NOTYPE, 0, 0);
Please add spaces before and after colon.

[aram, 2014-01-09 15:09:02]

https://codereview.appspot.com/35980043/diff/310001/src/cmd/ld/symtab.c
File src/cmd/ld/symtab.c (right):

https://codereview.appspot.com/35980043/diff/310001/src/cmd/ld/symtab.c#newco...
src/cmd/ld/symtab.c:201: putelfsyment(putelfstr(s->type == SDYNIMPORT ?
s->extname:s->name), 0, 0, (STB_GLOBAL<<4)|STT_NOTYPE, 0, 0);
On 2014/01/07 13:28:46, jsing wrote:
> Please add spaces before and after colon.

Done.

[aram, 2014-01-09 15:09:54]

Hello golang-codereviews@googlegroups.com, jsing@google.com, rsc@golang.org,
minux.ma@gmail.com, gobot@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[iant, 2014-01-10 22:29:14]

Partial look.

https://codereview.appspot.com/35980043/diff/350001/src/cmd/6l/asm.c
File src/cmd/6l/asm.c (right):

https://codereview.appspot.com/35980043/diff/350001/src/cmd/6l/asm.c#newcode210
src/cmd/6l/asm.c:210: if(s->type == STEXT) {
The notion of a GOT is ELF-specific.  Shouldn't this be if(s->type == STEXT &&
iself) { ?

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

https://codereview.appspot.com/35980043/diff/350001/src/cmd/ld/data.c#newcode215
src/cmd/ld/data.c:215: if(rs->type != SDYNIMPORT)
Might as well write this condition the same as line 183.

[aram, 2014-01-11 14:11:39]

Hello golang-codereviews@googlegroups.com, jsing@google.com, rsc@golang.org,
minux.ma@gmail.com, gobot@golang.org, iant@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[aram, 2014-01-11 14:11:44]

https://codereview.appspot.com/35980043/diff/350001/src/cmd/6l/asm.c
File src/cmd/6l/asm.c (right):

https://codereview.appspot.com/35980043/diff/350001/src/cmd/6l/asm.c#newcode210
src/cmd/6l/asm.c:210: if(s->type == STEXT) {
On 2014/01/10 22:29:14, iant wrote:
> The notion of a GOT is ELF-specific.  Shouldn't this be if(s->type == STEXT &&
> iself) { ?

Done.

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

https://codereview.appspot.com/35980043/diff/350001/src/cmd/ld/data.c#newcode215
src/cmd/ld/data.c:215: if(rs->type != SDYNIMPORT)
On 2014/01/10 22:29:14, iant wrote:
> Might as well write this condition the same as line 183.

Done.

[iant, 2014-01-17 15:01:55]

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/data.c
File src/cmd/ld/data.c (left):

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/data.c#oldcode157
src/cmd/ld/data.c:157: if(r->sym != S && r->sym->type == SDYNIMPORT)
As far as I can tell, there are a number of cases where this reloc is still
unhandled.  I think you are only handling it for the specific case of amd64 ELF
systems.  If you drop the error here, we need to make sure that some error
occurs for the other cases where this is not handled, rather than generating
incorrect code.

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/elf.c
File src/cmd/ld/elf.c (right):

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/elf.c#newcode682
src/cmd/ld/elf.c:682: // DT_JMPREL is emitted so we have to defer generation of
these
This comment would be more appropriate in doelf.  It talks about deferring
generation, but that is done in doelf, not here.

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/elf.c#newcode1208
src/cmd/ld/elf.c:1208: interpreter = solarisdynld;
Does this break the ARM build?

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/symtab.c
File src/cmd/ld/symtab.c (right):

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/symtab.c#newco...
src/cmd/ld/symtab.c:201: putelfsyment(putelfstr(s->type == SDYNIMPORT ?
s->extname : s->name), 0, 0, (STB_GLOBAL<<4)|STT_NOTYPE, 0, 0);
Use a local variable rather than ?: in a long line.

https://codereview.appspot.com/35980043/diff/420001/src/liblink/asm6.c
File src/liblink/asm6.c (left):

https://codereview.appspot.com/35980043/diff/420001/src/liblink/asm6.c#oldcod...
src/liblink/asm6.c:2360: r->add = a->offset - ctxt->tlsoffset;
Why is it OK to remove these lines?

https://codereview.appspot.com/35980043/diff/420001/src/liblink/asm6.c
File src/liblink/asm6.c (right):

https://codereview.appspot.com/35980043/diff/420001/src/liblink/asm6.c#newcod...
src/liblink/asm6.c:2340: if(v >= -128 && v < 128 && a->index != D_FS) {
What does this change do?

[rsc, 2014-01-22 21:20:14]

Can you split this into multiple CLs? Let's do one that just deals with all the
Hsolaris stuff. And then one that deals with the relocations. And one that deals
with D_FS. There is too much subtle stuff here, and I am worried about the
review missing things because it is so big. All the points Ian brought up are
troubling.

https://codereview.appspot.com/35980043/diff/440001/src/cmd/6l/asm.c
File src/cmd/6l/asm.c (right):

https://codereview.appspot.com/35980043/diff/440001/src/cmd/6l/asm.c#newcode218
src/cmd/6l/asm.c:218: } else if(s->type != SDATA)
no else; the if body ends in return so the else is redundant.
just "if"

https://codereview.appspot.com/35980043/diff/440001/src/cmd/6l/asm.c#newcode284
src/cmd/6l/asm.c:284: if(r->siz == 4)
please put { } here to enclose the multiline body.

[minux1, 2014-01-23 08:30:10]

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/data.c
File src/cmd/ld/data.c (left):

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/data.c#oldcode157
src/cmd/ld/data.c:157: if(r->sym != S && r->sym->type == SDYNIMPORT)
On 2014/01/17 15:01:56, iant wrote:
> As far as I can tell, there are a number of cases where this reloc is still
> unhandled.  I think you are only handling it for the specific case of amd64
ELF
> systems.  If you drop the error here, we need to make sure that some error
> occurs for the other cases where this is not handled, rather than generating
> incorrect code.
Yeah, we probably should restore the error to all non solaris/amd64 hosts.

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/elf.c
File src/cmd/ld/elf.c (right):

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/elf.c#newcode682
src/cmd/ld/elf.c:682: // DT_JMPREL is emitted so we have to defer generation of
these
On 2014/01/17 15:01:56, iant wrote:
> This comment would be more appropriate in doelf.  It talks about deferring
> generation, but that is done in doelf, not here.
Agreed.

https://codereview.appspot.com/35980043/diff/420001/src/cmd/ld/elf.c#newcode1208
src/cmd/ld/elf.c:1208: interpreter = solarisdynld;
On 2014/01/17 15:01:56, iant wrote:
> Does this break the ARM build?
It will. We need to also define a dummy solarisdynld in cmd/5l/asm.c

https://codereview.appspot.com/35980043/diff/420001/src/liblink/asm6.c
File src/liblink/asm6.c (left):

https://codereview.appspot.com/35980043/diff/420001/src/liblink/asm6.c#oldcod...
src/liblink/asm6.c:2360: r->add = a->offset - ctxt->tlsoffset;
On 2014/01/17 15:01:56, iant wrote:
> Why is it OK to remove these lines?
I think it's probably a merge error, we should restore these two lines.

https://codereview.appspot.com/35980043/diff/420001/src/liblink/asm6.c
File src/liblink/asm6.c (right):

https://codereview.appspot.com/35980043/diff/420001/src/liblink/asm6.c#newcod...
src/liblink/asm6.c:2340: if(v >= -128 && v < 128 && a->index != D_FS) {
On 2014/01/17 15:01:56, iant wrote:
> What does this change do?
I wrote this line, it's to recognize and special case (FS*1)
on solaris at that time, however, it's not needed anymore.

(FTR,
https://bitbucket.org/minux/go-sunos/commits/8271a5b96f7d6f680904727043bc6167...)

[gobot, 2014-02-12 11:18:30]

R=close (assigned by aram@mgk.ro)