Ensure harvesterCounter 8-byte alignment (#3273) #3338
Conversation
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.
|
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run. |
1 similar comment
|
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run. |
|
jenkins, test it |
| @@ -21,6 +21,7 @@ var ( | |||
| ) | |||
|
|
|||
| type Prospector struct { | |||
| harvesterCounter uint64 // Must be 8-byte aligned. Ensured if first field in struct | |||
There was a problem hiding this comment.
As it is quite likely that this is changed by accident in the future, I suggest to add a comment on top of the variable with the following content:
// harvesterCount MUST be first field in struct. See golang/go#599
There was a problem hiding this comment.
Thanks for the feedback @ruflin. Should I make the changelog entry under Added or Bugfixes? I know you wrote Added above but thought I'd double check since this is a bug fix.
Also, I agree this is brittle. I.e. someone could easily break this by changing the order of the fields. Should we add a test for this? I realize we may address the issue more generally per @urso's comment #3273 (comment) but not sure what the priority of that is.
There was a problem hiding this comment.
I'm fine putting it under Bugfixes.
Do you have some ideas on how to add a test? Would be great to have one.
|
Thanks @spacewander for the fix! As we have a little bit of urgency around this, I merged it as is and will add the changelog in a follow up PR. |
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)
* 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)
* 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 f6f5e10) * Follow up with changelog for 3338 (elastic#3363) (cherry picked from commit 6af51cd)
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.