code review 88190043: liblink, cmd/ld: reenable nosplit checking and test

liblink, cmd/ld: reenable nosplit checking and test

The new code is adapted from the Go 1.2 nosplit code,
but it does not have the bug reported in issue 7623:

g% go run nosplit.go
g% go1.2 run nosplit.go
BUG
rejected incorrectly:
        main 0 call f; f 120

        linker output:
        # _/tmp/go-test-nosplit021064539
        main.main: nosplit stack overflow
                120	guaranteed after split check in main.main
                112	on entry to main.f
                -8	after main.f uses 120

g%

Fixes issue 6931.
Fixes issue 7623.

[rsc, 2014-04-16 21:06:12]

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

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

[iant, 2014-04-16 21:29:08]

https://codereview.appspot.com/88190043/diff/70001/include/link.h
File include/link.h (right):

https://codereview.appspot.com/88190043/diff/70001/include/link.h#newcode234
include/link.h:234: R_CALL, // relocation for direct call
The way this is used in the machine-independent code suggests that this comment
should "relocation for direct PC-relative call".

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

https://codereview.appspot.com/88190043/diff/70001/src/cmd/6l/asm.c#newcode203
src/cmd/6l/asm.c:203: case R_CALL:
Why no changes to 5l/asm.c?

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1033
src/cmd/ld/lib.c:1033: s->stkcheck = 1;
I don't think you need to set the stkcheck field here, it will be set in the
stkcheck function.

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1077
src/cmd/ld/lib.c:1077: 
Trailing whitespace?

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1098
src/cmd/ld/lib.c:1098: prolog = !s->nosplit;
Argh, negating a flag expressed as a negative.

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1130
src/cmd/ld/lib.c:1130: if(end < pcsp.nextpc)
Why check this both here and at the top of the loop?

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1154
src/cmd/ld/lib.c:1154: if(stkcheck(&ch1, depth+2) < 0)
Because the code checks explicitly for morestack above, this seems to boil down
to if(ch1.limit < 0).  Is it worth doing the recursive call?

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

https://codereview.appspot.com/88190043/diff/70001/src/liblink/asm6.c#newcode...
src/liblink/asm6.c:2907: case_Zo_m64:
Where is case_Zo_m64 defined?

https://codereview.appspot.com/88190043/diff/70001/src/liblink/asm8.c
File src/liblink/asm8.c (right):

https://codereview.appspot.com/88190043/diff/70001/src/liblink/asm8.c#newcode...
src/liblink/asm8.c:2293: case_Zo_m:
Where is case_Zo_m defined?

https://codereview.appspot.com/88190043/diff/70001/src/run.bash
File src/run.bash (right):

https://codereview.appspot.com/88190043/diff/70001/src/run.bash#newcode102
src/run.bash:102: go run testld.go || exit 1
Should testld.go be in this CL?

[rsc, 2014-04-16 22:59:15]

https://codereview.appspot.com/88190043/diff/70001/include/link.h
File include/link.h (right):

https://codereview.appspot.com/88190043/diff/70001/include/link.h#newcode234
include/link.h:234: R_CALL, // relocation for direct call
On 2014/04/16 21:29:09, iant wrote:
> The way this is used in the machine-independent code suggests that this
comment
> should "relocation for direct PC-relative call".

Thanks. Changed, and changed ARM to use R_CALLARM.

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

https://codereview.appspot.com/88190043/diff/70001/src/cmd/6l/asm.c#newcode203
src/cmd/6l/asm.c:203: case R_CALL:
On 2014/04/16 21:29:09, iant wrote:
> Why no changes to 5l/asm.c?

Thanks, will fix.

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1033
src/cmd/ld/lib.c:1033: s->stkcheck = 1;
On 2014/04/16 21:29:09, iant wrote:
> I don't think you need to set the stkcheck field here, it will be set in the
> stkcheck function.

Done.

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1077
src/cmd/ld/lib.c:1077: 
On 2014/04/16 21:29:09, iant wrote:
> Trailing whitespace?

