fix: fix field alignment to avoid panics on 32bit OSes #44

hairyhenderson · 2024-06-22T18:52:47Z

Fixes #43

To solve this, I:

enabled the fieldalignment linter (which is a sub-linter of govet)
ran the fieldalignment -fix command to fix misaligned structs that were non-obvious
switch from using e.g. int64 + atomic.StoreInt64 to using atomic.Int64 (etc) and because of this...
drop Go 1.18 support, and require Go 1.19+ (where the new atomic types were introduced)
added a new CI job to test in a 32-bit container (so far just Intel 386, which should catch the same bugs as arm32/etc)
added some new tests to cover keyvalue/blob.Bytes, which was what was giving me trouble initially

Signed-off-by: Dave Henderson <[email protected]>

keyvalue/blob/bytes.go

JohnStarich

I'm not very experienced with these kinds of alignment issues, so I'm definitely open to a bit of discussion on how to handle these.

If at all possible, my strong preference is to maintain field ordering in the table driven test case types to keep it easier to quickly read and understand the tests. Of course if the tests fail, we'll need to do something else 🤔

.github/workflows/ci.yml

fstest/file.go

keyvalue/file.go

keyvalue/record.go

mount_test.go

tar/bufferpool.go

hairyhenderson · 2024-06-23T00:24:14Z

If at all possible, my strong preference is to maintain field ordering in the table driven test case types

I can revert the test files, no big deal.

Signed-off-by: Dave Henderson <[email protected]>

hairyhenderson · 2024-06-23T01:50:19Z

@JohnStarich thanks for the feedback, I think I've addressed it all - please have another look!

