Add support for 64 bit chunks, including 64 bit chunk dimensions #5965

fortnern · 2025-11-02T21:57:53Z

Various related changes, including refactoring part of dataset creation, and reworking how layout versions are calculated.

Needs more testing of filters that create very large chunks, but that will need code to turn it off in cases where we can't allocate >4GiB buffers. I tested this manually by increasing the expansion ration in the expand2 test in dsets, and it passed everything up until it tried to expand the datasets with H5Dset_extent() and my laptop ran out of disk space.

We should add code and testing to handle the case where the "size of size" is set less than 8 bytes. This is not a new issue, since it can be set to 2.

I will file issues for these, but I don't think they are necessary for the release.

Important

Add support for 64-bit chunk dimensions, refactor dataset creation, and rework layout version calculations.

Behavior:
- Support for 64-bit chunk dimensions added, removing 32-bit limitations in H5D.c, H5Dbtree.c, and H5Dbtree2.c.
- New macro H5D_CHUNK_ENCODE_SIZE_CHECK added to H5Dchunk.c to check chunk size encoding.
- Refactored dataset creation and layout version calculation in H5Dchunk.c and H5Dint.c.
Data Types:
- Changed uint32_t to hsize_t for chunk sizes in H5Dchunk.c, H5Dbtree.c, and H5Dbtree2.c.
- Updated function signatures to use hsize_t for chunk sizes in H5D.c and H5Dpkg.h.
Misc:
- Updated warning flags in config/gnu-warnings/4.8 and config/gnu-warnings/cxx-4.8.
- Removed H5D__layout_set_version and H5D__layout_set_latest_indexing from H5Dlayout.c.

^{This description was created by}^{for 9a04ce5. You can customize this summary. It will automatically update as commits are pushed.}

test/big.c

fortnern · 2025-11-02T22:27:50Z

src/H5Dchunk.c

    assert(dset->shared->layout.u.chunk.ndims > 0);

+    /* In this function, some of these sizes may have already been set since they are sometimes stored in the
+     * file. If this is the case, verify the calculated sizes match the stored sizes. */


This is a strange thing I found - we are storing these things but not using the stored values. I added these file integrity checks here.

fortnern · 2025-11-02T22:30:53Z

src/H5Dchunk.c

+
+    /* Calculate layout version */
+    /* Do not downgrade version */
+    version_req = layout->version;


This function took over the functions of the deleted H5D__layout_set_latest_indexing(), and now handles the main layout version calculation. We must upgrade to at least version_req, and we will upgrade to version_perf if allowed by the bounds, and up to the low bound if not.

…5 if allowed by low bound.

fortnern · 2025-11-02T22:44:18Z

src/H5Dlayout.c

- *-------------------------------------------------------------------------
- */
-herr_t
-H5D__layout_set_version(H5F_t *f, H5O_layout_t *layout)


Functionality moved to the construct callbacks and H5D__create (for version bounds check)

fortnern · 2025-11-02T22:44:35Z

src/H5Dlayout.c

- *-------------------------------------------------------------------------
- */
-herr_t
-H5D__layout_set_latest_indexing(H5D_t *dset)


Functionality moved to H5D__chunk_construct()

fortnern · 2025-11-02T22:45:55Z

src/H5Pdcpl.c

+        HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, FAIL, "can't get low/high bounds from API context");
+
+    /* Determine version - only upgrade for chunked datasets that have a dimension size >32 bits */
+    if (H5D_CHUNKED == layout->type)


The new layout property encoding also needs a test - I will file an issue

fortnern · 2025-11-02T22:47:46Z

test/big.c

 }

+static int
+run_tests(char *filename, hid_t fapl, fsizes_t testsize, int wrt_n)


Added many test cases to this test

fortnern · 2025-11-02T22:49:06Z

test/tmisc.c

    dcpl = H5Pcreate(H5P_DATASET_CREATE);
    CHECK(dcpl, FAIL, "H5Pcreate");

-    /* Try to use chunked storage for this dataset */


This is now allowed. We could cap the high bound to test that it's rejected in that case but that's already tested elsewhere.

fortnern · 2025-11-02T22:50:02Z

test/dsets.c

    return FAIL;
 } /* end test_simple_io() */