Done, but everywhere in the file (sorry).

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1098
src/cmd/ld/lib.c:1098: prolog = !s->nosplit;
On 2014/04/16 21:29:09, iant wrote:
> Argh, negating a flag expressed as a negative.

I know, but NOSPLIT is the name of the assembler directive.

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1130
src/cmd/ld/lib.c:1130: if(end < pcsp.nextpc)
On 2014/04/16 21:29:09, iant wrote:
> Why check this both here and at the top of the loop?

It made sense at the time (yesterday morning). Note that the difference is that
we avoid the call to pciternext if we break early. It's unclear to me now. I
will write it to be clearer and come back.

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1154
src/cmd/ld/lib.c:1154: if(stkcheck(&ch1, depth+2) < 0)
On 2014/04/16 21:29:09, iant wrote:
> Because the code checks explicitly for morestack above, this seems to boil
down
> to if(ch1.limit < 0).  Is it worth doing the recursive call?

Yes, to get a nice diagnostic that tells you that a function pointer is
involved. Also, this is old Go 1.2 code - I didn't write this today - so I'm
just trying to go with what I know works (or worked).

[ality, 2014-04-16 23:02:29]

iant@golang.org once said:
>
https://codereview.appspot.com/88190043/diff/70001/src/liblink/asm6.c#newcode...
> src/liblink/asm6.c:2907: case_Zo_m64:
> Where is case_Zo_m64 defined?

Right there. It's a label for the goto in the Zcallindreg branch.

  Anthony

[rsc, 2014-04-16 23:11:19]

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

https://codereview.appspot.com/88190043/diff/70001/src/liblink/asm6.c#newcode...
src/liblink/asm6.c:2907: case_Zo_m64:
On 2014/04/16 21:29:09, iant wrote:
> Where is case_Zo_m64 defined?

This is the definition; used in the new case below.

https://codereview.appspot.com/88190043/diff/70001/src/liblink/asm8.c
File src/liblink/asm8.c (right):

https://codereview.appspot.com/88190043/diff/70001/src/liblink/asm8.c#newcode...
src/liblink/asm8.c:2293: case_Zo_m:
On 2014/04/16 21:29:09, iant wrote:
> Where is case_Zo_m defined?

Same.

https://codereview.appspot.com/88190043/diff/70001/src/run.bash
File src/run.bash (right):

https://codereview.appspot.com/88190043/diff/70001/src/run.bash#newcode102
src/run.bash:102: go run testld.go || exit 1
On 2014/04/16 21:29:09, iant wrote:
> Should testld.go be in this CL?

Sorry, that was an old test. Replaced by the one in the test directory; will
revert this file.

[rsc, 2014-04-17 00:28:00]

PTAL

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/88190043/diff/70001/src/cmd/ld/lib.c#newcode1130
src/cmd/ld/lib.c:1130: if(end < pcsp.nextpc)
On 2014/04/16 22:59:16, rsc wrote:
> On 2014/04/16 21:29:09, iant wrote:
> > Why check this both here and at the top of the loop?
> 
> It made sense at the time (yesterday morning). Note that the difference is
that
> we avoid the call to pciternext if we break early. It's unclear to me now. I
> will write it to be clearer and come back.

This code is tricky because it needs to iterate over the relocations and the
pcsp table simultaneously. This version loops over the relocations and, inside,
over the pcsp table. My latest upload swap them, looping over the pcsp table
and, inside, the relocations. This is much clearer.

[iant, 2014-04-17 01:37:52]

LGTM

https://codereview.appspot.com/88190043/diff/110002/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/88190043/diff/110002/src/cmd/ld/lib.c#newcode1038
src/cmd/ld/lib.c:1038: ctxt->cursym = s;
The indentation looks wrong here and below, but that may just be Reitveld.

