[jasoneplumb]

Summary

Function-level tracing via -finstrument-functions: hooks in cyclescope::instrument (a separately compiled, uninstrumented static library) record every call in the trace core; names resolve once per unique function via dladdr + demangling and are interned for the process lifetime.

Closes #3

Changes

• src/instrument.cpp: __cyg_profile hooks with fixed-depth (512) POD per-thread frame stacks, overflow counting, unmatched-frame protection (longjmp/exceptions), reentrancy + collector suppression
• include/cyclescope/trace.hpp: thread-local suppress flag set inside record/write_json/clear and honored by the hooks; collector singleton made immortal
• Tests: instrumented fixture TU only (assertions run over a known call set); covers event naming/counts and the flush-from-instrumented-code deadlock regression, with a ctest timeout so a regression fails rather than hangs
• examples/demo.cpp: instrumented fib demo writing demo.trace.json for ui.perfetto.dev; ENABLE_EXPORTS so dladdr resolves executable symbols on Linux
• CMake: cyclescope::instrument target (non-MSVC), examples option

Two defects found and fixed during bring-up (real runs, stack-sampled)

1. Self-deadlock: the linker chose the demo TU's instrumented copy of write_json; a hook fired while the thread held a buffer mutex and record() relocked it. Fixed by the suppression flag; regression-tested.
2. Teardown crash: an exit hook after static destruction locked a destroyed function-local static mutex (uncaught system_error in a noexcept context). Fixed by making the interning mutex and collector immortal.

Test plan

• Release and TSan green locally on macOS arm64 (3/3 binaries); demo exits 0 with 13,512 events and valid JSON verified by a parser

Assumptions

• MSVC has no -finstrument-functions; the backend target is non-MSVC only, scope macros remain fully portable
• Symbols outside the dynamic symtab fall back to their address; executables should set ENABLE_EXPORTS on Linux (documented)

🤖 Generated with Claude Code

[claude]

Potential infinite recursion in debug builds

suppress_scope::suppress_scope() is an inline function defined in trace.hpp. When a TU compiled with -finstrument-functions (e.g., demo.cpp, which is the sole non-library TU in the demo and includes trace.hpp) is linked before cyclescope_instrument.a, the linker selects the instrumented weak symbol for the constructor. At -O0, instrument.cpp emits a real call instruction here rather than inlining the two-line body.

Consequence: entering that instrumented constructor fires __cyg_profile_func_enter(suppress_scope_ctor_addr, …). At that point tls_suppress is still false (the constructor hasn't run yet), so the check at line 101 passes and the hook tries to construct another suppress_scope → infinite recursion → stack overflow.

The same risk applies at line 119 in the exit hook.

Because the hooks are no_instrument_function, no recursion guard exists for the hook's own entry, so the suppression mechanism must be in place before any potentially-instrumented function is called.

Fix – avoid the function call entirely:

Suggested change

	cyclescope::detail::suppress_scope suppress;
	cyclescope::detail::tls_suppress = true;
	frame_stack& stack = tls_stack;

Then at the end of the hook body (before the closing }) restore with cyclescope::detail::tls_suppress = false;. The hooks are non-throwing C functions, so no RAII is needed for the restore.

Alternatively, add [[gnu::always_inline]] / __attribute__((always_inline)) to both suppress_scope::suppress_scope() and ~suppress_scope() in trace.hpp so the constructor is never emitted as an out-of-line call.

[claude]

Same suppress_scope recursion risk applies to all three inline collector methods

record(), write_json() (line 112), and clear() (line 174) all create a suppress_scope to prevent re-entrancy. When any of these inline functions is instantiated in an instrumented TU (e.g. instrument_fixture.cpp or demo.cpp) and the linker selects that instrumented copy program-wide, the entry hook for the function fires before suppress_scope is constructed. If the compiler also emits a call to the un-inlined suppress_scope::suppress_scope() (likely at -O0), the constructor's entry hook fires with tls_suppress == false, and we have the same recursion chain described on the hooks.

record() is the most critical case: it is called on the hot path from every exit hook, so if the linker picks an instrumented copy the recursion is reached on every traced call, not just during flush.

The cleanest fix across the board is to mark both suppress_scope methods always_inline:

struct suppress_scope {
  bool previous;
  [[gnu::always_inline]] suppress_scope() noexcept
      : previous(tls_suppress) { tls_suppress = true; }
  suppress_scope(const suppress_scope&) = delete;
  suppress_scope& operator=(const suppress_scope&) = delete;
  [[gnu::always_inline]] ~suppress_scope() { tls_suppress = previous; }
};

This guarantees no call instruction is emitted for the guard regardless of TU, optimization level, or linker symbol selection.

[claude]

