New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: make 64-bit fields 64-bit aligned on 32-bit systems #599

Open
rsc opened this Issue Feb 12, 2010 · 29 comments

Comments

Projects
None yet
@rsc
Copy link
Contributor

rsc commented Feb 12, 2010

Even though the 386 has a 4-byte word size, some instructions move 
data in larger chunks.  In particular float64 loads move 8 bytes and
some SSE2 operations (as yet unused) move even larger amounts.
A float64 load from an only-4-byte-aligned address is significantly
more expensive than one from a 8-byte-aligned address.  The same
is probably true of SSE2 too.

We should make sure that data symbols >32 bits (e.g., float64 constants)
are aligned on 8-byte boundaries in the data segment.  (Fairly easy.)

We should also make sure that the stack pointer can be relied upon to be
8-byte aligned (harder), by making stack frame sizes = -4 mod 8 
(the caller PC will add 4 more) and starting new goroutines with a
properly aligned stack pointer.

It might be worth using 16 bytes for the stack alignment.  OS X requires
that for their own ABI, presumably because it matters for SSE2.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 9, 2011

Comment 1:

Labels changed: added priority-someday.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Sep 12, 2012

Comment 2:

Issue #3799 has been merged into this issue.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Sep 12, 2012

Comment 3:

This is necessary for atomics to work correctly.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Sep 12, 2012

Comment 4:

Labels changed: added priority-soon, removed priority-someday.

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Sep 12, 2012

Comment 5:

Labels changed: added go1.1.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 10, 2012

Comment 7:

Labels changed: added size-l.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 22, 2012

Comment 8:

Too hard. I tried a few different things and failed miserably. The gc toolchain expects
that the struct and function argument frame alignments match. The stack frame 
barely matters, since we don't take pointers of stack variables anymore, but that's
hard too.
This only really matters for atomics, and we do guarantee 64-bit alignment for the
base of an allocated struct or slice, so that will have to be good enough for now.

Labels changed: added priority-someday, removed priority-soon, go1.1.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 22, 2012

Comment 9:

Labels changed: added size-xl, removed size-l.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jul 30, 2013

Comment 10:

Labels changed: added go1.3maybe.

@robpike

This comment has been minimized.

Copy link
Contributor

robpike commented Aug 20, 2013

Comment 11:

Labels changed: removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Oct 29, 2013

Comment 13:

I have had an opportunity to revisit this problem recently in a different context. I
think we can do it for Go 1.3.

Labels changed: added go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 4, 2013

Comment 14:

Labels changed: added release-go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 4, 2013

Comment 15:

Labels changed: removed go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 4, 2013

Comment 16:

Labels changed: added repo-main.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Apr 3, 2014

Comment 17:

We didn't do it for 1.3. My guess is that in comment 13 I was referring to NaCl having
unusual alignment requirements, so perhaps that would pave the way.

Labels changed: added release-go1.4, removed release-go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Sep 15, 2014

Comment 18:

Labels changed: added release-none, removed release-go1.4.

tsg added a commit to elastic/beats that referenced this issue Jan 13, 2017

