code review 142150043: liblink: make GO_ARGS the default for functions beginni...

liblink: make GO_ARGS the default for functions beginning with ·

If there is a leading ·, assume there is a Go prototype and
attach the Go prototype information to the function.
If the function is not called from Go and does not need a
Go prototype, it can be made file-local instead (using name<>(SB)).

This fixes the current BSD build failures, by giving functions like
sync/atomic.StoreUint32 argument stack map information.

Fixes issue 8753.

[rsc, 2014-09-16 17:16:21]

Hello golang-codereviews@googlegroups.com (cc: iant, khr, r, rlh),

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

[iant, 2014-09-16 18:35:47]

https://codereview.appspot.com/142150043/diff/40001/src/liblink/objfile.c
File src/liblink/objfile.c (right):

https://codereview.appspot.com/142150043/diff/40001/src/liblink/objfile.c#new...
src/liblink/objfile.c:272: p->to.sym = linklookup(ctxt,
smprint("%s.args_stackmap", s->name), s->version);
What happens if this symbol is not defined anywhere?  I think I see that it is
supposed to be defined when the Go compiler sees a func declaration, but what
happens if there is no such declaration?

[rsc, 2014-09-16 18:53:56]

https://codereview.appspot.com/142150043/diff/40001/src/liblink/objfile.c
File src/liblink/objfile.c (right):

https://codereview.appspot.com/142150043/diff/40001/src/liblink/objfile.c#new...
src/liblink/objfile.c:272: p->to.sym = linklookup(ctxt,
smprint("%s.args_stackmap", s->name), s->version);
On 2014/09/16 18:35:47, iant wrote:
> What happens if this symbol is not defined anywhere?  I think I see that it is
> supposed to be defined when the Go compiler sees a func declaration, but what
> happens if there is no such declaration?

Then there will be a link-time error about a missing symbol. This will need to
be mentioned in the assembler doc updates (issue 8712). You can avoid this by
not naming the function beginning with a center dot, but I expect that 9 times
out of 10 the right fix is to define a Go prototype. We're coupling the Go
compiler and the assembler but that's been going on for a few releases now with
go vet and such. Assembler has to coordinate with the runtime, there's no way
around it.

[khr, 2014-09-16 19:30:51]

LGTM.

https://codereview.appspot.com/142150043/diff/40001/test/nosplit.go
File test/nosplit.go (right):

https://codereview.appspot.com/142150043/diff/40001/test/nosplit.go#newcode232
test/nosplit.go:232: var gobuf bytes.Buffer
/gobuf/mainbuf/ ?

[bradfitz, 2014-09-16 19:36:51]

https://codereview.appspot.com/142150043/diff/40001/test/nosplit.go
File test/nosplit.go (right):

https://codereview.appspot.com/142150043/diff/40001/test/nosplit.go#newcode288
test/nosplit.go:288: ioutil.WriteFile(filepath.Join(dir, "main.go"),
gobuf.Bytes(), 0666)
missing error checks on both these.

our builders are not always happy machines.

[iant, 2014-09-16 20:51:03]

LGTM

modulo comments from khr and bradfitz.

[rsc, 2014-09-16 21:08:03]

https://codereview.appspot.com/142150043/diff/40001/test/nosplit.go
File test/nosplit.go (right):

https://codereview.appspot.com/142150043/diff/40001/test/nosplit.go#newcode232
test/nosplit.go:232: var gobuf bytes.Buffer
On 2014/09/16 19:30:51, khr wrote:
> /gobuf/mainbuf/ ?

it's the .go output buffer

https://codereview.appspot.com/142150043/diff/40001/test/nosplit.go#newcode288
test/nosplit.go:288: ioutil.WriteFile(filepath.Join(dir, "main.go"),
gobuf.Bytes(), 0666)
On 2014/09/16 19:36:51, bradfitz wrote:
> missing error checks on both these.
> 
> our builders are not always happy machines.

Done.

[rsc, 2014-09-16 21:40:01]

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

liblink: make GO_ARGS the default for functions beginning with ·

If there is a leading ·, assume there is a Go prototype and
attach the Go prototype information to the function.
If the function is not called from Go and does not need a
Go prototype, it can be made file-local instead (using name<>(SB)).

This fixes the current BSD build failures, by giving functions like
sync/atomic.StoreUint32 argument stack map information.

Fixes issue 8753.

LGTM=khr, iant
R=golang-codereviews, iant, khr, bradfitz
CC=golang-codereviews, r, rlh
https://codereview.appspot.com/142150043

[rsc, 2014-09-16 21:45:38]

this fixed the widespread breakage introduced by rob's (innocent) sync CL.
it is wrong for arm. i will fix the arm in a few hours.