Fix AppendVector random access iterator#917
Open
Symmetricity wants to merge 1 commit into
Open
Conversation
AppendVector advertises a random access iterator and is passed to sorting algorithms that may rely on the full iterator contract. Newer standard library implementations can exercise distance, comparison, offset, indexing, and postfix operations more aggressively, so storing chunk and offset separately leaves room for invalid iterator arithmetic. Track iterator position as one absolute index and cover the random access operations in the existing append vector test. This keeps the container shape unchanged while making the advertised iterator category match the supported operations. Co-authored-by: Codex <noreply@openai.com>
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.
This PR was prepared with AI assistance.
AppendVector::Iteratoradvertises itself as a random access iterator and isused by tile sorting code through Boost sorting algorithms. Several iterator
operations were implemented by repeatedly converting between chunk/offset and
absolute positions, while other random-access operations expected by sorting
implementations were missing.
This changes the iterator to track a single absolute index, then derive the
underlying chunk and offset only when dereferencing. It also fills out the
random-access operations used by standard/Boost sorting code, including
comparisons, iterator distance, offset addition/subtraction, and indexing.
The existing
append_vectortest now exercises the random-access operations,including postfix movement and movement across the internal chunk boundary.
Why
On a current MSVC toolchain, an Austria-sized OpenMapTiles build could crash
while finalizing z6 output tiles, inside sorting of
AppendVectoriterators.Diagnostics showed the sorter reaching an invalid iterator position before the
beginning of the vector. Making the iterator arithmetic use one absolute
position avoids inconsistent chunk/offset state and makes the advertised
iterator category match the supported operations.
Crash evidence
This appears to be exposed by newer MSVC/STL behavior rather than by the input
data. Older VS 2022/MSVC builds used locally had not reproduced this crash, but
the Windows artifact from a newer GitHub Actions release run on the fork
(
Symmetricity/tilemakerrun27551026617, artifact7640269070) reproducedan access violation while generating Austria tiles. The user-visible failure was
0xc0000005during z6 finalization, around:osm: finalizing z6 tile 2135/4096Rebuilding the same code shape with the current Windows toolchain
(MSVC 19.51 / VS 18 2026 Build Tools) also reproduced the crash. A diagnostic
build showed the active sort was
OutputObjectXYover about 12.7 millionobjects:
source=osm,type=OutputObjectXY,indexZoom=14,threads=8,objects=12680604vec=-1,offset=8191,absolute=-1vec=0,offset=8190,absolute=8190That points to an iterator arithmetic/contract issue being surfaced by the
newer sort implementation. With this patch, the same MSVC 19.51 build completed
both Austria MBTiles and PMTiles generation.
Testing
cmake -S . -B ... -DCMAKE_BUILD_TYPE=RelWithDebInfocmake --build ... --parallel 8make test_append_vectormake testx64-windows-static-mdvcpkg tripletPerformance
Compared an upstream master build against the same commit plus this change,
using the same RelWithDebInfo build shape and OpenMapTiles profile.
The Austria PMTiles runtime/RSS profile used three alternating runs in each
order. Wall time was effectively unchanged between the forward and reverse
orders. A small peak-RSS high-water movement was observed, but heaptrack and
Massif on a smaller fixture showed allocation counts and heap shape unchanged,
so this does not appear to introduce a meaningful allocation regression.