8000 Move int64 to beginning of struct to ensure alignment (fixes 32-bit ARM crash) by mattdsteele · Pull Request #61 · nakabonne/tstorage · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Move int64 to beginning of struct to ensure alignment (fixes 32-bit ARM crash) #61

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 24, 2023
Merged

Move int64 to beginning of struct to ensure alignment (fixes 32-bit ARM crash) #61

merged 1 commit into from
Mar 24, 2023

Conversation

mattdsteele
Copy link
Contributor

This change reorders the fields in memoryPartition to ensure safe access on some architectures.

In particular, 32-bit operating systems running on ARM processors (common with Raspberry Pi) will encounter errors.

You can reproduce the crash by running the tests on a armv7l machine:

pi@stream:~/tstorage$ go test
--- FAIL: Test_memoryPartition_InsertRows (0.00s)
panic: unaligned 64-bit atomic operation [recovered]
        panic: unaligned 64-bit atomic operation

goroutine 36 [running]:
testing.tRunner.func1.2({0x205e80, 0x278fb4})
        /usr/local/go/src/testing/testing.go:1526 +0x27c
testing.tRunner.func1()
        /usr/local/go/src/testing/testing.go:1529 +0x42c
panic({0x205e80, 0x278fb4})
        /usr/local/go/src/runtime/panic.go:884 +0x218
runtime/internal/atomic.panicUnaligned()
        /usr/local/go/src/runtime/internal/atomic/unaligned.go:8 +0x24
runtime/internal/atomic.Store64(0x112e42c, 0x2)
        /usr/local/go/src/runtime/internal/atomic/atomic_arm.s:291 +0x14
github.com/nakabonne/tstorage.(*memoryPartition).insertRows.func1()
        /home/pi/tstorage/memory_partition.go:74 +0xd4
sync.(*Once).doSlow(0x112e454, 0x10286ac)
        /usr/local/go/src/sync/once.go:74 +0xb8
sync.(*Once).Do(0x112e454, 0x10286ac)
        /usr/local/go/src/sync/once.go:65 +0x40
github.com/nakabonne/tstorage.(*memoryPartition).insertRows(0x112e410, {0x1100360, 0x1, 0x1})
        /home/pi/tstorage/memory_partition.go:66 +0xb0
github.com/nakabonne/tstorage.Test_memoryPartition_InsertRows.func1()
        /home/pi/tstorage/memory_partition_test.go:40 +0xb8
github.com/nakabonne/tstorage.Test_memoryPartition_InsertRows(0x1107590)
        /home/pi/tstorage/memory_partition_test.go:44 +0x58
testing.tRunner(0x1107590, 0x2457fc)
        /usr/local/go/src/testing/testing.go:1576 +0x118
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:1629 +0x43c
exit status 2
FAIL    github.com/nakabonne/tstorage   0.017s

This issue has been identified and addressed in other modules: owncloud/ocis#1888

The Golang project documents this at https://golang.org/pkg/sync/atomic/#pkg-note-BUG mentioning it is the "caller's responsibility to arrange for 64-bit alignment of 64-bit words accessed atomically" and "The first word in (...) an allocated struct (...) can be relied upon to be 64-bit aligned"

@mattdsteele mattdsteele mentioned this pull request Feb 27, 2023
Closed
@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -1.54 ⚠️

Comparison is base (6ca1faf) 62.00% compared to head (58e5f45) 60.45%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #61      +/-   ##
==========================================
- Coverage   62.00%   60.45%   -1.54%     
==========================================
  Files          19       19              
  Lines        1134     1613     +479     
==========================================
+ Hits          703      975     +272     
- Misses        296      502     +206     
- Partials      135      136       +1
Impacted Files Coverage Δ
memory_partition.go 82.61% <ø> (+0.93%) ⬆️

... and 18 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nakabonne
Copy link
Owner

@mattdsteele Hi, sorry for the delay! This change makes sense so I will release this fix quickly.

@nakabonne nakabonne merged commit 4444bd3 into nakabonne:main Mar 24, 2023
@github-actions github-actions bot mentioned this pull request Mar 24, 2023
Merged
@nakabonne
Copy link
Owner

@mattdsteele This change is released https://github.com/nakabonne/tstorage/releases/tag/v0.3.6 Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mattdsteele @codecov-commenter @nakabonne
0