+/*-------------------------------------------------------------------------


I wrote this to test the code I wrote for scalar chunk datasets before I realized that was impossible. There was some existing code that seemed to imply it would work.

src/H5Dchunk.c

mattjala

Besides clarifying questions, LGTM

src/H5Ppublic.h

…unks

release_docs/CHANGELOG.md

brtnfld · 2025-11-03T22:39:35Z

src/H5Dpkg.h

+            tconv_buf_hsize += (PIECE_INFO)->piece_points *                                                  \
+                               MAX((DINFO)->type_info.src_type_size, (DINFO)->type_info.dst_type_size);      \
+            (IO_INFO)->tconv_buf_size = (size_t)tconv_buf_hsize;                                             \
+            if (H5_UNLIKELY((hsize_t)(IO_INFO)->tconv_buf_size != tconv_buf_hsize))                          \


Does this detect when hsize_t itself overflows during the addition at line 113?

On 64-bit systems where sizeof(hsize_t) == sizeof(size_t) == 8:
The addition overflows first, wrapping tconv_buf_hsize
The wrapped value gets cast to size_t and back
The comparison is wrapped_value != wrapped_value -> FALSE
Overflow goes undetected

Is there some constraint that prevents the multiplication from overflowing?

if (!(PIECE_INFO)->in_place_tconv) { \ hsize_t tconv_buf_hsize; \ hsize_t old_size; \ H5_CHECKED_ASSIGN(tconv_buf_hsize, hsize_t, (IO_INFO)->tconv_buf_size, size_t); \ old_size = tconv_buf_hsize; \ tconv_buf_hsize += (PIECE_INFO)->piece_points * \ MAX((DINFO)->type_info.src_type_size, (DINFO)->type_info.dst_type_size); \ if (H5_UNLIKELY(tconv_buf_hsize < old_size || tconv_buf_hsize > SIZE_MAX)) \ (IO_INFO)->tconv_buf_overflow = true; \ else \ (IO_INFO)->tconv_buf_size = (size_t)tconv_buf_hsize; \ }

It probably wouldn't handle a 64 bit value overflowing correctly, but I'm sure this would be far from the only place in the library that does so.

By the time we need to start worrying about that we'll be doing calculations in 128 bit and checking for overflow that way

Is that true now that we have complex support? So you don't expect multi-exabyte buffer accumulations until 128-bit systems? the check is two lines, it does not seem like a big deal to check it.

I'm not sure I follow re: complex support. I suppose checks like this could potentially guard against wildly invalid input data. I'll add it if you want me to, just be aware that there are many other places that don't guard against uin64 overflow (I'm not sure I've seen anywhere that does), so the check will likely have no effect until we add checks in most of those other places as well.

Check added

jhendersonHDF · 2025-11-04T01:01:22Z

src/H5Dchunk.c

                HGOTO_ERROR(H5E_DATASET, H5E_CANTFILTER, FAIL, "output pipeline failed");
-#if H5_SIZEOF_SIZE_T > 4
-            /* Check for the chunk expanding too much to encode in a 32-bit value */
-            if (nbytes > ((size_t)0xffffffff))


Similar to the comment above, just highlighting a few places for review in case they also need a H5D_CHUNK_ENCODE_SIZE_CHECK check. Here

This is not necessary here because the check will be performed when the chunk is allocated on disk in H5D__chunk_file_alloc

jhendersonHDF · 2025-11-04T01:01:37Z

src/H5Dchunk.c


-#if H5_SIZEOF_SIZE_T > 4
-                    /* Check for the chunk expanding too much to encode in a 32-bit value */
-                    if (nbytes > ((size_t)0xffffffff))


Here as well

jhendersonHDF · 2025-11-04T01:01:58Z

src/H5Dchunk.c

            HGOTO_ERROR(H5E_PLINE, H5E_CANTFILTER, H5_ITER_ERROR, "output pipeline failed");
-#if H5_SIZEOF_SIZE_T > 4
-        /* Check for the chunk expanding too much to encode in a 32-bit value */
-        if (nbytes > ((size_t)0xffffffff))


jhendersonHDF

Generally looks good, just a few comments around potential checks needed

H5D_INIT_PIECE_TCONV macro

overflow check

fortnern · 2025-11-04T15:50:34Z

