Trace - make clear that trace context is unused with Zephyr - #11102
Trace - make clear that trace context is unused with Zephyr#11102lyakh wants to merge 3 commits into
Conversation
Under Zephyr with CONFIG_ZEPHYR_LOG, tr_err()/tr_warn()/tr_info()/tr_dbg() expand directly to LOG_ERR()/LOG_WRN()/LOG_INF()/LOG_DBG() and completely ignore their first (tr_ctx) argument. A plain, otherwise-unreferenced DECLARE_TR_CTX() static is therefore dead and the linker discards it. However, a `struct comp_driver`/`struct dai_driver` instance whose `.tctx` field points at such a context keeps it alive, because the driver struct itself is always referenced (component/DAI driver registration). This adds a few bytes of otherwise-dead .data per affected driver and forces the firmware to retain values that can never be read on Zephyr. Guard the `.tctx = &foo_tr,` initializers (and the equivalent runtime `drv->tctx = &lib_manager_tr;` assignment) with #ifndef __ZEPHYR__ so the now fully-unreferenced trace contexts can be garbage-collected by the linker, same as any other unused DECLARE_TR_CTX(). Fix the shared DECLARE_MODULE_ADAPTER() macro once, which covers ~40 IPC4 module_adapter components: MODULE_ADAPTER_TCTX_INIT(tr) resolves to &(tr) normally and to NULL under Zephyr, so `.tctx = MODULE_ADAPTER_TCTX_INIT(tr),` still parses as a valid (harmless) initializer either way. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
pipeline_new() and buffer_new() unconditionally memcpy_s() the static pipe_tr/buffer_tr DECLARE_TR_CTX() objects into the new pipeline's/buffer's embedded tr_ctx field. On Zephyr this copy is pure overhead: comp_init() in component.h already skips the equivalent copy for comp_dev with #ifndef __ZEPHYR__, but these two sites were missed. buffer_new()'s copy was already conditional on \!CONFIG_SOF_USERSPACE_LL; simplify that to \!__ZEPHYR__ since CONFIG_SOF_USERSPACE_LL can only be defined in Zephyr builds. Also, since &pipe_tr/&buffer_tr were the only references to those objects, guarding the copy lets the linker garbage-collect them on Zephyr, same as ipc_tr and friends. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Under Zephyr with CONFIG_ZEPHYR_LOG, tr_err()/tr_warn()/tr_info()/tr_dbg() expand directly to LOG_ERR()/LOG_WRN()/LOG_INF()/LOG_DBG() and never look at their tr_ctx argument. None of these remaining DECLARE_TR_CTX() instances are referenced by anything other than tr_*() calls in the same file (verified: no .tctx/->tctx assignment picks up their address), so they are already dead code eliminated by the linker on Zephyr builds - this commit only documents that fact, it changes no generated code. (dai-legacy.c, host-legacy.c and the platform/library/schedule/*.c files are excluded: they are only ever built for non-Zephyr, non-hardware configurations - testbench/cmocka/legacy XTOS - so the comment would not apply to them.) Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Thanks for tackling, but I think this needs iteration still...
| @@ -41,6 +41,7 @@ | |||
| LOG_MODULE_REGISTER(basefw, CONFIG_SOF_LOG_LEVEL); | |||
|
|
||
| static const struct comp_driver comp_basefw = { | ||
| .uid = SOF_RT_UUID(basefw_uuid), | ||
| #ifndef __ZEPHYR__ |
There was a problem hiding this comment.
Could this be reversed? I mean these are only needed in LIBRARY builds now (or are they even needed there)? Or maybe that's not accurate enough, but maybe "#ifdef CONFIG_TRACE" (i.e. a build really using sof-logger with dictionaries).
And for Zephyr, this doesn't cover the case where you build with Zephyr but disable CONFIG_ZEPHYR_LOG. I think these cases should be prevented somehow.
| @@ -35,6 +35,7 @@ LOG_MODULE_REGISTER(pipe, CONFIG_SOF_LOG_LEVEL); | |||
|
|
|||
| SOF_DEFINE_REG_UUID(pipe); | |||
There was a problem hiding this comment.
What's with the "!CONFIG_SOF_USERSPACE_LL" in the commit message?
|
|
||
| #else | ||
|
|
||
| /* unused with Zephyr, generates no output */ |
There was a problem hiding this comment.
unused if not CONFIG_TRACE, not really related to Zephyr...
|
This adds a lot of duplicated comments for relatively little value (and wasted tokens for future agents). Is it worth repeating the same comment everywhere? Could we add it once to the definition of DECLARE_TR_CTX instead? |
@abonislawski well, it's a trade-off. Ideally I'd just remove them all. This seemed the next best option to me, but I'm open to opinions. "Future tokens" don't bother me TBH - I more care about humans (e.g. about myself) and personally every time I stumble over one of those my fingers begin to itch to remove it. So, I hope this at least could help. |
We have a lot of code that uses trace context like
tr_dbg()/tr_err()calls. Changing all those locations would be a very big change. This PR takes a middle ground: it adds comments to trace context declarations where they are harmless and makes harmless those, that end up generating needless code under Zephyr. Yes, made with AI.