From 315353e065210c0af934b49f6094df7d2a415359 Mon Sep 17 00:00:00 2001 From: Brian Pugh Date: Sat, 8 Aug 2026 10:23:30 -0400 Subject: [PATCH 1/3] Make find_best_match selection an explicit build-system opt-in The core C sources auto-detected 64-bit hosts and silently included compressor_find_match_desktop.c. That put an architecture policy decision inside the vendored trio, where consumers that are not setup.py (espidf, wasm, the MicroPython native module, users who vendor the three .c files) had no say in it and no way to see what they were getting. Invert it: the core now always defaults to the portable scan, and each build system opts into the configuration measured for its platform, the way TAMP_ESP32 already worked. setup.py sets TAMP_USE_DESKTOP_MATCH=1 on 64-bit interpreters, mirroring the previous wheel behavior. - common.h gains a "Platform performance tuning" section documenting the contract, with TAMP_USE_EMBEDDED_MATCH / TAMP_USE_DESKTOP_MATCH defaulting to 0. - Selections are mutually exclusive, enforced with an #error that counts TAMP_ESP32 too: the espidf component supplies find_best_match via extern and the dispatch checks it first, so pairing it with an explicit TAMP_USE_*_MATCH would otherwise silently drop the requested finder. - The setup.py gate tests sys.maxsize as well as platform.machine(), which reports the OS architecture: under a 32-bit interpreter on a 64-bit host (win32 cibuildwheel legs, linux32-personality containers) machine() still says AMD64, and the desktop matcher needs MSVC's 64-bit-only _BitScanForward64. The desktop finder #errors on 32-bit MSVC targets as a backstop. - c-test now compiles the same finder the wheels use, so the desktop path stays covered once it is no longer auto-selected; c-test-embedded keeps covering the portable path. - New c-compile-matrix target (a c-test prerequisite) compiles the vendored trio under each documented flag combination, and asserts the two conflicting match selections fail to compile. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 20 ++++++--- Makefile | 43 +++++++++++++++++-- setup.py | 11 +++++ tamp/_c_src/tamp/common.h | 36 ++++++++++++++++ tamp/_c_src/tamp/compressor.c | 21 +++++---- .../tamp/compressor_find_match_desktop.c | 8 ++++ 6 files changed, 121 insertions(+), 18 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 94dbce8e..47c5c886 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,9 +31,12 @@ different platforms: - `compressor.h/c` - Compression implementation (sink/poll low-level API and higher-level compress/flush API) - `decompressor.h/c` - Decompression implementation -- `compressor_find_match_desktop.c` - Desktop-optimized match finding (included - by `compressor.c` on 64-bit targets: x86_64, aarch64, unless - `TAMP_USE_EMBEDDED_MATCH=1`) +- `common.c`/`compressor.c`/`decompressor.c` must compile standalone with only + the headers (users vendor these three files), so every implementation + reachable on embedded targets is defined inline. Only variants unreachable + there may live in `#include`'d files (`compressor_find_match_desktop.c`, used + when `TAMP_USE_DESKTOP_MATCH=1`) or come from a platform component ESP32-style + (extern `find_best_match`, `private/tamp_copy.h`). ## Development Commands @@ -226,8 +229,15 @@ make website-clean # Clean website build artifacts (default: 32 bytes, 256+ recommended for performance) - `TAMP_STREAM_MEMORY` / `TAMP_STREAM_STDIO` / `TAMP_STREAM_LITTLEFS` / `TAMP_STREAM_FATFS` - Enable built-in I/O handlers for specific backends -- `TAMP_USE_EMBEDDED_MATCH=1` - Force embedded `find_best_match` implementation - on desktop (for testing) +- Platform tuning flags (see `common.h`'s "Platform performance tuning" + section): the core sources never select architecture-specific code on their + own - every flag defaults to the portable implementation, and each build + system opts into its platform's measured configuration (`setup.py` sets + `TAMP_USE_DESKTOP_MATCH=1` on 64-bit hosts, espidf Kconfig defaults + `TAMP_ESP32=y`): + - `TAMP_USE_EMBEDDED_MATCH=1` - the portable `find_best_match` (selections are + mutually exclusive, including `TAMP_ESP32`; conflicts are a compile error) + - `TAMP_USE_DESKTOP_MATCH` - 64-bit SWAR for 64-bit hosts - `TAMP_USE_MEMSET=1` - Use libc `memset` (default: 1). Set to `0` for environments without libc (e.g. MicroPython native modules). diff --git a/Makefile b/Makefile index 72765b23..2884a273 100644 --- a/Makefile +++ b/Makefile @@ -479,6 +479,11 @@ CTEST_DEFINES = -DTAMP_STREAM_STDIO=1 -DTAMP_STREAM_MEMORY=1 \ -DTAMP_STREAM_FATFS=1 -DTEST_FATFS=1 \ -DTAMP_LAZY_MATCHING=1 \ -DLFS_NO_DEBUG -DLFS_NO_WARN -DLFS_NO_ERROR +# c-test covers the same match finder the pip/Cython build opts into on +# 64-bit hosts (the core defaults to the portable one); c-test-embedded +# covers the portable path. Selections are mutually exclusive (compile +# error), so this is applied only to the non-embedded tamp objects. +CTEST_MATCH_DEFINE = -DTAMP_USE_DESKTOP_MATCH=1 CTEST_CFLAGS = $(CTEST_INCLUDES) $(CTEST_SANITIZER_FLAGS) $(CTEST_DEFINES) # Strict warnings applied only to first-party tamp sources, not third-party (Unity/LittleFS/FatFs) CTEST_WARN_FLAGS = -Wall -Wextra -Wtype-limits -Werror @@ -510,7 +515,7 @@ CTEST_TEST_OBJS = \ # Build tamp source files for testing build/ctests/%.o: tamp/_c_src/tamp/%.c @mkdir -p build/ctests - $(CTEST_CC) $(CTEST_CFLAGS) $(CTEST_WARN_FLAGS) -c $< -o $@ + $(CTEST_CC) $(CTEST_CFLAGS) $(CTEST_MATCH_DEFINE) $(CTEST_WARN_FLAGS) -c $< -o $@ # Build Unity framework build/unity/unity.o: ctests/Unity/src/unity.c ctests/Unity/src/unity.h @@ -546,15 +551,44 @@ build/ctests/fatfs_ramdisk.o: ctests/fatfs_ramdisk.c # Build test runner (includes test files via #include) build/ctests/test_runner.o: ctests/test_runner.c ctests/test_compressor.c ctests/test_decompressor.c ctests/test_stream.c ctests/test_stream_filesystems.c @mkdir -p build/ctests - $(CTEST_CC) $(CTEST_CFLAGS) -c $< -o $@ + $(CTEST_CC) $(CTEST_CFLAGS) $(CTEST_MATCH_DEFINE) -c $< -o $@ # Link test executable build/test_runner: $(CTEST_TAMP_OBJS) $(CTEST_LFS_OBJS) $(CTEST_FATFS_OBJS) $(CTEST_TEST_OBJS) $(CTEST_CC) $(CTEST_LDFLAGS) -o $@ $^ -c-test: build/test_runner +c-test: build/test_runner c-compile-matrix ./build/test_runner +# Compile the vendored trio under every documented flag combination. +# Regression guard: flag-gated code paths must always compile. +C_COMPILE_MATRIX_CONFIGS = \ + "" \ + "-DTAMP_EXTENDED=0" \ + "-DTAMP_USE_MEMSET=0" \ + "-DTAMP_STREAM=0" + +.PHONY: c-compile-matrix +c-compile-matrix: + @mkdir -p build + @set -e; for cfg in $(C_COMPILE_MATRIX_CONFIGS); do \ + echo "c-compile-matrix: $$cfg"; \ + for src in tamp/_c_src/tamp/common.c tamp/_c_src/tamp/compressor.c tamp/_c_src/tamp/decompressor.c; do \ + $(CC) -O2 -Wall -Itamp/_c_src $$cfg -c $$src -o build/c_compile_matrix.o.tmp; \ + done; \ + done; rm -f build/c_compile_matrix.o.tmp + @set -e; for cfg in \ + "-DTAMP_ESP32=1 -DTAMP_USE_DESKTOP_MATCH=1" \ + "-DTAMP_USE_EMBEDDED_MATCH=1 -DTAMP_USE_DESKTOP_MATCH=1"; do \ + echo "c-compile-matrix (must NOT compile): $$cfg"; \ + if $(CC) -O2 -Wall -Itamp/_c_src $$cfg -c tamp/_c_src/tamp/common.c \ + -o build/c_compile_matrix.o.tmp 2>/dev/null; then \ + echo "c-compile-matrix: ERROR: conflicting match selection compiled: $$cfg"; \ + rm -f build/c_compile_matrix.o.tmp; exit 1; \ + fi; \ + done; rm -f build/c_compile_matrix.o.tmp + @echo "c-compile-matrix: all configurations compile" + clean-c-test: @rm -f build/test_runner @rm -f build/test_runner_embedded @@ -578,7 +612,7 @@ build/ctests-embedded/%.o: tamp/_c_src/tamp/%.c @mkdir -p build/ctests-embedded $(CTEST_CC) $(CTEST_CFLAGS) $(CTEST_WARN_FLAGS) -DTAMP_USE_EMBEDDED_MATCH=1 -c $< -o $@ -build/ctests-embedded/test_runner.o: ctests/test_runner.c ctests/test_compressor.c ctests/test_decompressor.c +build/ctests-embedded/test_runner.o: ctests/test_runner.c ctests/test_compressor.c ctests/test_decompressor.c ctests/test_stream.c ctests/test_stream_filesystems.c @mkdir -p build/ctests-embedded $(CTEST_CC) $(CTEST_CFLAGS) -DTAMP_USE_EMBEDDED_MATCH=1 -c $< -o $@ @@ -589,6 +623,7 @@ c-test-embedded: build/test_runner_embedded ./build/test_runner_embedded + ############ # Fuzzing ############ diff --git a/setup.py b/setup.py index 8d8c235f..8cf85d85 100644 --- a/setup.py +++ b/setup.py @@ -1,6 +1,7 @@ """Builds the Cython extensions; all other packaging configuration is in pyproject.toml.""" import os +import platform import sys from setuptools import setup @@ -52,6 +53,16 @@ def build_extensions(): if os.environ.get("TAMP_USE_EMBEDDED_MATCH", "0") == "1": print("Using embedded find_best_match implementation") define_macros.append(("TAMP_USE_EMBEDDED_MATCH", "1")) + elif sys.maxsize > 2**32 and platform.machine().lower() in ("x86_64", "amd64", "arm64", "aarch64"): + # The core C sources default to portable code everywhere; each build + # system opts into its platform's measured configuration (see the + # platform tuning section in tamp/_c_src/tamp/common.h). + # platform.machine() reports the OS architecture, so the sys.maxsize + # check is what excludes 32-bit interpreters on 64-bit hosts (win32 + # cibuildwheel legs, linux32-personality containers) - the desktop + # matcher needs a 64-bit target (e.g. MSVC's _BitScanForward64 does + # not exist on 32-bit targets). + define_macros.append(("TAMP_USE_DESKTOP_MATCH", "1")) if profile: print("Setting profiling configuration.") diff --git a/tamp/_c_src/tamp/common.h b/tamp/_c_src/tamp/common.h index 984bd211..3e67a278 100644 --- a/tamp/_c_src/tamp/common.h +++ b/tamp/_c_src/tamp/common.h @@ -69,6 +69,42 @@ extern "C" { #define TAMP_OPTIMIZE_SIZE #endif +/******************************************************************************* + * Platform performance tuning + * + * The core sources never select architecture-specific code on their own: + * every flag below defaults to the portable implementation. Build systems opt + * in to the measured configuration for their platform, mirroring TAMP_ESP32: + * + * pip/Cython (setup.py): TAMP_USE_DESKTOP_MATCH=1 on 64-bit hosts + * espidf component (Kconfig): TAMP_ESP32 (default y) + * + * Individual flags can still be set/overridden with -D=0/1. Measured + * numbers below are from devices/BENCHMARKS.md workloads; when enabling a + * flag on an unmeasured core, benchmark it. + ******************************************************************************/ + +/* find_best_match implementation (see compressor.c). At most one of these + * may be 1 (enforced below); with none set, the portable scan is used. + * embedded: portable single-byte-first scan, the default. + * desktop: 64-bit SWAR for 64-bit hosts (little-endian, cheap unaligned + * loads). */ +#ifndef TAMP_USE_EMBEDDED_MATCH +#define TAMP_USE_EMBEDDED_MATCH 0 +#endif +#ifndef TAMP_USE_DESKTOP_MATCH +#define TAMP_USE_DESKTOP_MATCH 0 +#endif + +/* The selections are mutually exclusive; reject conflicting configurations + * loudly rather than silently picking one. TAMP_ESP32 counts as a selection: + * the espidf platform component provides find_best_match via extern, and + * compressor.c's dispatch checks it first, so combining it with an explicit + * TAMP_USE_*_MATCH would otherwise silently drop the requested finder. */ +#if (TAMP_USE_EMBEDDED_MATCH + TAMP_USE_DESKTOP_MATCH + TAMP_ESP32) > 1 +#error "At most one find_best_match selection (TAMP_USE_*_MATCH / TAMP_ESP32) may be enabled" +#endif + /* TAMP_USE_MEMSET: Use libc memset (default: 1). * Set to 0 for environments without libc (e.g. MicroPython native modules). * When disabled, uses a volatile loop that prevents GCC from emitting a diff --git a/tamp/_c_src/tamp/compressor.c b/tamp/_c_src/tamp/compressor.c index 92a43ca7..ac2d8aab 100644 --- a/tamp/_c_src/tamp/compressor.c +++ b/tamp/_c_src/tamp/compressor.c @@ -79,25 +79,28 @@ inline bool tamp_compressor_full(const TampCompressor* compressor) { } /* - * Platform-specific find_best_match implementations: + * find_best_match implementations, selected by the flags from common.h's + * platform tuning section (the default is the portable embedded scan; build + * systems opt into their platform's measured configuration): * * 1. TAMP_ESP32: External implementation in espidf/tamp/compressor_esp32.cpp * - * 2. Desktop 64-bit (x86_64, aarch64, Windows 64-bit): - * Included from compressor_find_match_desktop.c - uses bit manipulation - * and 64-bit loads for parallel match detection + * 2. TAMP_USE_DESKTOP_MATCH (64-bit hosts): included from + * compressor_find_match_desktop.c - uses bit manipulation and 64-bit + * loads for parallel match detection. * - * 3. Embedded/Default (Cortex-M0/M0+, other 32-bit): - * Defined below - single-byte-first comparison, safe for all architectures + * 3. Default: defined below - portable single-byte-first comparison, safe + * for all architectures. * - * Set TAMP_USE_EMBEDDED_MATCH=1 to force the embedded implementation on desktop - * (useful for testing the embedded code path on CI). + * Implementations reachable on embedded targets are defined inline so that + * common.c/compressor.c/decompressor.c compile standalone with only the + * headers; only desktop/experimental variants live in #include'd files. */ #if TAMP_ESP32 extern void find_best_match(TampCompressor* compressor, uint16_t* match_index, uint8_t* match_size); -#elif (defined(__x86_64__) || defined(__aarch64__) || defined(_M_X64) || defined(_M_ARM64)) && !TAMP_USE_EMBEDDED_MATCH +#elif TAMP_USE_DESKTOP_MATCH #include "compressor_find_match_desktop.c" #else diff --git a/tamp/_c_src/tamp/compressor_find_match_desktop.c b/tamp/_c_src/tamp/compressor_find_match_desktop.c index 21ee8736..f7dff8b8 100644 --- a/tamp/_c_src/tamp/compressor_find_match_desktop.c +++ b/tamp/_c_src/tamp/compressor_find_match_desktop.c @@ -13,6 +13,14 @@ * - 64-bit compiler intrinsics (__builtin_ctzll or _BitScanForward64) */ +/* Fail loudly on targets that can't provide the 64-bit intrinsics (e.g. MSVC + * x86: _BitScanForward64 exists only on x64/ARM64 targets, and would otherwise + * surface as an unresolved-symbol link error). Selection is a build-system + * decision, so a wrong opt-in must be a clear compile error. */ +#if defined(_MSC_VER) && !defined(_M_X64) && !defined(_M_ARM64) +#error "TAMP_USE_DESKTOP_MATCH requires a 64-bit MSVC target (x64/ARM64); use the portable matcher on 32-bit targets" +#endif + #include // for memcpy (portable unaligned loads) // MSVC compatibility for count trailing zeros From 6c027a55014c64c0cdd863f51e74eff6a3d2f5c8 Mon Sep 17 00:00:00 2001 From: Brian Pugh Date: Sat, 8 Aug 2026 20:15:59 -0400 Subject: [PATCH 2/3] Normalize match-selection flags in the exclusivity check Count each selection as `(FLAG != 0)` instead of summing the raw macro values. This keeps the check correct for a flag defined to something other than 0/1 (e.g. `-DTAMP_USE_DESKTOP_MATCH=2` no longer trips a false conflict), and parenthesizing each term makes the sum immune to `+` binding tighter than `&&` should a future selection flag be defined as an unparenthesized expression. Co-Authored-By: Claude Opus 5 --- tamp/_c_src/tamp/common.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tamp/_c_src/tamp/common.h b/tamp/_c_src/tamp/common.h index 3e67a278..3fb433f0 100644 --- a/tamp/_c_src/tamp/common.h +++ b/tamp/_c_src/tamp/common.h @@ -101,7 +101,7 @@ extern "C" { * the espidf platform component provides find_best_match via extern, and * compressor.c's dispatch checks it first, so combining it with an explicit * TAMP_USE_*_MATCH would otherwise silently drop the requested finder. */ -#if (TAMP_USE_EMBEDDED_MATCH + TAMP_USE_DESKTOP_MATCH + TAMP_ESP32) > 1 +#if ((TAMP_USE_EMBEDDED_MATCH != 0) + (TAMP_USE_DESKTOP_MATCH != 0) + (TAMP_ESP32 != 0)) > 1 #error "At most one find_best_match selection (TAMP_USE_*_MATCH / TAMP_ESP32) may be enabled" #endif From 66ba39301e3f52cb099f1090de088fc3c78d98fa Mon Sep 17 00:00:00 2001 From: Brian Pugh Date: Sat, 8 Aug 2026 20:17:28 -0400 Subject: [PATCH 3/3] Clarify what TAMP_USE_EMBEDDED_MATCH actually does compressor.c's dispatch is `#if TAMP_ESP32 / #elif TAMP_USE_DESKTOP_MATCH / #else portable` and never reads TAMP_USE_EMBEDDED_MATCH, so listing it alongside "desktop" implied a selection it does not make. Co-Authored-By: Claude Opus 5 --- tamp/_c_src/tamp/common.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tamp/_c_src/tamp/common.h b/tamp/_c_src/tamp/common.h index 3fb433f0..abfe0e1e 100644 --- a/tamp/_c_src/tamp/common.h +++ b/tamp/_c_src/tamp/common.h @@ -85,8 +85,10 @@ extern "C" { ******************************************************************************/ /* find_best_match implementation (see compressor.c). At most one of these - * may be 1 (enforced below); with none set, the portable scan is used. - * embedded: portable single-byte-first scan, the default. + * may be 1 (enforced below); with none set, the portable single-byte-first + * scan is used. + * embedded: the portable scan; compressor.c does not read this flag, so + * setting it only guards against a conflicting selection. * desktop: 64-bit SWAR for 64-bit hosts (little-endian, cheap unaligned * loads). */ #ifndef TAMP_USE_EMBEDDED_MATCH