(I'm not sure why coveralls thinks the coverage decreased - I added new tests... ¯\_(ツ)_/¯)

JohnStarich · 2024-06-23T17:36:42Z

.github/workflows/ci.yml

+          - platform: macos-latest
+            goarch: '386'
+          - platform: windows-latest
+            goarch: '386'


This is great, thanks!

We may want to reduce the matrix a bit to a single 32 bit runner in the future, assuming we can prove the effective coverage is the same. As someone who's worked with 32 bit arch's more than me, do you foresee any problems with only validating those arch's on a single platform and Go version?

The only reason I didn't also add 32-bit ARM as well is that it's a huge pain to do (without Docker) in GitHub Actions, because the GitHub-hosted runners are all AMD64 currently (ARM64 is coming this fall maybe?).

Another option is to use a self-hosted runner, and I happen to be deep in that rabbit-hole right now (hairyhenderson/gomplate#2126) and it's going about as well as you'd expect (i.e. not at all well).

For this particular class of bug any 32-bit platform will generally do, but there are other classes of bugs that can crop up only on 32-bit ARM CPUs. Somewhat ironically it's extremely rare to run into 32-bit Intel systems except for niche embedded use-cases, but it's still quite common to run into 32-bit ARM given the massive proliferation of older Raspberry Pi-type systems and routers strewn across the IoT landscape.

.github/workflows/ci.yml

JohnStarich

Thanks again! Super appreciate the thought and time you put into this.

I did have one question about the future of maintaining 32 bit support here, since I'm not as well versed in field alignment issues like this. #44 (comment)

JohnStarich · 2024-06-23T17:47:26Z

Makefile

+TEST_ARGS=-race
+# the race detector needs cgo (at least on Linux and Windows, and macOS until Go 1.20)
+export CGO_ENABLED=1
+endif


Ah, bummer about race not being supported, this looks like a reasonable setup. Thank you for handling all of that. This must've been a pain wrangling with CI for so long.

I was already in the midst doing a similar thing for gomplate so it at least wasn't a new kind of pain 😅

It probably isn't always true, but races tend to happen in the same way on all platforms, so I wouldn't be too worried here.

JohnStarich · 2024-06-23T17:51:14Z

fstest/file.go

@@ -653,6 +654,7 @@ func TestFileSync(tb testing.TB, o FSOptions) {
 	assert.NoError(tb, file.Close())
 }

+//nolint:govet


Note to self: This might be doable with a .golangci.yml config change. Will look into it.

It's at least awkward in the golangci config, if not impossible - it lets you disable whole linters (i.e. govet in this case), but not sub-linters like fieldalignment.

If you want to avoid linting all tests with govet, then that's an option, but...

JohnStarich · 2024-06-23T17:55:52Z

Makefile

@@ -2,6 +2,7 @@ BROWSERTEST_VERSION = v0.7
 LINT_VERSION = 1.50.1
 GO_BIN = $(shell printf '%s/bin' "$$(go env GOPATH)")
 SHELL = bash
+ENABLE_RACE = true


Note to self: Looks like this var is now unused.

🤦‍♂️ good catch - that can be deleted

hairyhenderson marked this pull request as draft June 22, 2024 18:53

hairyhenderson force-pushed the fix-32bit-field-alignment branch 4 times, most recently from 1704878 to f8a5687 Compare June 22, 2024 19:52

hairyhenderson marked this pull request as ready for review June 22, 2024 19:55

fix: fix field alignment to avoid panics on 32bit OSes

eb37578

Signed-off-by: Dave Henderson <[email protected]>

hairyhenderson force-pushed the fix-32bit-field-alignment branch from f8a5687 to eb37578 Compare June 22, 2024 19:59

hairyhenderson commented Jun 22, 2024

View reviewed changes

keyvalue/blob/bytes.go Show resolved Hide resolved

hairyhenderson mentioned this pull request Jun 22, 2024

bug: Panic on 32-bit platforms #43

Closed

JohnStarich requested changes Jun 22, 2024

View reviewed changes

chore: don't bother aligning structs in tests

e6c6709

Signed-off-by: Dave Henderson <[email protected]>

hairyhenderson force-pushed the fix-32bit-field-alignment branch from 67918d1 to e6c6709 Compare June 23, 2024 00:37

hairyhenderson added 2 commits June 22, 2024 20:52

chore: addressing more review comments

c5ddd52

Signed-off-by: Dave Henderson <[email protected]>

chore: disable race detector on 32-bit

2ed9108

Signed-off-by: Dave Henderson <[email protected]>

hairyhenderson force-pushed the fix-32bit-field-alignment branch 5 times, most recently from 2f03e1c to db06695 Compare June 23, 2024 01:28

chore: run/test 32-bit binaries outside of Docker

724f886

Signed-off-by: Dave Henderson <[email protected]>

hairyhenderson force-pushed the fix-32bit-field-alignment branch from db06695 to 724f886 Compare June 23, 2024 01:29

chore: clean up race detector enablement

119a5ab

Signed-off-by: Dave Henderson <[email protected]>

hairyhenderson force-pushed the fix-32bit-field-alignment branch from 51e9ea1 to 0deb83d Compare June 23, 2024 01:41

chore: remove explicit cgo disablement

3c2bd7f

Signed-off-by: Dave Henderson <[email protected]>

hairyhenderson force-pushed the fix-32bit-field-alignment branch from 0deb83d to 3c2bd7f Compare June 23, 2024 01:42

JohnStarich reviewed Jun 23, 2024

View reviewed changes

JohnStarich approved these changes Jun 23, 2024

View reviewed changes

JohnStarich merged commit 58eb4cd into hack-pad:main Jun 23, 2024
5 of 6 checks passed

hairyhenderson mentioned this pull request Jun 23, 2024

deps(go): bump github.com/hack-pad/hackpadfs to fix running on 32-bit OSes hairyhenderson/gomplate#2127

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix field alignment to avoid panics on 32bit OSes #44

fix: fix field alignment to avoid panics on 32bit OSes #44

hairyhenderson commented Jun 22, 2024 •

edited

Loading

JohnStarich left a comment

hairyhenderson commented Jun 23, 2024

hairyhenderson commented Jun 23, 2024

JohnStarich Jun 23, 2024

hairyhenderson Jun 23, 2024

JohnStarich left a comment •

edited

Loading

JohnStarich Jun 23, 2024

hairyhenderson Jun 23, 2024

JohnStarich Jun 23, 2024

hairyhenderson Jun 23, 2024

JohnStarich Jun 23, 2024

hairyhenderson Jun 23, 2024

fix: fix field alignment to avoid panics on 32bit OSes #44

fix: fix field alignment to avoid panics on 32bit OSes #44

Conversation

hairyhenderson commented Jun 22, 2024 • edited Loading

JohnStarich left a comment

Choose a reason for hiding this comment

hairyhenderson commented Jun 23, 2024

hairyhenderson commented Jun 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnStarich left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hairyhenderson commented Jun 22, 2024 •

edited

Loading

JohnStarich left a comment •

edited

Loading