Fix: strace.h must include profiling_config.h (STRACE silently no-op'd in c_api_shared)#1268
Fix: strace.h must include profiling_config.h (STRACE silently no-op'd in c_api_shared)#1268ChaoWao wants to merge 1 commit into
Conversation
…d in c_api_shared)
strace.h gated its macros on `#if SIMPLER_PROFILING` but never included
profiling_config.h (the header that defines SIMPLER_PROFILING=1). It relied on
each TU to pull the definition in transitively — which the runtime pto_*.h
headers do, but the platform c_api_shared.cpp (which wraps simpler_run and owns
the root STRACE("simpler_run") / .bind / .runner_run / .validate spans) does
not. There, SIMPLER_PROFILING was undefined, the gate evaluated false, and every
STRACE compiled to a no-op.
Net effect: the host-trace markers for each simpler_run were silently dropped on
the platform path (the very path the DFX contract relies on — pypto-serving
reads per-stage timing of each simpler_run purely from these [STRACE] log
lines). This surfaced as a persistent CI failure in
examples/workers/l2/vector_add/test_run_timing.py::test_simpler_run_emits_strace_markers,
which asserts the markers are present.
Fix: make strace.h self-contained by including profiling_config.h itself, so
SIMPLER_PROFILING is always defined for any TU that uses the macros. One-line
include; no behavior change for TUs that already had it.
Verified: the previously-failing test now passes on a2a3sim; all 4 platforms ×
2 runtimes build clean; a2a3sim/a5sim hbg + tensormap scene tests pass.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change makes ChangesStrace Header Fix
Estimated code review effort: 1 (Trivial) | ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request includes "profiling_config.h" in the low-level header "strace.h" to ensure it is self-contained and correctly resolves the SIMPLER_PROFILING macro. The reviewer notes that this introduces a layering violation, as low-level logging utilities should not depend on higher-level task interface modules, and suggests relocating the configuration or using structured include paths to prevent compilation issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // `#if SIMPLER_PROFILING` gate below evaluate false and silently compile every | ||
| // STRACE to a no-op — dropping the simpler_run host-trace markers that | ||
| // consumers (pypto-serving) read from the log. | ||
| #include "profiling_config.h" |
There was a problem hiding this comment.
Including profiling_config.h directly from task_interface inside a low-level header in common/log introduces a layering violation (dependency inversion). Low-level utilities like logging should not depend on higher-level modules like task_interface. This forces any target using common/log to include task_interface in its header search paths, which can break compilation for other targets or modules that do not use task_interface.
Additionally, using a flat include like #include "profiling_config.h" makes the dependency implicit and prone to header name collisions.
Suggested Improvements:
- Relocate the configuration: Move
profiling_config.h(or at least theSIMPLER_PROFILINGdefinition) to a lower-level common configuration directory (e.g.,src/common/log/include/common/or a dedicatedcommon/config/directory). - Use structured paths: If the dependency must remain, use a structured include path (e.g.,
#include "task_interface/profiling_config.h") and ensure that thecommon/logCMake target publicly propagates this dependency so that all consumers can resolve the include path.
| #include "profiling_config.h" | |
| #include "task_interface/profiling_config.h" |
1 participant
Problem
strace.hgates all its macros on#if SIMPLER_PROFILINGbut never includesprofiling_config.h— the header that definesSIMPLER_PROFILING=1. It relied on each TU to pull the definition in transitively.The runtime's
pto_*.hheaders include it, but the platformc_api_shared.cpp(which wrapssimpler_runand owns the rootSTRACE("simpler_run")+.bind/.runner_run/.validatespans) does not. There,SIMPLER_PROFILINGwas undefined →#if SIMPLER_PROFILINGevaluated false → everySTRACEcompiled to a no-op.Impact
The host-trace markers for each
simpler_runwere silently dropped on the platform path — the very path the DFX contract relies on (consumers like pypto-serving read per-stage timing of eachsimpler_runpurely from these[STRACE]log lines).This surfaced as a persistent CI failure (pre-existing on
main, unrelated to any recent feature PR):Fix
Make
strace.hself-contained — includeprofiling_config.hitself soSIMPLER_PROFILINGis always defined for any TU using the macros. One-line include; no behavior change for TUs that already had the definition.How the root cause was found
Instrumenting
simpler_runconfirmed it was entered and returnedrc=0, yet itsSTRACE("simpler_run")destructor never emitted — while inner spans fromruntime_maker.cpp(a different TU that does transitively includeprofiling_config.h) emitted fine. That per-TU split pointed straight at the missing transitive include inc_api_shared.cpp.Verification
test_simpler_run_emits_strace_markersnow passes ona2a3sim(was failing onmain).-Werror.