diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 04abc268a..54d930fde 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,141 +1,70 @@ -# Claude AI Guidelines for snmalloc +# AI Guidelines for snmalloc ## Working Style -**Complete the plan, then check in**: When a plan is approved, execute all -steps to completion. Don't stop after each step for review. When you think -you're done, recursively apply all relevant principles from this file — check -each one, act on any that apply, then check again until no more principles -are relevant. Only then report completion and wait for feedback. - -**Plans require discussion before implementation**: After devising a plan -(whether in plan mode or not), run the review loop (see "Mandatory review -checkpoints") before presenting it. Do NOT proceed to implementation until -the plan has been seen and explicitly approved. - -**Store plans in PLAN.md**: Always write plans to `PLAN.md` in the repository -root so that context survives session boundaries. Update (not append to) the -file when the plan evolves. This is the single source of truth for what is -planned and what has been completed. - -**Baseline the checkout before starting work**: Before beginning implementation -of any plan, verify that the current checkout builds and passes tests. Run the -build and test suite (per `.github/skills/building_and_testing.md`) and record the -results. If the baseline is broken, report the failures and stop — do not start -implementation on a broken base. Pre-existing failures that are not caused by -your changes must be acknowledged upfront so they are not confused with -regressions introduced by the plan. This establishes the ground truth against -which your changes will be measured. - -**Every plan step must have a test gate**: Each step in a plan must produce -a testable result — a test, a build check, or a verifiable property — that -acts as the gate to the next step. Do not move to step N+1 until step N's -gate passes. This catches integration issues incrementally rather than -deferring all testing to the end. When writing a plan, structure it so that -independently testable components are implemented and verified first, and -later steps build on proven foundations. - -**Mandatory review checkpoints**: At each of these points, run the full -review loop — spawn a fresh-context reviewer subagent, address findings, -spawn another fresh reviewer, repeat until a reviewer finds no issues. When -you disagree with a reviewer's finding, escalate — do not resolve disputes -unilaterally. Do not proceed past a checkpoint without a clean review. -1. **After devising a plan**, before presenting it for discussion. For plan - reviews, adapt the reviewer prompt: instead of reading changed files and - running tests, the reviewer should read the plan document, read existing - code the plan references, verify assumptions about the codebase, and check - for structural gaps (missing steps, naming conflicts, incorrect - dependencies). -2. **After completing implementation and self-review**, before opening a PR. - -The only exception: if you believe a change is truly trivial (a typo fix, a -one-line config change), ask for permission to skip the review. Do not decide -on your own that something is trivial enough to skip. When in doubt, run the -review. - -**Go slow to go fast**: Before starting implementation work, identify and state -which principles from these instructions are most relevant to the current task. -This surfaces the right guidelines before they're needed rather than -rediscovering them after a mistake. - -**Challenge me when the evidence says I'm wrong**: If a reviewer flags something -that contradicts what I said, or if you have concrete evidence that an -instruction is incorrect, raise it — don't silently comply. Present the evidence -and discuss it. - -**Research findings belong in the plan**: If research or exploration surfaces -issues beyond the original task (inaccurate comments, dead code, related bugs), -include them as explicit plan steps — don't just mention them in the analysis -and move on. Anything worth noting is worth acting on or explicitly deferring. - -**Self-review is part of done**: The recursive principle check described in -"Complete the plan, then check in" IS the self-review. It's not a separate -step — it's what "done" means. Never report completion without having done it. - -**During reviewer loops**: At any point during the review loop — when fixing -findings, when unsure about a reviewer's suggestion, when making tradeoff -decisions — stop and ask. The automated review removes me as a gatekeeper, not -as a collaborator. - -## Debugging Principles - -1. **Logging is essential** - When debugging issues in allocator code, add tracing to identify the exact point of failure. Use `write()` directly to stderr/file rather than `printf`/`message` to avoid recursion through the allocator. - -2. **New code is most likely at fault** - When tests fail after changes, assume the new code introduced the bug. Don't blame existing infrastructure that was working before. - -3. **Baseline against origin/main** - Before assuming a system-wide issue, verify the test passes on `origin/main`. This confirms whether the issue is a regression introduced by your changes. - -4. **Check the whole PR for patterns** - When fixing a bug of a specific shape (e.g., "one-armed `if constexpr` causes MSVC C4702"), immediately search all changed files in the PR for the same pattern. Fix all instances at once rather than waiting for CI to report each one individually. - -5. **Verify hypotheses before acting** - A hypothesis about a bug's cause is not knowledge — it's a guess. Before investing effort in workarounds or fixes, validate empirically that your suspected cause is actually the cause. Read the code more carefully, write a minimal reproducer, or examine the actual data. Verify first, then act. - -6. **CI is the source of truth for build status** - A local build failure does not mean the build is broken. Local toolchain versions, stale dependency caches, and environment differences can all cause local failures that don't reproduce in CI. Never declare a build "broken on main" based on local results — check CI first. +- Read the relevant code before proposing or making changes. Keep changes + focused, preserve existing structure where practical, and avoid unrelated + renames or refactoring. +- Use a written plan for genuinely multi-stage work or when the user requests + one. Store repository plans in `PLAN.md`, and obtain approval before + implementing a plan that introduces significant design choices. +- Match validation effort to the risk and scope of the change. Establish a + relevant baseline before broad or high-risk work; small changes may use + targeted validation. Use CI or `origin/main` when failure attribution is + unclear. +- Complete approved work through validation and a bounded self-review before + reporting it done. Use independent review when the breadth, risk, or + complexity of the change warrants it. +- Report incidental findings separately unless they are necessary to make the + requested change correct. Challenge instructions or assumptions when + evidence contradicts them. + +## Debugging + +- Allocator tracing must use `write()` directly to stderr or a file rather + than `printf` or `message`, which may recurse through the allocator. +- Verify hypotheses before implementing a workaround. Inspect the actual data, + write a minimal reproducer, or compare against a known-good revision. +- Start by checking new or changed code, but do not claim that `main` is broken + from a local failure alone. Check CI and, when useful, reproduce on + `origin/main`. +- When a bug pattern may occur more than once, search the complete change for + other instances and fix the relevant occurrences together. ## Code Quality -- **Use cross-platform macros from `defines.h`** - Never use raw compiler attributes like `__attribute__((used))` or `__forceinline` directly. Instead use the corresponding `SNMALLOC_*` macros (e.g., `SNMALLOC_USED_FUNCTION`, `SNMALLOC_FAST_PATH`, `SNMALLOC_SLOW_PATH`, `SNMALLOC_PURE`, `SNMALLOC_COLD`, `SNMALLOC_UNUSED_FUNCTION`, `ALWAYSINLINE`, `NOINLINE`). These are defined in `ds_core/defines.h` with correct expansions for MSVC, GCC, and Clang. - -- **Don't encode platform assumptions** - Avoid hardcoding limits like "48-bit address space" or "256 TiB max allocation". These assumptions may not hold on future platforms (56-bit, 64-bit address spaces, CHERI, etc.). - -- **Trust the existing bounds checks** - snmalloc already has appropriate bounds checking at API boundaries. New internal code should defer to the backend for edge cases rather than adding redundant checks. - -- **Guard new data structures** - When adding caches or intermediate layers, ensure they handle all input ranges correctly, including sizes larger than what they cache. Return early/bypass for out-of-range inputs. - -- **Keep headers minimal** - Each header should only include what it directly needs. Avoid adding transitive includes "for convenience" — if a header's own declarations only need ``, don't pull in heavier internal headers. Includers are responsible for their own dependencies. This keeps compile times low and dependency graphs clean. - -- **No C++ STL or C++ standard library headers** - snmalloc must be compilable as part of a libc implementation, so it cannot depend on an external C++ STL. Never use headers like ``, ``, ``, ``, etc. directly. Instead use the C equivalents (``, ``) or snmalloc's own STL wrappers in `src/snmalloc/stl/` (e.g., `snmalloc/stl/type_traits.h`, `snmalloc/stl/atomic.h`, `snmalloc/stl/array.h`). These wrappers have both a `gnu/` backend (no C++ STL dependency) and a `cxx/` backend, selected at build time. - -- **Prefer explicit over implicit** - Avoid relying on implicit conversions, convention-based wiring, or unnamed dependencies. A few extra characters of explicit code is almost always cheaper than someone later needing to reconstruct the hidden knowledge. This is especially relevant in C++ with its many implicit conversion paths and template magic. - -- **Document coupling at the point of breakage** - When code A depends on the internal behaviour of code B (read sequence, execution order, size assumptions), put the comment on B — that's where a future maintainer would make a breaking change. Commenting at A doesn't help because the person changing B won't be reading A. - -- **Design for changeability, not for predicted changes** - Make designs modular and replaceable so future needs can be accommodated, but don't add abstractions, extension points, or features for changes that haven't happened yet. The goal is a design that's easy to modify, not one that anticipates specific modifications. - -## Code Change Discipline - -- **Read before modifying** - Do not propose changes to code you haven't read. Understand existing code before suggesting modifications. - -- **Prefer editing over creating** - Edit existing files rather than creating new ones. This prevents file bloat and builds on existing work. - -- **Avoid over-engineering** - Only make changes that are directly requested or clearly necessary. Don't add error handling for scenarios that can't happen. Don't add docstrings or comments to code you didn't change. Don't create helpers or abstractions for one-time operations. Three similar lines of code is better than a premature abstraction. - -- **Evaluate copied patterns, don't cargo-cult** - When reusing a pattern from existing snmalloc code, evaluate each choice (`constexpr` vs runtime, template vs function parameter, etc.) in the context of the new usage. The original may have had reasons that don't apply, or it may have been a mistake. Copy the intent, not the incidental choices. Conventions (legal headers, naming schemes, file organisation) should be followed for consistency; technical patterns should be evaluated on merit. - -- **Fix what your change makes stale** - When a change invalidates something elsewhere — a comment, a test description, documentation — fix it in the same PR. Stale artefacts left behind are bugs in the making, and "I didn't modify that line" isn't an excuse when your change is what made it wrong. - -- **Document the code, not the change** - Comments and documentation describe how the code IS, not how it was changed or why it differs from a previous version. Don't leave comments explaining "we removed X" or "this was changed from Y" — a reader shouldn't need the git history to understand the code. If code needs context about alternatives or design decisions, put that in design docs, not inline comments. +- Prefer designs simple enough that their correctness is evident. Do not + mistake the absence of an obvious bug in a complicated design for evidence + that the design is correct. +- Use the cross-platform macros from `ds_core/defines.h` rather than raw + compiler attributes such as `__attribute__((used))` or `__forceinline`. +- Do not encode platform assumptions such as a fixed virtual-address width or + maximum allocation derived from one current platform. +- Trust existing API-boundary bounds checks. Internal code should defer edge + cases to the backend rather than adding redundant checks. +- New caches and intermediate data structures must safely bypass inputs outside + the range they handle. +- Keep headers minimal and include only direct dependencies. +- Do not depend directly on the C++ standard library. Use C headers such as + `` and ``, or the wrappers in `src/snmalloc/stl/`. +- Prefer explicit wiring and conversions over hidden dependencies or + convention-based behaviour. +- Document behavioural coupling at the component whose modification could + break the dependency. +- Design code to be easy to change, but do not add extension points or + abstractions for hypothetical requirements. + +## Change Discipline + +- Avoid over-engineering and review churn. Preserve names and function + boundaries unless changing them improves correctness or clarity. +- Evaluate copied implementation patterns in their new context rather than + reproducing incidental choices. +- Update comments, tests, and documentation made stale by the change. +- Comments and documentation describe current behaviour, not the history of + the change. ## Building, Testing, and Benchmarking -All build, test, and benchmarking guidance lives in `.github/skills/building_and_testing.md`. - -**Delegate testing to a subagent.** When it is time to build and run tests, -spawn a subagent whose prompt includes the contents of -`.github/skills/building_and_testing.md` and tells it which tests to run (or "run the -full suite"). Do NOT include implementation context — the subagent must not -know what code changed. This prevents the tester from rationalising failures -as related to the changes instead of reporting them objectively. - -The subagent will report back: which tests passed, which failed, exact -commands, and full output of any failures. If failures are reported, treat -them as actionable per the failure protocol in the skill file. +Follow `.github/skills/building_and_testing.md`. Select validation based on the +affected behaviour and report commands and failures factually. diff --git a/.github/skills/building_and_testing.md b/.github/skills/building_and_testing.md index 019449d7b..53413b46e 100644 --- a/.github/skills/building_and_testing.md +++ b/.github/skills/building_and_testing.md @@ -1,57 +1,85 @@ # Building and Testing Skill -This file is the complete reference for building and testing snmalloc. -It is designed to be used by a subagent that has NO context about what -code changes were made — only that it needs to build and verify the -project. This isolation is intentional: test results must be interpreted -without bias from knowing what changed. +This file contains the repository-specific guidance for building, testing, and +benchmarking snmalloc. ## Build -- Build directory: `build/` -- Build system: Ninja with CMake -- **Always test with a Debug build.** Debug enables assertions (`-check` variants) that catch invariant violations invisible in Release. A Release-only test run can report 100% pass while masking real bugs. Verify with `grep CMAKE_BUILD_TYPE build/CMakeCache.txt` — it must show `Debug`. -- Rebuild all targets before running ctest: `ninja -C build` (required — ctest runs pre-built binaries) -- Rebuild specific targets: `ninja -C build ` -- Always run `clang-format` before committing changes: `ninja -C build clangformat` +- The conventional build directory is `build/`. Commands below use a + single-config Ninja build. For a multi-config generator, build with + `cmake --build build --config Debug` and pass `-C Debug` to CTest; use + `Release` instead when benchmarking. +- Use a Debug build for functional validation so that allocator assertions and + debug-conditioned checks are enabled. The separate `-check` test flavour + defines `SNMALLOC_CHECK_CLIENT` in every build configuration. Verify the + configuration of a single-config build with + `grep CMAKE_BUILD_TYPE build/CMakeCache.txt`. +- Rebuild the relevant targets before running tests. Use `ninja -C build` for + the full build or `ninja -C build ` for a focused test. +- Format changed code before committing with `ninja -C build clangformat`. ## Testing -- Run `func-malloc-fast` and `func-jemalloc-fast` to catch allocation edge cases -- The `-check` variants include assertions but may pass when `-fast` hangs due to timing differences -- Use `timeout` when running tests to avoid infinite hangs -- Never run a test on a stale build artifact. Rebuild in the same build directory/config before any run or rerun: `ninja -C ` for `ctest` runs, or `ninja -C ` if you invoke a direct binary. If the rebuild fails, stop and report with the rebuild log. -- Testing skill: keep commands stable (right build dir/config, consistent flags), prefer `ctest -R --output-on-failure`, and avoid ad-hoc command variants that change coverage or filters. -- Before considering a change complete, run the full test suite: `ctest --output-on-failure -j 4 --timeout 60` +- Prefer focused tests while developing: + `ctest --test-dir build -R --output-on-failure`. +- Run `func-malloc-fast` and `func-jemalloc-fast` when allocator API or + compatibility behaviour may be affected. The `-check` variants can miss + timing-dependent hangs that appear in `-fast`. +- Use a timeout for tests that could hang. +- Run the full Debug suite when the breadth or risk of the change warrants it: + `ctest --test-dir build --output-on-failure -j 4 --timeout 400`. +- Never test a stale binary. Rebuild in the same directory and configuration + before a run or rerun. -### Test failures (never hand-wave) +### Test failures -- Never describe a failure as transient without evidence. Treat every failure as actionable until disproven. -- After a rebuild (per the testing rule above) succeeds, rerun the exact failing command twice: Rerun #1 must match the original command (including filters/flags such as `-R`, `-j`, `--timeout`); Rerun #2 may add only `--output-on-failure` if it was missing. No other changes to flags or filters between reruns. -- Required logging bundle for any failure or flake claim: rebuild command plus stdout/stderr; original failing command plus stdout/stderr; both rerun commands plus stdout/stderr; commit/branch; build directory and config (Release/Debug); compiler/toolchain; host OS; env vars/options affecting the run (allocator config, sanitizers, thread count); note if Rerun #2 added `--output-on-failure`. -- Workflow: record failing command/output → rebuild in the same build directory (stop/report if rebuild fails) → two reruns as above → capture all logs/context → check CI status and origin/main baseline → only label a flake with evidence. Report flakes or unresolved failures in a PR comment with logs and CI links. +- Treat a failure as actionable until there is evidence otherwise. Do not call + it transient solely because a rerun passes. +- Preserve the failing command and output, rebuild the same configuration, and + rerun with materially equivalent options. Record enough environment and + configuration information to reproduce unresolved or intermittent failures. +- If attribution is unclear, compare with CI and `origin/main`. Report the + evidence and any remaining uncertainty rather than guessing. ### Test library (`snmalloc_testlib`) -Tests that only use the public allocator API can link against a pre-compiled static library (`snmalloc-testlib-{fast,check}`) instead of compiling the full allocator in each TU. +Tests that use only the public allocator API can include the lightweight test +header and be compiled once for both allocator test flavours. -- **Header**: `test/snmalloc_testlib.h` — forward-declares the API surface; does NOT include any snmalloc headers. Tests that also need snmalloc internals (sizeclasses, pointer math, etc.) include `` or `` alongside it. -- **CMake**: Add the test name to `LIBRARY_FUNC_TESTS` or `LIBRARY_PERF_TESTS` in `CMakeLists.txt`. -- **Apply broadly**: When adding new API to testlib (e.g., `ScopedAllocHandle`), immediately audit all remaining non-library tests to see which ones can now be migrated. Don't wait for CI to find them one by one. -- **Cannot migrate**: Tests that use custom `Config` types, `Pool`, override machinery, internal data structures (freelists, MPSC queues), or the statically-sized `alloc()` template with many size values genuinely need `snmalloc.h`. +- `src/test/snmalloc_testlib.h`, included as ``, + declares the supported API without including the full `snmalloc.h` or + `snmalloc_core.h` allocator headers. +- Add tests whose only allocator-facing snmalloc header is this header to + `TESTLIB_ONLY_TESTS` in `CMakeLists.txt` so their source is compiled once and + linked against both `snmalloc-testlib-fast` and `snmalloc-testlib-check`. +- Tests using custom `Config` types, `Pool`, override machinery, internal + data structures, or many instantiations of `alloc()` require direct + allocator headers and must not be classified as testlib-only. +- When extending the test-library API, consider whether existing tests can now + use it, but avoid unrelated migration churn. ## Benchmarking -- Before benchmarking, verify Release build: `grep CMAKE_BUILD_TYPE build/CMakeCache.txt` should show `Release` -- Debug builds have assertions enabled and will give misleading performance numbers +- Benchmark only an optimized Release build. For a single-config build, verify + it with `grep CMAKE_BUILD_TYPE /CMakeCache.txt`; for a multi-config + build, select `Release` explicitly when building and running the benchmark. +- Rebuild the benchmark target before measuring, retain raw results, and report + the relevant allocator configuration and known methodological limitations. -## Subagent protocol +### Quick performance experiments -When you are invoked as a testing subagent: +For an explicitly exploratory or disposable experiment: -1. **Read this file first.** It is your only reference for how to build and test. -2. **You have no knowledge of what changed.** Do not ask. Do not speculate. Report only what you observe. -3. **Rebuild before testing.** Always run `ninja -C build` before any test invocation. If the rebuild fails, report the failure and stop. -4. **Run the requested tests** (or the full suite if not specified). Use the exact commands from this file. -5. **Report results factually**: which tests passed, which failed, the exact commands you ran, and the full output of any failures. Do not interpret failures in terms of code changes — you don't know what they are. -6. **Never label a failure as transient.** If a test fails, follow the failure protocol above (rebuild + two reruns + logging bundle). Report all evidence. +1. Record the revision, dirty state, compiler, host, benchmark options, and + relevant allocator configuration. +2. Use a dedicated Release build directory, or verify that the selected build + is Release. +3. Rebuild the specific target and run a smoke check that exercises the + instrumented path before collecting measurements. +4. Retain raw measurements and report timer resolution, run count, variability, + and known limitations. + +A disposable experiment does not require a full Debug baseline, full test +suite, formatting, or independent review. Experimental code is not merge-ready. +If it will be retained, committed, or submitted, promote it to a normal change +and perform the validation appropriate to its final scope.