code review 134410043: runtime: fix sigtrampPC on Windows

runtime: fix sigtrampPC on Windows

The arm5 build breakage at CL 139110043 was caused by
calling funcPC on a lessstack defined as a struct{}.
That symbol ended up with a non-4-aligned address,
which caused the memory fault that broke the builders.
The definition of lessstack was fixed in CL 140880043.

Tracking that down suggested that it would be worth
looking for the same bug elsewhere in the directory.
This is the only one I found.

[rsc, 2014-09-04 04:12:16]

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

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

[dave_cheney.net, 2014-09-04 04:17:20]

I'm glad that strict alignment checking by the arm5 hardware was able to
find a real bug.
On 4 Sep 2014 14:12, <rsc@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: dvyukov, iant, khr, r),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: fix sigtrampPC on Windows
>
> The arm5 build breakage at CL 139110043 was caused by
> calling funcPC on a lessstack defined as a struct{}.
> That symbol ended up with a non-4-aligned address,
> which caused the memory fault that broke the builders.
> The definition of lessstack was fixed in CL 140880043.
>
> Tracking that down suggested that it would be worth
> looking for the same bug elsewhere in the directory.
> This is the only one I found.
>
> Please review this at https://codereview.appspot.com/134410043/
>
> Affected files (+1, -1 lines):
>   M src/pkg/runtime/traceback_windows.go
>
>
> Index: src/pkg/runtime/traceback_windows.go
> ===================================================================
> --- a/src/pkg/runtime/traceback_windows.go
> +++ b/src/pkg/runtime/traceback_windows.go
> @@ -10,7 +10,7 @@
>  // The traceback needs to recognize it on link register architectures.
>  var sigtrampPC uintptr
>
> -var sigtramp struct{} // assembly function
> +func sigtramp()
>
>  func init() {
>         sigtrampPC = funcPC(sigtramp)
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[bradfitz, 2014-09-04 04:42:15]

LGTM

But keep the comment maybe?



On Wed, Sep 3, 2014 at 9:12 PM, <rsc@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: dvyukov, iant, khr, r),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: fix sigtrampPC on Windows
>
> The arm5 build breakage at CL 139110043 was caused by
> calling funcPC on a lessstack defined as a struct{}.
> That symbol ended up with a non-4-aligned address,
> which caused the memory fault that broke the builders.
> The definition of lessstack was fixed in CL 140880043.
>
> Tracking that down suggested that it would be worth
> looking for the same bug elsewhere in the directory.
> This is the only one I found.
>
> Please review this at https://codereview.appspot.com/134410043/
>
> Affected files (+1, -1 lines):
>   M src/pkg/runtime/traceback_windows.go
>
>
> Index: src/pkg/runtime/traceback_windows.go
> ===================================================================
> --- a/src/pkg/runtime/traceback_windows.go
> +++ b/src/pkg/runtime/traceback_windows.go
> @@ -10,7 +10,7 @@
>  // The traceback needs to recognize it on link register architectures.
>  var sigtrampPC uintptr
>
> -var sigtramp struct{} // assembly function
> +func sigtramp()
>
>  func init() {
>         sigtrampPC = funcPC(sigtramp)
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[rsc, 2014-09-04 04:48:27]

the comment is about a different function.
it was copy/pasted from traceback.go.

[rsc, 2014-09-04 04:54:43]

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

runtime: fix sigtrampPC on Windows

The arm5 build breakage at CL 139110043 was caused by
calling funcPC on a lessstack defined as a struct{}.
That symbol ended up with a non-4-aligned address,
which caused the memory fault that broke the builders.
The definition of lessstack was fixed in CL 140880043.

Tracking that down suggested that it would be worth
looking for the same bug elsewhere in the directory.
This is the only one I found.

LGTM=bradfitz
R=golang-codereviews, dave, bradfitz
CC=dvyukov, golang-codereviews, iant, khr, r
https://codereview.appspot.com/134410043

[dave_cheney.net, 2014-09-04 05:06:18]

Interesting, I wonder why the runtime thinks this is the signal goroutine.

cmd/go
fatal error: onM called from signal goroutine

goroutine 1 [running]:
runtime.gothrow(0x252038, 0x20)
/usr/local/go/src/pkg/runtime/panic.c:365 +0x68 fp=0x1051beac sp=0x1051be9c
runtime.badonm()
/usr/local/go/src/pkg/runtime/stubs.go:105 +0x3c fp=0x1051beb8 sp=0x1051beac
runtime.onM(0x284114)
/usr/local/go/src/pkg/runtime/asm_arm.s:203 +0x2c fp=0x1051bebc sp=0x1051beb8
runtime.gomallocgc(0x240, 0x221568, 0x0, 0x85808)
/usr/local/go/src/pkg/runtime/malloc.go:178 +0x760 fp=0x1051bf08 sp=0x1051bebc
runtime.cmallocgc(0x220, 0x221568, 0x0, 0x1051bf30)
/usr/local/go/src/pkg/runtime/malloc.go:337 +0x3c fp=0x1051bf1c sp=0x1051bf08
runtime.mallocgc(0x220, 0x221568, 0x0, 0x0)
/usr/local/go/src/pkg/runtime/malloc.c:39 +0x5c fp=0x1051bf34 sp=0x1051bf1c
cnew(0x221568, 0x1, 0x0)
/usr/local/go/src/pkg/runtime/malloc.c:358 +0x98 fp=0x1051bf48 sp=0x1051bf34
runtime.cnew(0x221568, 0x0)
/usr/local/go/src/pkg/runtime/malloc.c:365 +0x34 fp=0x1051bf58 sp=0x1051bf48
runtime.allocm(0x0, 0x0)
/usr/local/go/src/pkg/runtime/proc.c:951 +0x90 fp=0x1051bf70 sp=0x1051bf58
newm(0x96de0, 0x0)
/usr/local/go/src/pkg/runtime/proc.c:1201 +0x30 fp=0x1051bf8c sp=0x1051bf70
runtime.main()
/usr/local/go/src/pkg/runtime/proc.c:230 +0x50 fp=0x1051bfc4 sp=0x1051bf8c
runtime.goexit()
/usr/local/go/src/pkg/runtime/proc.c:1752 fp=0x1051bfc4 sp=0x1051bfc4

On Thu, Sep 4, 2014 at 2:54 PM,  <rsc@golang.org> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=5179e31ac3da ***
>
>
> runtime: fix sigtrampPC on Windows
>
> The arm5 build breakage at CL 139110043 was caused by
> calling funcPC on a lessstack defined as a struct{}.
> That symbol ended up with a non-4-aligned address,
> which caused the memory fault that broke the builders.
> The definition of lessstack was fixed in CL 140880043.
>
> Tracking that down suggested that it would be worth
> looking for the same bug elsewhere in the directory.
> This is the only one I found.
>
> LGTM=bradfitz
> R=golang-codereviews, dave, bradfitz
> CC=dvyukov, golang-codereviews, iant, khr, r
> https://codereview.appspot.com/134410043
>
>
> https://codereview.appspot.com/134410043/

[rsc, 2014-09-04 05:35:45]

On Thu, Sep 4, 2014 at 1:06 AM, Dave Cheney <dave@cheney.net> wrote:

> Interesting, I wonder why the runtime thinks this is the signal goroutine.
>

Fixed.