Repository navigation
Use off_t instead of the glibc-specific off64_t in FileStreamer - #228
hjmjohnson wants to merge 1 commit into
Conversation
21cb6bd to
f122877
Compare
|
@malaterre Improved code compilation. |
|
Full-toolkit musl results, since "it builds" deserves numbers. Pristine
Environment and exact methodAlpine 3.24.1, gcc 15.2.0, musl x86_64 ( The source tree is a pristine git archive gitupstream/master | tar -x -C ./pristine
docker run --rm --platform linux/amd64 \
-v "$PWD/pristine:/src:ro" \
-v /path/to/gdcmData:/data:ro \
alpine:latest sh -c '
apk add -q build-base cmake ninja patch
cp -r /src /work && cd /work
# omit the next line for the baseline column
patch -p1 < /sp/pr228.patch
cmake -G Ninja -S /work -B /bld \
-DCMAKE_BUILD_TYPE=Release \
-DGDCM_BUILD_TESTING=ON \
-DGDCM_BUILD_APPLICATIONS=ON \
-DGDCM_DATA_ROOT=/data
ninja -C /bld -j"$(nproc)" -k 0 # -k 0 so the baseline reports ALL errors, not the first
cd /bld && ctest -j"$(nproc)"
'
Baseline error breakdownAll 67 errors come from a single file. Grouped by source file: The first three are the reported symptom; the remaining 64 are cascade from the three wrapper signatures failing to parse: Test results, and why 27 fail
So: no test regresses on musl relative to macOS, and nothing in the failure set is attributable to this patch. Reproduced twice from scratch with identical results. The 32-bit |
|
Corroboration: Alpine has been carrying a downstream patch for this, Their patch also makes the case for using a distinct alias name rather than renaming in place. What the Alpine patch does, and why this PR does it differently
#if defined(_WIN32) && (defined(_MSC_VER) || defined(__MINGW32__))
#include <io.h>
-typedef int64_t off64_t;
+typedef int64_t off_t; // redefines the system off_t
#else
#if defined(__APPLE__) || ...
-# define off64_t off_t
+# define off_t off_t // no-op self-definition
#endif-// Basically enforce use of off64_t over off_t since on windows off_t is pretty
+// Basically enforce use of off_t over off_t since on windows off_t is pretty
If this lands, Separately, on whether this was already reported: I searched the SourceForge tracker via its REST API ( |
|
@malaterre A few PR's for issues identified from ITK buildings. |
|
@malaterre Is there anything I can do to assist with the GDCM reviews? |
|
@malaterre Could the ITK team be of assistance in reviewing and updating GDCM? We have several upstream patches outstanding, and would like to avoid local vendering of separate code paths if possible. |
gdcmFileStreamer.cxx used off64_t and hand-maintained a list of platforms on which it must be #defined to off_t. musl-based distributions (Alpine) were not on that list and fail with "off64_t has not been declared": musl gates its LFS64 names on _LARGEFILE64_SOURCE and, unlike glibc, does not imply that from _GNU_SOURCE. musl is also removing those aliases, so enabling _LARGEFILE64_SOURCE is not a durable answer. Extending the list is not a fix either. off64_t is a typedef, so the preprocessor cannot test for it; every guard is a proxy for "which libc is this", which is why the list grew one platform at a time. The glibc build worked only because the C++ driver implicitly defines _GNU_SOURCE, which turns on __USE_LARGEFILE64; the same block does not compile as C. Separately, the _FILE_OFFSET_BITS 64 definition in this file was a no-op. It sat after the gdcm*.h include block, which transitively pulls <features.h>, so it was ignored. On 32-bit glibc off_t therefore stayed 32-bit and FSeeko/FTello/FTruncate silently narrowed, breaking writes past 2 GB. Move the definition ahead of all includes and replace off64_t with a GDCM-owned gdcm::offset_t: off_t on POSIX, int64_t on the Windows CRT. This deletes the __APPLE__ || __FreeBSD__ || __OpenBSD__ || __NetBSD__ || __EMSCRIPTEN__ list without adding anything. A static_assert makes a 32-bit off_t a build error rather than silent truncation. offset_t does not appear in gdcmFileStreamer.h, so the change is contained to one translation unit and alters no public API. Verified: Alpine/musl reproduces the reported error before the change and compiles clean after; on 32-bit glibc the static_assert fires with the old define placement and passes with the new one; macOS arm64 full build at -Wall -Wextra with all six TestFileStreamer tests passing and no change to the pre-existing failure set. Reported downstream as InsightSoftwareConsortium/ITK#6779. Assisted-by: Claude Code -- cross-platform instrumentation and patch drafting
f122877 to
7f17c88
Compare
gdcmFileStreamer.cxxfails to build on musl-based distributions (Alpine) becauseoff64_tis a glibc LFS64 name. This replaces it with a GDCM-ownedgdcm::offset_t, deleting the hand-maintained platform list without adding to it.It also fixes a latent large-file bug found while investigating: the file's
_FILE_OFFSET_BITS 64definition is placed after the include block, which makes it a no-op, so writes past 2 GB are silently broken on 32-bit glibc today.Reported downstream as InsightSoftwareConsortium/ITK#6779.
Question for you before this is worth reviewing in detail: defining
_FILE_OFFSET_BITSin a single TU changesstruct statlayout andoff_twidth for that TU only. It is safe here (the onlystruct statuse is a localfstat(), andoffset_tdoes not appear ingdcmFileStreamer.h), but if you would rather set it project-wide in CMake I am happy to redo it that way; say the word.Root cause
musl gates its LFS64 names strictly on
_LARGEFILE64_SOURCEand, unlike glibc, does not imply that from_GNU_SOURCE:musl upstream is also removing these aliases, so enabling
_LARGEFILE64_SOURCEis not a durable answer. musl'soff_tis unconditionally 64-bit, sooff_twas always the right type there.Extending the platform list would fix Alpine but not the class of bug.
off64_tis a typedef, so the preprocessor can never test for it, so every guard is necessarily a proxy for "which libc is this", which is why the list has grown one platform at a time.The glibc build works only because the C++ frontend implicitly defines
_GNU_SOURCE, which turns on__USE_LARGEFILE64. Nothing in GDCM requests it, and the same block does not compile as C:Separately,
#define _FILE_OFFSET_BITS 64must precede every libc header. In this file it sits after ninegdcm*.hincludes, all of which transitively pull<string>→<features.h>. Once that include guard is set, a later_FILE_OFFSET_BITSis ignored.__GNU_LIBRARY__is likewise only defined after a libc header has been read, so the guard is order-dependent in the same way.What changed
#define _FILE_OFFSET_BITS 64ahead of all includes, unconditionally and#ifndef-guarded. It has no effect on macOS, the BSDs, musl, or MSVC, whereoff_tis already 64-bit or unused for file offsets. mingw-w64 honors it, which is harmless becauseoffset_tisint64_ton the Windows CRT.off64_twithgdcm::offset_t:off_ton POSIX,int64_ton the Windows CRT. This deletes the__APPLE__ || __FreeBSD__ || __OpenBSD__ || __NetBSD__ || __EMSCRIPTEN__list without adding anything.static_assert(sizeof(offset_t) >= 8, ...)so a 32-bitoff_tis a build error rather than silent truncation.<cstdint>forint64_t, which was previously reached only transitively.The one remaining
#iftests "is this the Windows CRT", a genuine API difference.offset_tdoes not appear ingdcmFileStreamer.h, so this is contained to one translation unit and changes no public API._GNU_SOURCEoff_toutcome_FILE_OFFSET_BITStakes effectTesting
Synthetic probes, three libc/arch combinations:
off_t=8off_t=8off_t=4off_t=8off_t=8off64_tlogicoffset_tlogicAgainst the real source rather than probes:
error: 'off64_t' has not been declaredatgdcmFileStreamer.cxx:55,:68,:81, reproducing the report.-Wall -Wextra.static_assertpassing is itself proof thatoff_tis now 64-bit in GDCM's own TU.static_assert failed: GDCM requires 64-bit file offsets, evidence that 32-bit builds truncate today.macOS arm64, full build,
-DGDCM_BUILD_TESTING=ONwithGDCM_DATA_ROOTset,-Wall -Wextra:gdcmPrivateTag.h.TestFileStreamer1..6: 6/6 pass.master(verified by reverting only this file and re-running the same set), so they are pre-existing and unrelated.Not covered: MSVC, MinGW-w64, and a >2 GB
FileStreamerround-trip: no such fixture exists ingdcmData, so the 32-bit claim rests on thestatic_assertand the probes above rather than a runtime test.Verification, 2026-10-26 (after review fixups; gdcmData and gdcmDataExtra)
TestInvalidDICOMFiles)off_twithout the define)static_assertpasses; failure set identical to masterThe review fixups only shortened two comments; the code is unchanged.
Follow-ups, deliberately not bundled here
Hoist
_FILE_OFFSET_BITS=64to CMake (target_compile_definitions(gdcmMSFF PRIVATE ...)or global) and drop the per-file define. This is whatAC_SYS_LARGEFILEdoes and removes the single-TU caveat above entirely. Happy to open it if you prefer that shape.MinGW-w64 truncates at 2 GB. All three wrappers take 32-bit paths under
__MINGW32__(fseek,ftell,_chsizewith along), and mingw-w64 also defines__MINGW32__, so every mingw-w64 build silently caps at 2 GB. mingw-w64 provides_fseeki64,_ftelli64and_chsize_s, so those branches can likely fold into the MSVC path. Thestatic_asserthere does not catch it: the type is fine, the calls are wrong. Needs a real mingw-w64 build to confirm; untouched by this PR.Testing/Source/Common/Cxx/TestSystem2.cxxhas the same dead_FILE_OFFSET_BITSplacement (aftergdcmSystem.h, guarded on__GNU_LIBRARY__), so itssizeof(off_t)check still sees 4 on i386 glibc.32-bit Android below API 24 likely loses
fseeko/ftellounder_FILE_OFFSET_BITS=64(bionic gates the 64-bit variants on API 24). Unverified; a guard is easy if anyone builds that target.