Skip to content

Clarify pptr checked and unchecked access modes#320

Merged
netkeep80 merged 9 commits intonetkeep80:mainfrom
konard:issue-312-65d10bb90ae2
Apr 19, 2026
Merged

Clarify pptr checked and unchecked access modes#320
netkeep80 merged 9 commits intonetkeep80:mainfrom
konard:issue-312-65d10bb90ae2

Conversation

@konard
Copy link
Copy Markdown
Contributor

@konard konard commented Apr 19, 2026

Summary

Fixes #312.

This PR makes pptr<T> -> T* access semantics explicit:

  • Adds resolve_checked() for public live-block access.
  • Adds resolve_unchecked() for deliberate internal/raw offset access.
  • Keeps resolve() as a compatibility alias for the checked public path.
  • Routes pptr::resolve(), operator*, and operator-> through checked access.
  • Updates internal forest-domain call sites to use explicit unchecked access.
  • Corrects non-aligned block-header handling for SmallAddressTraits so checked and unchecked typed pptr resolution use the same canonical granule-aligned user pointer.
  • Keeps internal raw block payload access available for deallocation paths where the physical payload starts immediately after the compact block header.

Review And CI Follow-Up

Addressed review 4136392610, sanitizer CI follow-up, and review 4136442981:

  • resolve_unchecked() delegates to the canonical raw_user_ptr_from_pptr() conversion.
  • resolve_checked() validates the same pointer that it returns.
  • make_pptr_from_raw() is now the canonical constructor for public pptrs: it finds the owning block and returns block_idx + kBlockHdrGranules.
  • allocate_typed(), allocate_typed(count), reallocate_typed(), create_typed(), and create_typed_unlocked() now use that canonical factory path.
  • create_typed() constructs at the canonical public user pointer and destroy_typed() destroys at that same public pointer before freeing the physical block payload.
  • intern_symbol_unlocked() writes pstringview contents at the canonical public pointer and stores the canonical pptr.
  • Public pstringview interning now uses the same canonical public pptr model for variable-size stringview blocks.
  • Block-header lookup from pptr now subtracts kBlockHdrGranules, not sizeof(Block), so metadata access agrees with canonical public offsets for non-aligned headers.
  • Added focused SmallEmbeddedStaticConfig regressions for both create_typed() and interned symbol/domain lookup in the small/non-aligned configuration.

Tests

  • cmake -S . -B build
  • cmake --build build --target test_issue312_access_modes
  • ctest --test-dir build -R test_issue312_access_modes --output-on-failure
  • cmake --build build --target test_forest_registry test_issue241_bootstrap test_issue151_pstringview test_issue146_sh_small_embedded_static test_issue146_index_sizes
  • ctest --test-dir build -R "test_forest_registry|test_issue241_bootstrap|test_issue151_pstringview|test_issue146_sh_small_embedded_static|test_issue146_index_sizes" --output-on-failure
  • ./scripts/generate-single-headers.sh --strip-comments
  • find . \( -name '*.cpp' -o -name '*.h' \) ! -path './third_party/*' ! -path './include/pmm_*.h' ! -path './single_include/*' ! -path './build/*' -print0 | xargs -0 clang-format --dry-run --Werror
  • cmake --build build
  • ctest --test-dir build --output-on-failure — 87/87 passed
  • ./scripts/check-file-size.sh
  • GITHUB_BASE_REF=main ./scripts/check-changelog-fragment.sh
  • ./scripts/check-docs-consistency.sh
  • ./scripts/check-repo-guard-rollout.sh
  • ./scripts/generate-single-headers.sh --strip-comments --output-dir /tmp/pmm-generated plus diff checks against committed single headers
  • cmake -S . -B build-asan -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-fsanitize=address,undefined -fno-sanitize-recover=all -fno-omit-frame-pointer"
  • cmake --build build-asan --target test_issue312_access_modes test_issue146_index_sizes test_issue151_pstringview
  • ctest --test-dir build-asan -R "test_issue312_access_modes|test_issue146_index_sizes|test_issue151_pstringview" --output-on-failure

konard added 2 commits April 19, 2026 17:07
Adding .gitkeep for PR creation (default mode).
This file will be removed when the task is complete.

Issue: netkeep80#312
@konard konard changed the title [WIP] # Extraction prep: разделить resolve() на явные checked/unchecked/verified access modes Clarify pptr checked and unchecked access modes Apr 19, 2026
@konard konard marked this pull request as ready for review April 19, 2026 17:41
@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

Solution summary

Implemented issue 312 and updated PR 320: #320

What changed:

  • Added explicit resolve_checked(pptr<T>) and resolve_unchecked(pptr<T>) access modes.
  • Kept resolve(pptr<T>) as a compatibility alias for checked public access.
  • Updated pptr::resolve, operator*, and operator-> to use checked access.
  • Made stale/freed pointers return nullptr through checked access while preserving raw internal access through unchecked mode.
  • Added regression coverage for valid, invalid, and stale pptr cases.
  • Regenerated the single include headers required by CI.
  • Fixed the SmallAddressTraits alignment issue found by sanitizer CI.