cyclescope_instrument library not compiled with sanitizer flags

When CYCLESCOPE_SANITIZE=thread (or address), the compile/link options are applied only to instrument_test but not to the cyclescope_instrument static library. Object code from src/instrument.cpp (the hooks, interned_name, frame_stack accesses) enters the final binary without sanitizer instrumentation, so TSan/ASan cannot observe data races or memory errors in that code.

The top-level CMakeLists.txt builds cyclescope_instrument unconditionally before the test option is read, so the simplest fix is to propagate the flag there or use an INTERFACE propagation:

if(CYCLESCOPE_SANITIZE)
  target_compile_options(cyclescope_instrument PUBLIC
    -fsanitize=${CYCLESCOPE_SANITIZE} -g -fno-omit-frame-pointer)
  target_link_options(cyclescope_instrument PUBLIC
    -fsanitize=${CYCLESCOPE_SANITIZE})
endif()

(Use PUBLIC rather than PRIVATE so the flag is consistently applied to any target linking the library.)

[claude]

Review: Compiler-instrumentation backend

Reviewed: src/instrument.cpp, include/cyclescope/trace.hpp, tests/CMakeLists.txt, tests/instrument_test.cpp, tests/instrument_fixture.cpp, examples/demo.cpp, and both CMakeLists.txt files.

The overall architecture — fixed-depth per-thread frame stacks, immortal collector/intern-map singletons, and the reentrancy-suppression flag — is well thought out. The two defects called out in the PR description (self-deadlock and teardown crash) are correctly diagnosed and fixed. Two substantive issues remain.

---

Issue 1 — Infinite recursion in debug builds (high severity)

Files: src/instrument.cpp lines 104 & 119, include/cyclescope/trace.hpp lines 89, 112, 174

suppress_scope::suppress_scope() and ~suppress_scope() are inline functions. Any TU compiled with -finstrument-functions (e.g. demo.cpp, instrument_fixture.cpp) emits an instrumented weak-symbol copy of these. On ELF the linker processes executable object files before static-library archives, so the instrumented copy is typically chosen program-wide.

At -O0, instrument.cpp (compiled without -finstrument-functions) may emit a real call instruction to suppress_scope::suppress_scope() rather than inlining the two-line body. That call resolves to the instrumented copy, whose entry fires __cyg_profile_func_enter before tls_suppress is set to true — bypassing the check at the top of the hook. The hook then tries to construct another suppress_scope, calling the instrumented constructor again → infinite recursion → stack overflow.

The same applies to the inline record(), write_json(), and clear() methods in trace.hpp, which each create a suppress_scope while potentially executing as instrumented copies.

Fix: add [[gnu::always_inline]] (or __attribute__((always_inline))) to both suppress_scope::suppress_scope() and ~suppress_scope(). This guarantees no call instruction is ever emitted, regardless of TU, optimization level, or linker symbol selection. (See inline comments for details.)

---

Issue 2 — cyclescope_instrument not compiled with sanitizer flags (moderate severity)

File: tests/CMakeLists.txt, CMakeLists.txt

Sanitizer compile/link options are applied only to the instrument_test executable, not to the cyclescope_instrument static library. src/instrument.cpp — which contains the hooks, interned_name, and all frame-stack manipulation — enters the final binary without TSan/ASan instrumentation. Races or memory errors in that code are invisible to the sanitizer.

Fix: propagate -fsanitize=${CYCLESCOPE_SANITIZE} (and -g -fno-omit-frame-pointer) to cyclescope_instrument when CYCLESCOPE_SANITIZE is set. (See inline comment for a concrete suggestion.)

---

Everything else looks correct: lock ordering (registry_mutex_ always taken before buffer mutex), TLS teardown safety (trivially-destructible frame_stack/frame types), the immortal singletons, the matched-frame check for longjmp/exception safety, the interned-name fallback for unknown symbols, and the test coverage for the flush-deadlock regression.

[jasoneplumb]

Both findings applied. suppress_scope's constructor and destructor are forced inline (with the ODR rationale in a comment), and the fix is validated by an actual Debug (-O0) build: the instrumented demo runs to a clean exit where the described recursion would have overflowed. The instrument library now compiles and links with the sanitizer flags when CYCLESCOPE_SANITIZE is set; the cache variable is hoisted above the library block so the guard actually sees it. All three configs (Release, TSan, Debug) are 3/3 locally.

🤖 Generated with Claude Code

[claude]

Sanitizer link flags propagate to uninstrumented consumers via PUBLIC scope.