I realized the H5D_CHUNK_ENCODE_SIZE_CHECK in H5D__chunk_direct_write is also redundant so I removed that too. I'll leave the macro for now even though it's only used in H5D__chunk_file_alloc

explanatory comment

Add support for 64 bit chunks, including 64 bit chunk dimensions

9a04ce5

fortnern requested review from bmribler, brtnfld, byrnHDF, derobins, glennsong09, jhendersonHDF, lrknox, mattjala, qkoziol and vchoi-hdfgroup as code owners November 2, 2025 21:57

github-project-automation bot added this to HDF5 - TRIAGE & TRACK Nov 2, 2025

github-project-automation bot moved this to To be triaged in HDF5 - TRIAGE & TRACK Nov 2, 2025

Committing clang-format changes

8620437

fortnern assigned lrknox, jhendersonHDF and mattjala Nov 2, 2025

ellipsis-dev bot reviewed Nov 2, 2025

View reviewed changes

test/big.c Outdated Show resolved Hide resolved

fortnern commented Nov 2, 2025

View reviewed changes

fortnern added 2 commits November 2, 2025 16:38

Fix extra semicolon and promote singel chunk index to layout version …

c7ad5ef

…5 if allowed by low bound.

Fix uninitialized warnings.

c7e2ff1

fortnern commented Nov 2, 2025

View reviewed changes

Committing clang-format changes

286c044

fortnern commented Nov 2, 2025

View reviewed changes

Spelling

e29bde0

mattjala reviewed Nov 3, 2025

View reviewed changes

src/H5Dchunk.c Show resolved Hide resolved

mattjala previously approved these changes Nov 3, 2025

View reviewed changes

jhendersonHDF reviewed Nov 3, 2025

View reviewed changes

src/H5Ppublic.h Outdated Show resolved Hide resolved

fortnern added 2 commits November 3, 2025 15:44

Add chunk encoding size check to H5D__chunk_allocate

9472ecd

Merge branch '64bit_chunks' of github.com:fortnern/hdf5 into 64bit_ch…

9e8e0fa

…unks

fortnern dismissed mattjala’s stale review via 9e8e0fa November 3, 2025 21:45

jhendersonHDF reviewed Nov 3, 2025

View reviewed changes

release_docs/CHANGELOG.md Outdated Show resolved Hide resolved

Reword changlog note and reference manual entry for H5Pset_chunk()

a4b8972

brtnfld reviewed Nov 3, 2025

View reviewed changes

mattjala previously approved these changes Nov 3, 2025

View reviewed changes

jhendersonHDF reviewed Nov 4, 2025

View reviewed changes

Remove (revert) redundant chunk encoding size check

07c1aea

fortnern dismissed mattjala’s stale review via 07c1aea November 4, 2025 01:58

Add check for type conversion buffer size overflowing hsize_t to

fdad5cd

H5D_INIT_PIECE_TCONV macro

jhendersonHDF previously approved these changes Nov 4, 2025

View reviewed changes

mattjala previously approved these changes Nov 4, 2025

View reviewed changes

Remove another redundant chunk siz check and add an H5_UNLIKELY to an

204b0f9

overflow check

fortnern dismissed stale reviews from mattjala and jhendersonHDF via 204b0f9 November 4, 2025 15:49

Rename open parameter for layout init callback to open_op, add

8c54408

explanatory comment

mattjala approved these changes Nov 4, 2025

View reviewed changes

lrknox approved these changes Nov 4, 2025

View reviewed changes

lrknox merged commit 1093a18 into HDFGroup:develop Nov 4, 2025
95 checks passed

github-project-automation bot moved this from To be triaged to Done in HDF5 - TRIAGE & TRACK Nov 4, 2025

fortnern deleted the 64bit_chunks branch November 14, 2025 15:17

Uh oh!

Add support for 64 bit chunks, including 64 bit chunk dimensions #5965

Add support for 64 bit chunks, including 64 bit chunk dimensions #5965

Uh oh!

Conversation

fortnern commented Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fortnern Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattjala left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brtnfld Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fortnern Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhendersonHDF left a comment

Choose a reason for hiding this comment

Uh oh!

fortnern commented Nov 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

fortnern commented Nov 2, 2025 •

edited

Loading

fortnern Nov 2, 2025 •

edited

Loading

brtnfld Nov 3, 2025 •

edited

Loading

fortnern Nov 4, 2025 •

edited

Loading