Ensure harvesterCounter 8-byte alignment (#3273) (#3338)
harvesterCounter is accessed atomically and will fault on x86-32 or ARM
if not 8-byte aligned. See golang/go#599 for more details on why it
fails and https://golang.org/pkg/sync/atomic/#pkg-note-BUG for how
putting the field first in the struct fixes it.

tsg added a commit to tsg/beats that referenced this issue Jan 13, 2017

Ensure harvesterCounter 8-byte alignment (elastic#3273) (elastic#3338)
harvesterCounter is accessed atomically and will fault on x86-32 or ARM
if not 8-byte aligned. See golang/go#599 for more details on why it
fails and https://golang.org/pkg/sync/atomic/#pkg-note-BUG for how
putting the field first in the struct fixes it.
(cherry picked from commit fd64af2)

ruflin added a commit to elastic/beats that referenced this issue Jan 13, 2017

Backport 3338 to the 5.2 branch (#3364)
* Ensure harvesterCounter 8-byte alignment (#3273) (#3338)

harvesterCounter is accessed atomically and will fault on x86-32 or ARM
if not 8-byte aligned. See golang/go#599 for more details on why it
fails and https://golang.org/pkg/sync/atomic/#pkg-note-BUG for how
putting the field first in the struct fixes it.
(cherry picked from commit fd64af2)

* Follow up with changelog for 3338 (#3363)

(cherry picked from commit 03b70c3)

fomojola added a commit to fomojola/go-cast that referenced this issue Jan 15, 2017

ARM compatibility
Added an unused int32 to the Channel structure. This fixes execution on ARM platforms (specifically Raspberry Pi) caused by golang/go#599. After cross-compiling for ARM using

GOOS=linux GOARM=6 GOARCH=arm go build github.com/barnybug/go-cast/cmd/cast

it appears that the call

requestId := int(atomic.AddInt64(&c.requestId, 1))

in Request() causes errors because the Channel structure isn't appropriately 64-bit aligned. The returned error looks like this:

pi@raspberrypi:~ $ ./cast --name chromecast media play 'http://192.168.2.22:8080/music/play.mp3"
Connecting to 192.168.2.42:8009...
Connected
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x4 pc=0x152894]

goroutine 1 [running]:
panic(0x332b60, 0x10824008)
	/usr/local/go/src/runtime/panic.go:500 +0x33c
github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli.HandleAction.func1(0x108f8f30)
	/workspace/src/github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli/app.go:478 +0x290
panic(0x332b60, 0x10824008)
	/usr/local/go/src/runtime/panic.go:458 +0x454
sync/atomic.addUint64(0x1090159c, 0x1, 0x0, 0x1, 0x0)
	/usr/local/go/src/sync/atomic/64bit_arm.go:31 +0x68
github.com/barnybug/go-cast/net.(*Channel).Request(0x10901580, 0x76ef1120, 0x10816440, 0x495920, 0x4aa058, 0x141130, 0x0, 0x0)
	/workspace/src/github.com/barnybug/go-cast/net/channel.go:77 +0x60
github.com/barnybug/go-cast/controllers.(*ReceiverController).GetStatus(0x108c5700, 0x76ef1120, 0x10816440, 0x0, 0x0, 0x0)
	/workspace/src/github.com/barnybug/go-cast/controllers/receiver.go:150 +0x58
github.com/barnybug/go-cast.(*Client).launchMediaApp(0x10816900, 0x76ef1120, 0x10816440, 0x0, 0x0, 0x0, 0x0)
	/workspace/src/github.com/barnybug/go-cast/client.go:133 +0x48
github.com/barnybug/go-cast.(*Client).Media(0x10816900, 0x76ef1120, 0x10816440, 0x4, 0x0, 0x0)
	/workspace/src/github.com/barnybug/go-cast/client.go:171 +0x54
main.runCommand(0x76ef1120, 0x10816440, 0x10816900, 0x36447a, 0x4, 0x1080a0e8, 0x1, 0x1)
	/workspace/src/github.com/barnybug/go-cast/cmd/cast/main.go:343 +0xc8
main.cliCommand(0x10878280)
	/workspace/src/github.com/barnybug/go-cast/cmd/cast/main.go:128 +0x244
reflect.Value.call(0x31c8a0, 0x395e40, 0x13, 0x36428e, 0x4, 0x108f8ef0, 0x1, 0x1, 0x0, 0x0, ...)
	/usr/local/go/src/reflect/value.go:434 +0xd68
reflect.Value.Call(0x31c8a0, 0x395e40, 0x13, 0x108f8ef0, 0x1, 0x1, 0x0, 0x0, 0x0)
	/usr/local/go/src/reflect/value.go:302 +0x84
github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli.HandleAction(0x31c8a0, 0x395e40, 0x10878280, 0x0, 0x0)
	/workspace/src/github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli/app.go:487 +0x1d0
github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli.Command.Run(0x36447a, 0x4, 0x0, 0x0, 0x0, 0x0, 0x0, 0x3676e2, 0xf, 0x0, ...)
	/workspace/src/github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli/command.go:191 +0xc10
github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli.(*App).RunAsSubcommand(0x108883c0, 0x10878140, 0x0, 0x0)
	/workspace/src/github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli/app.go:361 +0xd18
github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli.Command.startApp(0x3647da, 0x5, 0x0, 0x0, 0x0, 0x0, 0x0, 0x3671f2, 0xe, 0x0, ...)
	/workspace/src/github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli/command.go:278 +0x78c
github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli.Command.Run(0x3647da, 0x5, 0x0, 0x0, 0x0, 0x0, 0x0, 0x3671f2, 0xe, 0x0, ...)
	/workspace/src/github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli/command.go:79 +0x54
github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli.(*App).Run(0x10888300, 0x1080a0c0, 0x6, 0x6, 0x0, 0x0)
	/workspace/src/github.com/barnybug/go-cast/vendor/github.com/codegangsta/cli/app.go:240 +0x798
main.main()
	/workspace/src/github.com/barnybug/go-cast/cmd/cast/main.go:116 +0x6a0

The addition of the unused int32 forces the alignment to adjust appropriately and prevents the runtime error.
@steeve

This comment has been minimized.

Copy link
Contributor

steeve commented Feb 27, 2017

For the record, this is still an open issue on linux/arm and android/arm for go 1.8 (we've just been bitten by it) when doing atomic.AddUint64.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Feb 27, 2017

Yes that's true. Go 1.8 is the same as all previous Go versions in this regard. I assume "bitten" means that atomic.AddUint64 panicked. (That's the intended behavior, as opposed to silently doing the wrong thing.)

@steeve

This comment has been minimized.

Copy link
Contributor

steeve commented Feb 27, 2017

Panicking is indeed better than silently failing, although the reason/message is obscure (we found out why because of this issue and GroupCache's).
Perhaps the panic message should be more descriptive?

Here was ours:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x4 pc=0x9c74983c]
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Feb 27, 2017

Since it's just a stop-gap until this is fixed and any additional work would slow down the common case, I think the message is probably fine as is.

@kozlovic kozlovic referenced this issue Jun 28, 2017

Merged

Round out config reload #523

11 of 11 tasks complete

amomchilov pushed a commit to amomchilov/Filebeat that referenced this issue Apr 19, 2018

Ensure harvesterCounter 8-byte alignment (#3273) (#3338)
harvesterCounter is accessed atomically and will fault on x86-32 or ARM
if not 8-byte aligned. See golang/go#599 for more details on why it
fails and https://golang.org/pkg/sync/atomic/#pkg-note-BUG for how
putting the field first in the struct fixes it.

juagargi added a commit to juagargi/scion that referenced this issue Jul 18, 2018

Patch ARM.
In particular, rearrange a field in Go's sciond.connector struct, due to a existing
issue golang/go#599 .

juagargi added a commit to juagargi/scion that referenced this issue Jul 18, 2018

Patch ARM.
In particular, rearrange a field in Go's sciond.connector struct, due to a existing
issue golang/go#599 .

juagargi added a commit to juagargi/scion that referenced this issue Jul 18, 2018

Patch ARM.
In particular, rearrange a field in Go's sciond.connector struct, due to a existing
issue golang/go#599 .
@cannium

This comment has been minimized.

Copy link

cannium commented Sep 10, 2018

My question might be off-topic, but I'd like to know other than plan struct carefully for sync/atomic, what else need to take care of in order to run on ARM32 platforms? It seems all arm-related problems lead to this issue...
Also, is this issue relevant to ARM64 platforms? The doc seems a little outdated:
On both ARM and x86-32, it is the caller's responsibility to arrange for 64-bit alignment of 64-bit words accessed atomically.

@go101

This comment has been minimized.

Copy link

go101 commented Sep 10, 2018

ARM means ARM32. ARM64 platforms have no this issue. On 32bit archs, there are several ways to guarantee 64-bit aligned at compile time and a trick to do this at run time. Please read this article for more explanations.

TizianoPerrucci added a commit to StackVista/stackstate-process-agent that referenced this issue Dec 7, 2018

Upstream 6 7 0 (#16)
* Move from glide to dep

* Removing unneccesary constraints and pinning constraints to revisions

* Add environment flag override for using local network tracer

* Set local tracer usage flag earlier (before errors may happen)

* Removing fmt.print statements

* Removing accidently commited binary

* Merge pull request DataDog#187 from DataDog/sunhay/add_dd_log_to_console_flag

Support DD_LOG_TO_CONSOLE environment flag

* Adding network-tracer.yaml for network related configuration

* Updating tests with expected assertions

* Add support for the new 'site' parameter in the agent config

* Fix warninng (ddUrl -> ddURL)

* Move main agent go package to cmd subdirectory

* Move network-tracer agent to process-agent repo

* Use new GetMainEndpoint from the datadog-agent config package

* Remove the ProcessDDUrl field from the yamlconfig struct since we now retrieve it using datadog-agent/pkg/config

* Fix network-tracer build script and config parsing

* Adding documentation on exported functions and constants

* More documentation

* Remove logic to retrieve the env variable DD_SITE when DD_PROCESS_AGENT_URL is not set

* Fix dependencies issues

* Add logging for signal handler on network-tracer

* Fix build by using updated golint import location

* Update datadog-agent revision to latest

* Bump winversion to 6.6.0

* fix windows CI

* Fix directory

* fix variable name

* Fixing dep lock file and bumping tcptracer-bpf w/ udp fixes

* Update runCounter to be int32

On 32 bits system like armv7 (raspberry pi), using `atomic.AddInt64` (and other variants) resulted in a nil pointer exception. I understand datadog does not officially support arm, but since the fix is pretty easy in this case, it seemed like a good candidate for a PR.

This go issue has more details golang/go#599

In our case we make limited use of 64 bits atomic functions, in two places:
- `runCounter`: This is only used for logging and assuming an increment each second would overflow in 70 years if 32 bits, so is most likely safe
- `realTimeEnabled`: This is used as a pseudo bolean so 0/1 and is safe

I've recompiled the agent 6.5.2 from source using this fix and I'm successfully running it on my Raspberry Pi, with processes reporting.

* applying branding

* applying gofmt

* addressed linting

* Fix tracer.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment