Repository navigation
C++ modernization audit — Tier A: cleanups, races, correctness - #423
Merged
Merged
Conversation
All grep-verified to have zero live consumers: - AudioDevice.h: COREAUDIO/ALSA_AUDIO backend selector — neither macro is ever defined, so the build always fell through to <portaudio.h>; the class is written entirely against PortAudio. macOS still gets AudioHardware.h from the block above, so include sets are unchanged on every platform. - UNIV.h: TARGET_64BIT/TARGET_32BIT — defined by two #if ladders, read nowhere in src/, include/, runtime/ or libs/. - EXTLLVM.cpp: <sys/syscall.h> — no syscall()/SYS_* user remains. - SchemeS7.h: SCHEME_EXPORT/_SCHEME_SOURCE dllexport block — unreferenced; headers use EXPORT from UNIV.h instead. - SchemeS7.cpp: orphan string_hash (seed 5381) — external linkage but undeclared and uncalled; the JIT-registered "string_hash" resolves to the canonical seed-0 version in EXTLLVM.h. No behavioural change on any platform. Build + libs-core/cpp-unit/compiler-unit green.
- SchemeREPL.h: add #pragma once — it had no include guard at all, a latent ODR hazard if ever included from two translation units. - SchemeS7.h: drop duplicate r64value/r32value/charvalue prototypes (already declared ~50 lines above). - Task.h/CM.h: move <iostream> to CM.h, which actually uses std::cout/std::cerr; Task.h still includes CM.h so the transitive chain is preserved. - Extempore.cpp: add <iostream>/<cstring>/<cstdio> (used directly, previously relied on transitive includes). - SchemeS7.cpp: drop unused <iostream>/<vector>, add <string> (std::string used). - EXTLLVM.cpp: remove duplicate <mutex>/<cmath> includes. SchemeFFI.cpp's platform-conditional include cleanup is deferred to the cross-platform CI pass. Build + libs-core/cpp-unit/compiler-unit green.
audio_sanity and audio_sanity_f were the same finite-check + clamp-to- [-0.99, 0.99] (SAMPLE is float); the only difference was audio_sanity_f's _mm_min_ss/_mm_max_ss path behind an x86 #ifdef with a scalar ARM fallback. The finite check runs first, so NaN/inf never reach the clamp, and GCC/Clang/ MSVC lower std::clamp on float to minss/maxss at -O2 — so the intrinsics, the <xmmintrin.h> dependency and the x86/ARM #ifdef earn nothing. Unify to a single audio_sanity(float) using std::clamp and update the five call sites. The audio-offline render tests produce byte-identical output.
- EXTClosureAddressTable.cpp: drop the local string_hash; it is identical to the canonical one in EXTLLVM.h (same seed 0, multiplier 33 and uint8_t cast), which is already included — closure-table ids are unchanged. - ffi/llvm.inc: llvm:remove-function, llvm:remove-globalvar and llvm:erase-function had three identical bodies; route all three through one remove_symbol_and_global() helper, preserving the FFI names. - Extempore.cpp: extract escape_backslashes() from the two identical backslash-doubling loops in the option handlers that embed a path in a Scheme literal.
Each MT audio worker malloc'd UNIV::IN_CHANNELS * 8 bytes once at startup and never freed it; the literal 8 also over-allocated 2x (sizeof(SAMPLE) is 4). Replace with a std::vector<SAMPLE> sized UNIV::IN_CHANNELS, allocated once before the realtime loop so the loop stays allocation-free. The existing indata[k] / &(indata[0]) uses work unchanged.
All verified to have zero callers in src/, include/, runtime/, libs/ and no FFI registration or extempore.def/bitcode.ll reference: - SchemeS7: scheme_apply1, mk_continuation, mk_closure, procname, is_promise, setimmutable, closure_code. Several were no-op stubs (setimmutable did nothing; closure_code returned nullptr) left over from the TinyScheme->s7 port — live traps if anything had called them. closure_env is kept: unlike the others it is still reachable through the get-closure-env FFI wrapper (a separate Tier-D decision). - SchemeProcess::loadString: declared and defined, never called. The xtlang get-closure-code is a separate Scheme-level function, not bound to the removed C++ closure_code. Build + libs-core/cpp-unit/compiler-unit/ audio-offline green.
These were read and written from different threads without synchronisation, matching the same hazard already fixed for UNIV::TIME (commit ef0b2b6). All are simple published values (store on one thread, load on another), so plain std::atomic with default ordering suffices and stays lock-free on the audio callback path: - SchemeProcess::m_running (bool): set false by stop()/error paths, spun on by the task and server loops. As a plain bool the compiler could hoist the read out of 'while (m_running)', so stop() could fail to take effect. - TaskScheduler::m_numFrames (unsigned): written by setFrames() from the audio callback, read by timeSlice(). - UNIV::AUDIO_CLOCK_BASE/NOW and AudioDevice::CLOCKBASE/REALTIME (double): published by the audio callback, read by the scheduler and the clock FFI. std::atomic<double> is a single lock-free 64-bit access (static_assert added); the four atomic-to-atomic assignments use explicit .store(x.load()) since atomic's copy-assignment is deleted. No JIT/ABI impact: the clock globals are reached via the audio_clock_*() getters, not referenced directly from bitcode.ll. Build + libs-core/cpp-unit/ compiler-unit/audio-offline green.
- EXTClosureAddressTable get_address_table: the match test was inverted — 'if (strcmp(name, ...))' returns the first NON-matching node. Use == 0. (The function is registered but currently uncalled, so this is forward hardening.) - EXTClosureAddressTable check_address_exists: a do/while dereferenced table->id before the null check, so a null table would crash. It IS live (called from EXTLLVM and xtlang IR), so use a top-tested while; results are identical for non-null input and it now returns false on null. - EXTLLVM disassembler: 'instSize <= 0' on a size_t is always == 0 and warns under -Wtype-limits; make it == 0. - EXTZones: old_zone_size was int, truncating a uint64_t zone size and corrupting zones above 2GB; widen to uint64_t. Also drop a shadowing inner 'z' in llvm_peek_zone_stack (clears the last -Wshadow warning). - clock.inc lastSampleBlockClock: the EnvInjector GC guards were unnamed temporaries destroyed at the semicolon, protecting p1/p2 for zero time across the allocating mk_real/cons. Name them, matching every other call site. - SchemeProcess::resetOutportString: it copied s7's output into the port buffer and then immediately memset it to zero — the copy was dead. Its net effect is 'clear the buffer', which is its job; drop the dead strncpy. - sys.inc platform(): added an #else returning "unknown" so the function doesn't fall off the end (UB) on unrecognised platforms. Build is now warning-clean. libs-core/cpp-unit/compiler-unit/audio-offline green.
Collapse the three-way #ifdef _WIN32 / __linux__ / __APPLE__ getRealTime into a single std::chrono::system_clock implementation for Windows and Linux, which both want Unix-epoch wall-clock seconds. This also fixes a latent Windows bug: the old Windows branch used high_resolution_clock, whose epoch is unspecified (boot-relative on MSVC), yet the value is consumed as absolute wall-clock for cross-machine clock sync. std::chrono::system_clock is Unix-epoch wall time on every platform. Drops the Linux <time.h>/clock_gettime path and moves <chrono> to an unconditional include (the merged branch needs it on Linux too). <Windows.h> stays — the console-colour helper below uses SetConsoleTextAttribute. The macOS CoreFoundation branch is left untouched pending a check that AudioDevice's sample-clock base maths doesn't depend on its representation (Tier D). Verified on Linux: clock:clock returns the correct current wall-clock time. Build + libs-core/cpp-unit/compiler-unit/audio-offline green.
buildPreamble() iterates sTypeDefs/sFuncDecls/sGlobalDecls to assemble the IR prepended to every user module. They were std::unordered_map, whose iteration order is unspecified, so the generated IR text (and thus the AOT cache) varied between runs and standard-library versions. Switch the three to std::map; they only use size()/emplace()/count()/iteration, all map-compatible, and <map> replaces the now-unneeded <unordered_map>. Generated preamble is now byte-deterministic across runs. Build + libs-core/cpp-unit/compiler-unit (incl. aot-compilation)/audio-offline green.
EXTThread::kill() (xtlang thread_kill) called pthread_cancel, which is undefined behaviour against C++ RAII/locks — a cancelled thread holding a mutex deadlocks the process — and was a silent no-op on Windows. It has no callers in the shipped runtime. Replace it with a std::atomic<bool> stop flag: kill() requests a stop and stopRequested() lets a thread poll for it and unwind cleanly. This is uniform across platforms (the #ifndef _WIN32 split is gone) and safe. The contract is now cooperative — a forced kill of arbitrary JIT'd code was never safe anyway. Build + libs-core/cpp-unit/compiler-unit green (Linux); cross-platform via CI.
Completes the getRealTime collapse started in the Tier A commit. The macOS branch returned CFAbsoluteTimeGetCurrent() + kCFAbsoluteTimeIntervalSince1970, which is exactly the Unix-epoch wall time std::chrono::system_clock already gives, so the platform split (and the only UNIV.h CoreAudio/CoreFoundation includes) is now gone — getRealTime is one portable definition. Linux build + clock value + libs-core/compiler-unit green; macOS verified via CI.
SUBSUME_PRIMARY has been unconditionally #defined since the move to running the primary SchemeProcess on thread 0, so the whole 'run a clock scheduler on the free thread 0' feature was dead: the only XTMMainCallback invocation sat in the #ifndef SUBSUME_PRIMARY branch (compiled out), and run_main_event_loop — the only thing that registered the callback — has no callers. The public sched_main macro that fed it scheduled onto a scheduler that never ran. Remove it end-to-end and flatten the SUBSUME_PRIMARY conditionals: - Extempore.cpp: drop XTMMainCallback + xtm_set_main_callback and the dead #ifndef branches; the primary-on-thread-0 path is now the only path. - extempore.def / bitcode.ll: drop the xtm_set_main_callback export and IR declare. - scheduler.xtm: drop MAIN, MAINEVT, clock_scheduler_main_callback, run_main_event_loop and the sched_main macro (all unused). - llvmti-caches.xtm: drop the stale binding-cache entry. Verified on Linux: extempore rebuilds, the full stdlib AOT-compiles, and the 49-test non-graphics suite passes. Cross-platform via CI. Note: this removes the (dormant, unused) sched_main main-thread-scheduling surface, not just internal glue.
Grep-verified to have no callers anywhere in src/include/libs/runtime: - get_message_length (free function) - OSC::getOSCStringSection - OSC::clearMessageBuffer - the static OSC::singleton member (its definition and only use were already commented out) The socket-accessor pair getSendFD/setSendFD is left for the held socket-stack unification, since it touches the dual experimental::net/POSIX member layout. Build + libs-core/cpp-unit + osc_101 example green (Linux).
Zones promise LLVM_ZONE_ALIGN (32-byte) alignment, but on Windows llvm_zone_create used plain malloc (16-byte aligned) because _aligned_malloc 'crashed' -- the actual cause was llvm_zone_destroy freeing with plain free(), which is undefined behaviour for _aligned_malloc'd memory. Factor the platform split into one zone_aligned_alloc/zone_aligned_free pair (_aligned_malloc/_aligned_free on Windows, posix_memalign/free elsewhere) and use them in both create and destroy. Windows now gets the promised 32-byte alignment without the crash; the POSIX path is unchanged apart from now checking posix_memalign's return value. Linux build + AOT + libs-core/compiler-unit/audio-offline green; the Windows alignment fix is verified by CI.
Bump CMAKE_CXX_STANDARD 17 -> 20. Kept as the global setting (there is one first-party target) with GNU extensions on (-std=gnu++20), matching the existing build; CMAKE_CXX_STANDARD_REQUIRED stays ON and s7_lib keeps C11. GCC 13 / Apple Clang 16 / MSVC 2022 — the CI matrix — all support C++20. Also fixes the one new deprecation the bump surfaced: an implicit 'this' capture via [=] in EXTThread::start() is deprecated in C++20; make it [this]. (GCC 13 emits a false-positive -Wstringop-overflow from <atomic> in the offline file driver under C++20; it is not error-promoted and doesn't reproduce on Clang/MSVC.) Build + AOT + full non-graphics suite green under C++20.
MtSemaphore was explicitly a 'C++17 stand-in for std::counting_semaphore' — a hand-rolled mutex + condition_variable + counter. Now that the project requires C++20, replace it with std::counting_semaphore<> directly. The acquire() / release(n) call sites are unchanged (same API); both semaphores still start empty. libc++'s <semaphore> needs a macOS deployment target >= 11.0, which is exactly extempore's floor, so this is safe across the CI matrix. Build + libs-core/ audio-offline + the mtaudio example (which drives the dispatcher) green.
fp80_to_double_portable assembled the result double through a
union { uint64_t i; double d; }, which is undefined behaviour in C++ (legal
only in C). std::bit_cast<double>(result_bits) expresses the same
reinterpretation with defined behaviour. Build + libs-core/compiler-unit/
cpp-unit green.
The eight swap*/unswap* helpers (FFI-registered, called from xtlang) were ~130 lines of hand-written unsigned char* byte reversal -- the file's own comment called them 'UGLY AND INEFFICIENT'. They also reversed bytes unconditionally, which is correct only because every target is little-endian; the assumption was implicit. Replace the bodies with one osc_byteswap<T>() helper that reverses only when std::endian::native is little-endian, using std::bit_cast for the type-puns (no more aliasing through unsigned char*). The extern "C" signatures are unchanged, so the JIT ABI is preserved, and on little-endian hosts the output is byte-identical to before. The code is now also correct on big-endian hosts. Build + libs-core + the osc_101 round-trip example green.
The C++20 bump made the Windows CI build fail (MSVC C2375/C2733): the function was declared in EXTLLVM.h's extern "C" block without __declspec(dllexport) but defined with EXPORT (which adds it). MSVC tolerated this under C++17 but rejects the inconsistent dll-linkage under C++20. The definition's EXPORT is load-bearing (it provides the extern "C" linkage SchemeFFI.cpp links against — dropping it breaks the link on every platform), so add EXPORT to the header declaration to match. Linux build + link + libs-core/ compiler-unit green; Windows fix verified by CI.
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.
Staged C++ modernization from a full audit of the first-party runtime. Tier A here is the low-risk, Linux-verified set; Tier B (cross-platform: socket-stack unification, RT-priority dedup, Windows aligned-alloc) and Tier C (C++20 bump: counting_semaphore, endian/bit_cast) will follow on this branch. Draft until the full series lands.
Pushed now primarily to get cross-platform (Windows + macOS) CI confirmation of the platform-touching changes below before stacking Tier B.
Tier A commits (10)
TARGET_*BIT, dead<sys/syscall.h>,SCHEME_EXPORT, orphanstring_hash)#pragma oncetoSchemeREPL.h(had no guard); dedup includes/prototypes; IWYUstd::clamp(retires theUSE_SSE_AUDIO_SANITYx86#ifdef)string_hash,remove_*,escape_backslashes)malloc→std::vector)loadString(verified no FFI/IR refs)std::atomic(m_running,m_numFrames, 4 clock doubles)strcmp, null-deref do/while,intzone-size truncation, GC-unprotected temporaries,resetOutportString,platform()fallthrough, last-WshadowgetRealTimefromstd::chrono::system_clock(collapses 3-way#ifdef; fixes Windows boot-relative-epoch bug)unordered_map→map) for reproducible AOTVerification (Linux)
🤖 Generated with Claude Code