Fix paginator counter on x86-32 #2420

Merged
merged 1 commit into from Sep 9, 2016

Projects

None yet

2 participants

@bep
Collaborator
bep commented Sep 9, 2016

Atomic operations with 64 bit values must be aligned for 64-bit on x86-32.

According to the spec:

"The first word in a global variable or in an allocated struct or slice can be relied upon to be 64-bit aligned."

The above wasn't enough for the paginationPageCount on SiteInfo, maybe due to how SiteInfo is embedded.

This commit adds a 4 byte padding before the uint64 that creates the correct alignment.

Fixes #2415

@bep bep Fix paginator counter on x86-32
Atomic operations with 64 bit values must be aligned for 64-bit on x86-32.

According to the spec:

"The first word in a global variable or in an allocated struct or slice can be relied upon to be 64-bit aligned."

The above wasn't enough for the `paginationPageCount` on `SiteInfo`, maybe due to how `SiteInfo` is embedded.

This commit adds a 4 byte padding before the `uint64` that creates the correct alignment.

Fixes #2415
64b5414
@bep
Collaborator
bep commented Sep 9, 2016 edited

@MarkDBlackwell @moorereason this PR makes the tests pass on both x86-32 and the rest for me.

Would be cool if you could take it for a spin before pulling it into master.

@MarkDBlackwell
Contributor

Now, here, all tests pass:

$ git checkout bep/paginationcounter
Note: checking out 'bep/paginationcounter'.
HEAD is now at 64b5414... Fix paginator counter on x86-32
$ go test github.com/spf13/hugo/...
ok  github.com/spf13/hugo/hugolib  46.520s
@bep bep merged commit 4df86a7 into spf13:master Sep 9, 2016

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/wercker Wercker build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@bep bep deleted the bep:paginationcounter branch Sep 9, 2016
@MarkDBlackwell
Contributor

Here I document for myself (and others) some points you may already know. Per func (*Value) Store (in pkg/sync/atomic):

All calls to Store for a given Value must use values of the same concrete type. Store of an inconsistent type panics[.]

Per Bugs (in pkg/sync/atomic):

On both ARM and x86-32, it is the caller's responsibility to arrange for 64-bit alignment of 64-bit words accessed atomically. The first word in a global variable or in an allocated struct or slice can be relied upon to be 64-bit aligned.

However, per discussion here and here:

If I have an array of structs, are the first words of each and every struct in the array automatically 64-bit aligned, or just the first one?

Just the first one; it depends [upon whether] the size of each element, plus padding, is a multiple of [64 bits]. The key phrase there is "allocated"...A struct inside a slice or array is not allocated by itself, and no particular alignment is guaranteed beyond that of unsafe.AlignOf.

From this, (for 32-bit systems), I take that:

  1. Using pkg/sync/atomic, storing an int64 to any location unaligned to 64 bits panics;
  2. Structs align int64 fields only to 32 bits; and
  3. When a struct is an array element, starting it with an int64 field generally aligns that element only to 32 bits.

An array of SiteInfo is created here by hugolib/hugo_sites.go (I think).

Per the discussion referenced above, on 32-bit machines, perhaps:

  1. Only if a struct's size is a multiple of 64 bits will Go then guarantee for it (in arrays and slices) that all elements are 64-bit aligned;
  2. Currently, the particular, problematic test passes, because now the size of the SiteInfo struct is a multiple of 64 bits (I suppose).

Some research revealed that automatically aligning the size of a struct (on 32-bit machines) to a multiple of 64 bits seems difficult.

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