Verification:

The branch is issue-312-65d10bb90ae2, PR is marked ready for review, and the working tree is clean.

Useful next steps:

  1. Review and merge PR 320.
  2. Ask me to add a brief maintainer-facing note to the PR discussion summarizing the checked/unchecked API contract.

This summary was automatically extracted from the AI working session output.

@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

🤖 Solution Draft Log

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Model: GPT-5.4
  • Provider: OpenAI
  • Public pricing estimate: $50.241380

📊 Context and tokens usage:

  • 91.0M / 1.1M (8670%) input tokens, 51.4K / 128K (40%) output tokens

Total: (792.3K + 90.2M cached) input tokens, 51.4K output tokens, $50.241380 cost

🤖 Models used:

  • Tool: OpenAI Codex
  • Requested: gpt-5.4
  • Model: GPT-5.4 (gpt-5.4)

📎 Log file uploaded as Repository (2 chunks) (108271KB)


Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

🔄 Auto-restart triggered (iteration 1)

Reason: CI failures detected

Starting new session to address the issues.


Auto-restart-until-mergeable mode is active. Will continue until PR becomes mergeable.

@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

🔄 Auto-restart-until-mergeable Log (iteration 1)

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Model: GPT-5.4
  • Provider: OpenAI
  • Public pricing estimate: $1.776816
  • Token usage: 132,213 input, 4,001 output, 2,051,456 cache read

🤖 Models used:

  • Tool: OpenAI Codex
  • Requested: gpt-5.4
  • Model: GPT-5.4 (gpt-5.4)

📎 Log file uploaded as Repository (2 chunks) (115164KB)


Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

✅ Ready to merge

This pull request is now ready to be merged:

  • All CI checks have passed
  • No merge conflicts
  • No pending changes

Monitored by hive-mind with --auto-restart-until-mergeable flag

Copy link
Copy Markdown
Owner

@netkeep80 netkeep80 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Идея split на resolve_checked() / resolve_unchecked() по issue #312 правильная, и сам semantic seam нужен. Но в текущем виде здесь есть блокирующая ошибка именно в той части, которую PR заявляет как fix для SmallAddressTraits.

Проблема такая:

  • make_pptr_from_raw() теперь для non-aligned layout делает ceil(byte_off / granule); это выглядит как попытка корректно представить user pointer, который не лежит на границе гранулы;
  • но resolve_unchecked() по-прежнему возвращает base + p.offset() * granule_size, то есть адрес гранульной границы, а не реальный user pointer;
  • resolve_checked() использует raw_user_ptr_from_pptr(p) только для валидации блока, но в конце всё равно возвращает raw, полученный из resolve_unchecked().

То есть для non-aligned случая (sizeof(Block) % granule_size != 0) checked/unchecked resolve-path остаётся смещённым и возвращает не тот адрес, который сам же raw_user_ptr_from_pptr() считает каноническим user pointer. Это ломает заявленный контракт fix-а для SmallAddressTraits и делает публичный resolve_checked() / resolve_unchecked() внутренне несогласованными. fileciteturn56file0

Проще говоря: сейчас PR валидирует один адрес, а возвращает другой. Для DefaultAddressTraits тесты это не ловят, но для small/non-aligned path это именно семантическая ошибка. fileciteturn56file0

Что нужно поправить:

  1. Сделать так, чтобы canonical source of truth для pptr -> user pointer был один.
    В non-aligned случае resolve_unchecked() и resolve_checked() должны возвращать тот же адрес, который вычисляет raw_user_ptr_from_pptr().
  2. После этого добавить прямой regression test именно на SmallAddressTraits / non-aligned block-header path, а не только на default config. Сейчас issue #312 требует тесты на различие access modes, а PR дополнительно заявляет SmallAddressTraits fix, но отдельной focused regression на этот путь я в diff не вижу. fileciteturn57file0turn56file0

Пока это не исправлено, я бы PR не мёржил.

@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

🤖 AI Work Session Started

Starting automated work session at 2026-04-19T18:44:31.213Z

The PR has been converted to draft mode while work is in progress.

This comment marks the beginning of an AI work session. Please wait for the session to finish, and provide your feedback.

@konard konard marked this pull request as draft April 19, 2026 18:44
@konard konard marked this pull request as ready for review April 19, 2026 18:51
@konard konard force-pushed the issue-312-65d10bb90ae2 branch from 8ccbaf8 to 0ba03a3 Compare April 19, 2026 18:55
@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

🤖 Solution Draft Log

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Model: GPT-5.4
  • Provider: OpenAI
  • Public pricing estimate: $3.850374

📊 Context and tokens usage:

  • 6.3M / 1.1M (602%) input tokens, 10.3K / 128K (8%) output tokens

