Skip to content

Commit 7ba6738

Browse files
collaredLizardclaude
authored andcommitted
Fix MGARD-X HIP Huffman lossless (codec correctness + code-length OOB)
Four independent bugs in the MGARD-X HIP ParallelHuffman lossless path: three in the Huffman codec (canonical-codeword generation, header serialization, and codeword-length overflow handling) made GPU-compressed streams undecodable/oversized or abort, and one out-of-bounds device read in the parallel code-length generation faulted on the GPU. All verified on gfx90a (Frontier MI250X/MI210): HIP-compressed streams now round-trip and are byte-compatible with the SERIAL decoder. HuffmanWorkspace.hpp: allocate `status` as non-managed device memory. It was managed (hipMallocManaged); atomicMin on fine-grained memory is unreliable on ROCm/gfx90a, so GenerateCW::Operation4's atomic min never updated newCDPI. The Huffman length-group boundary was never found, all symbols collapsed into one group, and the canonical codewords came out with a constant offset -- codebook and decodebook mutually inconsistent, so the stream was undecodable by any backend. Huffman.hpp: default-initialize `outlier_count = 0`. ComposedLosslessCompressor::Compress -> CompressPrimary never writes outlier_count, but Serialize reads it to size compressed_data.resize(). The uninitialized value produced a garbage-sized allocation (hipMalloc OOM, or an invalid Copy1D). GetCodebook.hpp: throw instead of exit(1) when the longest codeword exceeds the H-type budget (sizeof(H)*8 - 8 bits), so callers can catch it and fall back to another lossless backend or a smaller dict_size. GenerateCL.hpp: bounds-check histogram[_lNodesCur + _curLeavesNum] in the parallel code-length generation; when every remaining leaf joins the batch the index reached dict_size (one past end of the nz_dict_size histogram), faulting on GPU. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent b40d1a7 commit 7ba6738

4 files changed

Lines changed: 39 additions & 10 deletions

File tree

include/mgard-x/Lossless/ParallelHuffman/GenerateCL.hpp

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -349,15 +349,30 @@ class GenerateCLFunctor : public HuffmanCLCustomizedFunctor<DeviceType> {
349349
(*status((IDX)_iNodesFront)) = (*status((IDX)_iNodesRear));
350350
}
351351
/* Odd number of nodes to merge - leave out one*/
352-
// If number of participating leaf node is zero OR
353-
// The highest frequency leaf node is less than the
354-
// highest frequency internal node
352+
// Remove one internal node (keep it for the next outer iteration) if:
353+
// (a) there are no leaf nodes in the batch (_curLeavesNum == 0), OR
354+
// (b) the first leaf node NOT in the batch has a frequency <= the
355+
// highest-frequency internal node in the batch, meaning it is
356+
// cheaper to defer that internal node.
357+
//
358+
// BUG FIX: The original code read histogram[_lNodesCur + _curLeavesNum]
359+
// unconditionally. When _curLeavesNum == dict_size - _lNodesCur (i.e.,
360+
// all remaining leaf nodes qualify for the batch), the index equals
361+
// dict_size, which is one past the end of the nz_dict_size-sized
362+
// histogram array, causing a GPU memory access fault.
363+
// FIX: Guard the histogram access with the bounds check
364+
// (_lNodesCur + _curLeavesNum < dict_size).
365+
// When there is no next leaf outside the batch, the condition is treated
366+
// as false and execution falls through to the else branch (remove one
367+
// leaf from the batch instead).
355368
else if (((*status((IDX)_iNodesSize)) != 0) and
356369
((*status((IDX)_curLeavesNum)) == 0 or
357-
(*histogram((IDX)(*status((IDX)_lNodesCur)) +
358-
(*status((IDX)_curLeavesNum))) <=
359-
*iNodesFreq(
360-
(IDX)MOD((*status((IDX)_iNodesRear)) - 1, dict_size))))) {
370+
(((*status((IDX)_lNodesCur)) + (*status((IDX)_curLeavesNum)) <
371+
dict_size) and
372+
(*histogram((IDX)(*status((IDX)_lNodesCur)) +
373+
(*status((IDX)_curLeavesNum))) <=
374+
*iNodesFreq((IDX)MOD((*status((IDX)_iNodesRear)) - 1,
375+
dict_size)))))) {
361376
(*status((IDX)_mergeRear)) =
362377
MOD((*status((IDX)_mergeRear)) - 1, dict_size);
363378
(*status((IDX)_iNodesFront)) =

include/mgard-x/Lossless/ParallelHuffman/GetCodebook.hpp

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@
1414
#include "ReorderByIndex.hpp"
1515
#include "ReverseArray.hpp"
1616

17+
#include <stdexcept>
18+
#include <string>
19+
1720
#ifndef MGARD_X_GET_CODEBOOK_TEMPLATE_HPP
1821
#define MGARD_X_GET_CODEBOOK_TEMPLATE_HPP
1922

@@ -119,7 +122,16 @@ void GetCodebook(int dict_size,
119122
<< "Huffman codeword representation requires at least "
120123
<< max_CL + 8 << " bits (longest codeword: " << max_CL << " bits)"
121124
<< std::endl;
122-
exit(1);
125+
// Throw (instead of exit) so callers can catch and fall back to another
126+
// lossless backend (e.g. raw Zstd) or retry with a smaller huff_dict_size.
127+
// A longer dictionary makes the tree deeper, so a degenerate/low-entropy
128+
// input can produce codewords exceeding the H-type budget (sizeof(H)*8 -
129+
// 8).
130+
throw std::runtime_error(
131+
"MGARD-X Huffman: longest codeword (" + std::to_string(max_CL) +
132+
" bits) exceeds the " + std::to_string(max_CW_bits) +
133+
"-bit budget of the H code type; retry with a smaller huff_dict_size "
134+
"or a different lossless backend");
123135
}
124136

125137
DeviceLauncher<DeviceType>::Execute(

include/mgard-x/Lossless/ParallelHuffman/Huffman.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -446,7 +446,7 @@ class Huffman : public LosslessCompressorInterface<S, DeviceType> {
446446
bool initialized;
447447
SIZE max_size;
448448
size_t primary_count;
449-
ATOMIC_IDX outlier_count;
449+
ATOMIC_IDX outlier_count = 0;
450450
int dict_size;
451451
int chunk_size;
452452
size_t huffmeta_size;

include/mgard-x/Lossless/ParallelHuffman/HuffmanWorkspace.hpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,9 @@ class HuffmanWorkspace {
135135
copyIsLeaf_array = Array<1, int, DeviceType>({dict_size});
136136
copyIndex_array = Array<1, int, DeviceType>({dict_size});
137137
_d_codebook_array_org = Array<1, H, DeviceType>({dict_size});
138-
status_array = Array<1, int, DeviceType>({(SIZE)16}, false, true);
138+
status_array = Array<1, int, DeviceType>(
139+
{(SIZE)16}, false,
140+
false); // non-managed: atomicMin on managed mem unreliable on ROCm
139141
SIZE mblocks = (DeviceRuntime<DeviceType>::GetMaxNumThreadsPerTB() /
140142
DeviceRuntime<DeviceType>::GetWarpSize()) *
141143
DeviceRuntime<DeviceType>::GetNumSMs();

0 commit comments

Comments
 (0)