Skip to content
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
Merged

Conversation

bep
Copy link
Member

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

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 gohugoio#2415
@bep
Copy link
Member Author

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
Copy link
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 gohugoio:master Sep 9, 2016
@bep bep deleted the paginationcounter branch September 9, 2016 12:32
@MarkDBlackwell
Copy link
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.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hugo test fails on 32-bit machine
2 participants