target_link_options(... PUBLIC -fsanitize=...) on a static library propagates the flag to every consuming target. cyclescope_demo (and any downstream library user who sets CYCLESCOPE_SANITIZE) will link the sanitizer runtime without their code being compiled with -fsanitize. For TSAN this is genuinely problematic: partially-instrumented binaries can produce false positives because TSAN's shadow memory doesn't account for accesses in uninstrumented frames. For instrument_test the flag is harmlessly duplicated (once from this PUBLIC propagation, once from the test's own target_link_options).

Consider using PRIVATE for the link options and having each consuming executable opt in, as instrument_test already does:

Suggested change

	-fsanitize=${CYCLESCOPE_SANITIZE})
	target_link_options(cyclescope_instrument PRIVATE
	-fsanitize=${CYCLESCOPE_SANITIZE})

If the intent is to auto-propagate to consumers, document the expectation explicitly and add the compile options to INTERFACE or PUBLIC too so the whole toolchain is consistent.

[claude]

CYCLESCOPE_ALWAYS_INLINE macro leaks into consumer namespace.

The macro is defined in a public header but never #undef'd. Any translation unit that includes <cyclescope/trace.hpp> inherits it and could conflict with a consumer-defined macro of the same name. Add #undef CYCLESCOPE_ALWAYS_INLINE immediately after the suppress_scope struct that uses it:

Suggested change

	#endif
	#define CYCLESCOPE_ALWAYS_INLINE __attribute__((always_inline)) inline
	#endif

(with a #undef CYCLESCOPE_ALWAYS_INLINE inserted after the closing brace of suppress_scope)

[claude]

Belt-and-suspenders: suppress_scope ctor/dtor could also carry no_instrument_function.

The comment explains the correctness argument: always_inline forces the ctor/dtor to be inlined at every call site, so -finstrument-functions never inserts a function-entry/exit hook for them. This is sound in practice.

However, if an instrumented TU happens to emit an out-of-line copy (e.g. via a pointer-to-member, a virtual dispatch, or a compiler that declines to inline in certain debug/LTO paths), the entry hook would fire before tls_suppress = true is set, allowing a hook to re-enter and recurse unboundedly — exactly the failure mode the comment warns about.

Adding __attribute__((no_instrument_function)) to both the constructor and destructor provides an independent, compiler-enforced guarantee at zero runtime cost:

struct suppress_scope {
  bool previous;
  CYCLESCOPE_ALWAYS_INLINE __attribute__((no_instrument_function))
  suppress_scope() noexcept : previous(tls_suppress) {
    tls_suppress = true;
  }
  suppress_scope(const suppress_scope&) = delete;
  suppress_scope& operator=(const suppress_scope&) = delete;
  CYCLESCOPE_ALWAYS_INLINE __attribute__((no_instrument_function))
  ~suppress_scope() { tls_suppress = previous; }
};

This is a defence-in-depth change, not a fix for a currently-triggered bug.

[claude]

Review Summary

Reviewed: CMakeLists.txt, src/instrument.cpp, include/cyclescope/trace.hpp, tests/instrument_test.cpp, tests/instrument_fixture.cpp, examples/demo.cpp.

The suppression/reentrancy mechanism is well-designed and sound: tls_suppress + suppress_scope + no_instrument_function on the hooks + the immortal singleton correctly handle all the tricky lifecycle cases (hook re-entry, collector-mutex deadlock, teardown ordering). The frame stack overflow tracking and longjmp/exception mismatch guard (entered.fn == fn) are also correct.

Three issues found:

1. Medium: target_link_options(cyclescope_instrument PUBLIC -fsanitize=...) in CMakeLists.txt line 30. PUBLIC scope propagates the sanitizer link flag to all consumers. cyclescope_demo ends up linked with the sanitizer runtime but compiled without -fsanitize, creating a mixed-instrumentation binary. For TSAN this can produce false positives; for ASAN it silently misses stack coverage. instrument_test also ends up with duplicate -fsanitize on the link command. Using PRIVATE would let each consumer opt in explicitly, as instrument_test already does.

2. Low: CYCLESCOPE_ALWAYS_INLINE macro not undef'd after use in the public header (trace.hpp line 48). Leaks into every consumer TU.

3. Low (defence-in-depth): suppress_scope ctor/dtor not also marked no_instrument_function (trace.hpp lines 52-59). Correctness currently depends solely on always_inline. Adding attribute((no_instrument_function)) alongside would make the reentrancy protection truly bulletproof.

[jasoneplumb]

All three cycle 2 findings applied: the static library no longer propagates the sanitizer link flag, and the demo opts into full sanitizer compile+link under CYCLESCOPE_SANITIZE so no binary mixes instrumentation (verified: TSan-built demo runs clean); the helper macros are undef'd after use; and suppress_scope carries no_instrument_function alongside always_inline. Findings plateaued; merging on green.

🤖 Generated with Claude Code