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

Fix paginator counter on x86-32 #2420

Merged
merged 1 commit into from Sep 9, 2016

Conversation

Projects
None yet
2 participants
@bep
Member

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

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
@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 9, 2016

Member

@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.

Member

bep commented Sep 9, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@MarkDBlackwell

MarkDBlackwell Sep 9, 2016

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
Contributor

MarkDBlackwell commented Sep 9, 2016

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 gohugoio: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

This comment has been minimized.

Show comment
Hide comment
@MarkDBlackwell

MarkDBlackwell Sep 9, 2016

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.

Contributor

MarkDBlackwell commented Sep 9, 2016

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