Support 32-bit platforms, and fix the three bugs that were hiding there - #75
Merged
Merged
Conversation
Bumps golibjpeg and goopenjpeg to v1.3.0, which add prebuilt libraries for
darwin/amd64 and windows/arm64 and, more importantly, now build on every
platform Go targets instead of only the ones they ship a library for. That made
"godicom is importable anywhere" a claim worth checking, and it was false:
godicom itself did not compile for a 32-bit target. Making it compile turned up
two bugs that had nothing to do with cross-compilation.
The dictionary one is the serious one. All 88 repeater masks were parsed with
fmt.Sscanf into int fields with the error discarded. A mask like 0xFF00FFFF does
not fit in a 32-bit int, so on those platforms every mask was zero, and
(tag ^ value) & 0 == 0 is true for every tag: whichever repeater came first in
map order answered for every non-private tag. LookupVR returned US for
PatientName and for group lengths, and IsRepeaterTag was true for everything.
Rebuilt with nibble arithmetic into uint32 -- no error to drop, no width to
overflow.
ParseTag was broken on every platform, not just 32-bit: strconv.ParseInt with a
bitSize of 32 rejects anything from 0x80000000 up, so ParseTag("FFFEE000")
failed with "unknown tag keyword". Those are the item and delimiter tags the
sequence and encapsulation code is built from. pydicom's int(arg, 16) has no
width, so ParseUint is the port.
The compile failures were all one mistake: a declared value length is unsigned
32 bits, and elementHeader.Length was an int. 0xFFFFFFFF -- undefined length --
is -1 in an int on a 32-bit platform, so simply casting to make it build would
have produced a binary that misread every undefined-length sequence. Carrying
uint32 through the header, deferred-read and sequence paths instead removed two
existing uint32() casts at call sites.
CI now vets eight cross targets and runs the full suite on linux/386, because
the mask bug compiled fine and only failed when run. mips and mipsle are left
out: purego does not build for them.
Tests that need a native codec skip instead of failing where no library exists,
and the README documents platform support and the per-platform binary cost --
one platform's libraries per binary, 11.0 MB on linux/amd64 against 24.3 MB if
all twelve were embedded unconditionally.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bumps
golibjpegandgoopenjpegto v1.3.0. Those releases add prebuiltlibraries for
darwin/amd64andwindows/arm64(six platforms each, up fromfour) and — the part that matters here — they now build on every platform Go
targets rather than only the ones they ship a library for, returning an error
wrapping
ErrUnsupportedPlatforminstead of failing to compile.That made "importing godicom is safe anywhere" a claim worth checking. It was
false: godicom itself did not compile for a 32-bit target. Fixing that turned up
two bugs that have nothing to do with cross-compilation.
Fixed
The data dictionary answered
USfor nearly every tag on 32-bitAll 88 repeater masks were built with
fmt.Sscanf(mask2Str, "%x", &mask2)intointfields, with the error discarded. A mask like0xFF00FFFFdoes not fitin a 32-bit
int, so on those platforms all 88 parsed to zero — and(tag ^ value) & 0 == 0is true for every tag. Whichever repeater happened tobe first in map iteration order became the answer for every non-private tag:
LookupVR(PatientName)returnedUS, notPNUS, notULIsRepeaterTagwas true for everythingA probe on
windows/386before the fix: 88 of 88 masks hadmask == 0, andevery tag matched
50xx0105. Rebuilt with nibble arithmetic intouint32—there is no error to drop and no platform-dependent width to overflow. 64-bit
platforms were unaffected.
ParseTagrejected every hex tag with a group ≥0x8000— on all platformsstrconv.ParseInt(v, 16, 32)rejects anything from0x80000000up, so:Those are the item, item-delimiter and sequence-delimiter tags that godicom's own
sequence and encapsulation code is built from, and the ones a
JSONKeyround-trip is most likely to hit. pydicom's
tag.pyusesint(arg, 16), whichhas no width at all, so
ParseUintis the correct port. The parenthesised form(FFFE,E000)always worked — it parses two 16-bit halves.Confirmed the new test catches it by reverting to
ParseInt: 5 subtests fail.encaps.Encapsulatedid not compile on 32-bitThe Basic Offset Table guard read
total > (1<<32)-1. That is exact in pydicom(
encaps.py:1124) because Python integers are arbitrary-precision; in Go theuntyped constant overflows
intat compile time on a 32-bit target.Changed
The compile failures were all one mistake: a declared value length is unsigned
32 bits, and
elementHeader.Lengthwas anint.0xFFFFFFFF— undefinedlength — is
-1in aninton a 32-bit platform, so casting to make it buildwould have shipped a binary that silently misread every undefined-length
sequence and every encapsulated Pixel Data element. A merely-compiling build
would have been worse than the compile error.
So
uint32is carried through the header, deferred-read and sequence pathsinstead. No exported signature changes, and it removed two existing
uint32(length)casts at call sites.Added
windows/386,linux/386,linux/arm(incl.
GOARM=5),linux/riscv64,linux/ppc64le,js/wasm,wasip1/wasm,and runs the full test suite on
linux/386.go vetrather thango buildso_test.gofiles are type-checked too. The test run is the point:the mask bug compiled fine and only failed when executed.
linux/mipsandlinux/mipsleare excluded — purego does not build for them(
func.go:184:9: undefined: addStruct), which is upstream.skipWithoutNativeCodecin bothgodicomandpixelstest packages, so the15 tests that genuinely need a native codec skip rather than fail where no
library exists. It cannot mask a regression on a supported platform: on 64-bit
exactly 1 test skips (a pre-existing fixture skip), so all 14 guarded tests
still run and assert there.
Docs
embeds one platform's libraries, never all twelve:
cmd/godicomonlinux/amd64is 11.0 MB, where embedding every library unconditionally wouldmake it 24.3 MB. All numbers measured, not estimated.
CompressPixelData's doc comment listed its supported targets without HTJ2K,which
pixels.EncodeFramehas dispatched on for some time.Verification
go test ./...(amd64)GOARCH=386 go test ./...(real run via WoW64)go vet ./...× 8 cross targetsstaticcheck -checks=allgofmt -l🤖 Generated with Claude Code