Total: (102.5K + 6.2M cached) input tokens, 10.3K output tokens, $3.850374 cost

🤖 Models used:

  • Tool: OpenAI Codex
  • Requested: gpt-5.4
  • Model: GPT-5.4 (gpt-5.4)

📎 Log file uploaded as Gist (17299KB)


Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

🔄 Auto-restart triggered (iteration 1)

Reason: CI failures detected

Starting new session to address the issues.


Auto-restart-until-mergeable mode is active. Will continue until PR becomes mergeable.

@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

🔄 Auto-restart-until-mergeable Log (iteration 1)

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Model: GPT-5.4
  • Provider: OpenAI
  • Public pricing estimate: $11.764353
  • Token usage: 242,776 input, 18,549 output, 20,266,240 cache read

🤖 Models used:

  • Tool: OpenAI Codex
  • Requested: gpt-5.4
  • Model: GPT-5.4 (gpt-5.4)

📎 Log file uploaded as Repository (49242KB)


Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

✅ Ready to merge

This pull request is now ready to be merged:

  • All CI checks have passed
  • No merge conflicts
  • No pending changes

Monitored by hive-mind with --auto-restart-until-mergeable flag

Copy link
Copy Markdown
Owner

@netkeep80 netkeep80 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Перепроверил обновлённую версию. Предыдущий блокер про несогласованный checked/unchecked path для SmallAddressTraits вы действительно закрыли: resolve_unchecked() теперь идёт через тот же canonical path, и focused regression на small/non-aligned case появился.

Но у меня здесь остаётся новый блокер: публичная canonical pptr-семантика всё ещё не проведена через все фабрики pptr.

Сейчас allocate_typed() / allocate_typed(count) уже возвращают pptr через block_idx + kBlockHdrGranules, то есть через новую canonical public pointer model. Но create_typed() и create_typed_unlocked() по-прежнему возвращают make_pptr_from_raw(raw), а intern_symbol_unlocked() тоже продолжает строить pptr<pstringview> через make_pptr_from_raw(raw).

Это критично, потому что в текущей версии make_pptr_from_raw() использует byte_off / granule_size, тогда как public resolve-path теперь читает pptr -> user pointer через raw_user_ptr_from_pptr() как base + offset * granule_size. Для non-aligned header path эти две модели уже не эквивалентны.

Иными словами:

  • allocate_typed() уже живёт в новой canonical модели;
  • create_typed() и symbol interning всё ещё возвращают pptr из старой raw-payload модели;
  • в non-aligned конфигурации такой pptr при resolve_checked()/resolve_unchecked() может указывать не на canonical public user pointer, а смещаться внутрь header/padding region.

Это делает PR внутренне непоследовательным: часть API уже переведена на новый contract, а часть фабрик pptr — ещё нет. Для issue #312 это блокирующий seam bug. fileciteturn61file0turn63file0turn68file0

Что нужно поправить:

  1. Либо make_pptr_from_raw() должен стать действительно canonical constructor для новой public pptr model;
  2. либо create_typed(), create_typed_unlocked() и intern_symbol_unlocked() должны перестать использовать его в текущем виде и строить pptr тем же способом, что и allocate_typed();
  3. после этого нужен regression test не только на allocate_typed, но и минимум на один путь, который идёт через create_typed() в small/non-aligned конфигурации.

Пока это не исправлено, я бы PR не мёржил.

@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

🤖 AI Work Session Started

Starting automated work session at 2026-04-19T19:34:43.370Z

The PR has been converted to draft mode while work is in progress.

This comment marks the beginning of an AI work session. Please wait for the session to finish, and provide your feedback.

@konard konard marked this pull request as draft April 19, 2026 19:34
@konard konard marked this pull request as ready for review April 19, 2026 19:53
@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

🤖 Solution Draft Log

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Model: GPT-5.4
  • Provider: OpenAI
  • Public pricing estimate: $15.953496

📊 Context and tokens usage:

  • 27.9M / 1.1M (2658%) input tokens, 34.1K / 128K (27%) output tokens

Total: (274.3K + 27.6M cached) input tokens, 34.1K output tokens, $15.953496 cost

🤖 Models used:

  • Tool: OpenAI Codex
  • Requested: gpt-5.4
  • Model: GPT-5.4 (gpt-5.4)

📎 Log file uploaded as Repository (39341KB)


Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard
Copy link
Copy Markdown
Contributor Author

konard commented Apr 19, 2026

✅ Ready to merge

This pull request is now ready to be merged:

  • All CI checks have passed
  • No merge conflicts
  • No pending changes

Monitored by hive-mind with --auto-restart-until-mergeable flag

@netkeep80 netkeep80 merged commit bcab8f4 into netkeep80:main Apr 19, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

# Extraction prep: разделить resolve() на явные checked/unchecked/verified access modes

2 participants