https://codereview.appspot.com/88190043/diff/110002/src/cmd/ld/lib.c#newcode1112
src/cmd/ld/lib.c:1112: for(; r < endr && r->off < pcsp.nextpc; r++) {
This looks like we require that the relocs be in order by address.  It would be
nice to write that down in liblink/objfile.c.

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

https://codereview.appspot.com/88190043/diff/110002/src/liblink/objfile.c#new...
src/liblink/objfile.c:361: wrint(b, s->nosplit);
You need to update pkg/debug/goobj/read.go.

https://codereview.appspot.com/88190043/diff/110002/test/nosplit.go
File test/nosplit.go (right):

https://codereview.appspot.com/88190043/diff/110002/test/nosplit.go#newcode190
test/nosplit.go:190: ioutil.WriteFile(dir+"/main.go", []byte("package main\nfunc
main()\n"), 0666)
filepath.Join(dir, "main.go") ?

[rsc, 2014-04-17 01:56:50]

For the record, this was not a waste of time. It found a real bug. In package
image, the method image.Point.Mod had a 120-byte frame with no calls, so we
marked it NOSPLIT. But it did have a DUFFCOPY, which is a call, and the linker
saw it as a call (through no advance planning on my part), and that 2-function
NOSPLIT chain ended up using 8+120+8 = too many bytes. If it had happened in
real life at exactly the bottom of a stack, we'd have scribbled 8 bytes into
whatever was below the stack in memory. I fixed the auto-NOSPLIT code to know
that DUFFCOPY and DUFFZERO are calls.

It may be that this would never have come up in practice. In earlier versions of
Go, it was very common to be executing at the bottom of a stack segment. With
the new growing stacks, every time you touch the bottom of a stack segment, the
bottom jumps away from you, so you're usually not executing near the bottom, and
so the opportunities for this kind of overflow to cause corruption are
considerably rarer. All the more reason to catch it at link time, of course.

Running another round of tests and then I will submit.

https://codereview.appspot.com/88190043/diff/110002/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/88190043/diff/110002/src/cmd/ld/lib.c#newcode1038
src/cmd/ld/lib.c:1038: ctxt->cursym = s;
On 2014/04/17 01:37:52, iant wrote:
> The indentation looks wrong here and below, but that may just be Reitveld.

I think it is.

https://codereview.appspot.com/88190043/diff/110002/src/cmd/ld/lib.c#newcode1112
src/cmd/ld/lib.c:1112: for(; r < endr && r->off < pcsp.nextpc; r++) {
On 2014/04/17 01:37:52, iant wrote:
> This looks like we require that the relocs be in order by address.  It would
be
> nice to write that down in liblink/objfile.c.

Done.

-//	- r [nr relocations]
+//	- r [nr relocations, sorted by off]

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

https://codereview.appspot.com/88190043/diff/110002/src/liblink/objfile.c#new...
src/liblink/objfile.c:361: wrint(b, s->nosplit);
On 2014/04/17 01:37:52, iant wrote:
> You need to update pkg/debug/goobj/read.go.

Done.

https://codereview.appspot.com/88190043/diff/110002/test/nosplit.go
File test/nosplit.go (right):

https://codereview.appspot.com/88190043/diff/110002/test/nosplit.go#newcode190
test/nosplit.go:190: ioutil.WriteFile(dir+"/main.go", []byte("package main\nfunc
main()\n"), 0666)
On 2014/04/17 01:37:52, iant wrote:
> filepath.Join(dir, "main.go") ?

i guess, although / works everywhere.

[rsc, 2014-04-17 02:08:06]

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

liblink, cmd/ld: reenable nosplit checking and test

The new code is adapted from the Go 1.2 nosplit code,
but it does not have the bug reported in issue 7623:

g% go run nosplit.go
g% go1.2 run nosplit.go
BUG
rejected incorrectly:
        main 0 call f; f 120

        linker output:
        # _/tmp/go-test-nosplit021064539
        main.main: nosplit stack overflow
                120	guaranteed after split check in main.main
                112	on entry to main.f
                -8	after main.f uses 120

g%

Fixes issue 6931.
Fixes issue 7623.

LGTM=iant
R=golang-codereviews, iant, ality
CC=golang-codereviews, r
https://codereview.appspot.com/88190043

[gobot, 2014-04-17 02:10:46]

This CL appears to have broken the darwin-amd64 builder.
See http://build.golang.org/log/01bc014ac6b370028d603b6fd2f8a04bdb1bccdb