Zephyr preproc.h implementation (OPTION B) #9603
Conversation
kv2019i
requested review from
lyakh,
iuliana-prodan,
dbaluta,
abonislawski,
ranj063,
jxstelter,
lgirdwood,
plbossart,
mmaka1 and
lbetlej
as code owners
|
Interesting, normal SOF builds are ok for both XTOS and Zephyr, but library builds (cmocka, testbench, sof ALSA plugin) fail. I'll debug this, please proceed to review the overall approach and let me know if this is a deadend and I'll stop trying to make this work... |
Add __maybe_unused attribute for XTOS, so this attribute can be used in common SOF code (it's already supported in Zephyr toolchains). Signed-off-by: Kai Vehmanen <[email protected]>
Add __maybe_unused attribute for posix RTOS layer, so this attribute can be used in common SOF code (it's already supported in Zephyr+XTOS). Signed-off-by: Kai Vehmanen <[email protected]>
Mark local variables that are only used in logging macros with "__maybe_unused" attribute. The compiler errors are currently avoided with preprocessor logic done in preproc.h for SOF_TRACE_UNUSED, but this brings in quite a bit of definitions not really needed in Zephyr logs. Signed-off-by: Kai Vehmanen <[email protected]>
Add Zephyr version of sof/trace/preproc.h interface. The full preproc.h interface needed for CONFIG_TRACE is not duplicated here, so this commit effectively prevents using CONFIG_TRACE in Zephyr. Link: thesofproject#9015 Signed-off-by: Kai Vehmanen <[email protected]>
Make CONFIG_TRACE=n the default for Zephyr builds and update document to explain the trade-offs when using CONFIG_TRACE on Zephyr. Signed-off-by: Kai Vehmanen <[email protected]>
kv2019i
force-pushed
the
202410-zephyr-preproc-optb
branch
from
b0168df to
6e167ca
Compare
| * Unlike the XTOS version of this macro, this does not silence all | ||
| * compiler warnings about unused variables. | ||
| */ | ||
| #define SOF_TRACE_UNUSED(arg1, ...) ((void)arg1) |
There was a problem hiding this comment.
doesn't silence all - is it because of the lack of recursion? Leaving the rest of the arguments without void-casting? Should that be done instead?
There was a problem hiding this comment.
@lyakh Ack, in "OPTION A" (9598) there's "full silencing implementation". preproc-*.h does just this and question is whether to move that out of XTOS code or remove it, and Liam was not keen to have this in SOF main library. I'd say, if you want full silencing, then just pull in preproc-*h, it's does just the thing, has the recursion helpers to iterate through the varargs in preprocessor. Looking at the code base, there's actually much less use of this than I thought (in common code at least), so just requiring callers to use __maybe_unused seems not so impractical after all. And for sure easier to maintain code in SOF tracing layer (that we want to minimize).
There was a problem hiding this comment.
the first argument to SOF_TRACE_UNUSED() is the trace context, right? That's why we have to "silence" it here, because it isn't obvious where we actually call comp_dbg() and others. And other arguments we'll cast to void at their locations?
There was a problem hiding this comment.
@lyakh This is really a very niche scenario of building with Zephyr but disable Zephyr logs (and fully disable, not just limiting logs to error level or something more usual). Now in Zephyr, logs are disabled like
#define LOG_DBG(...) (void) 0
In "old SOF", there has been more elaborate macro magic that disables unused variable warnings even if a local variable is only used in "comp_dbg(comp, "print local variable foo %d", foo)" statements. Now we could handle this in multiple ways:
- bring in the metaprocessor logic to support above kind of use (my OPTION A PR)
- use __maybe_unused and be aligned how Zephyr debugging stameemnts work (this PR)
- just live with the unsused variable warnings (currently treated as errors in our build and will fail CI)
I guess there are more variants of (1), but I'd say it's not worth the effort to make this pretty (especially as Zephyr does not do it).
raio. You are building with Zephyr but with Zephyr logging subsystem disabled (and fully disabled, not just limiting to plain error logs like we do on some targets). Zephyr headers take care of the
|
@lyakh ping? |
| @@ -239,7 +239,7 @@ static int module_adapter_ctrl_get_set_data(struct comp_dev *dev, struct sof_ipc | |||
| bool set) | |||
| { | |||
| int ret; | |||
| struct processing_module *mod = comp_mod(dev); | |||
| struct processing_module __maybe_unused *mod = comp_mod(dev); | |||
There was a problem hiding this comment.
At first I didn't like this, because it adds less than pretty code to potentially many locations (we need to handle information-, warning- and error-level messages similarly, right?), but then I thought, that if we have an automated way to identify all these locations, then maybe this is the best way to handle them. Firstly it's honest and explicit - it helps keep track of such cases and maybe avoid adding variables that can easily be avoided. The same holds for variables, only used in assert() calls, right? So, yes, I'd buy this.
| * Unlike the XTOS version of this macro, this does not silence all | ||
| * compiler warnings about unused variables. | ||
| */ | ||
| #define SOF_TRACE_UNUSED(arg1, ...) ((void)arg1) |
There was a problem hiding this comment.
the first argument to SOF_TRACE_UNUSED() is the trace context, right? That's why we have to "silence" it here, because it isn't obvious where we actually call comp_dbg() and others. And other arguments we'll cast to void at their locations?
@kv2019i yeah, tend to agree with @lgirdwood - this one looks better |
|
I'll go ahead and merge. If we figure out a prettier way to silence the unsused variable errors (see my comment above), we can do that in separate PR. For now, this seems most aligned with how Zephyr does things and would seem to make most sense for SOF Zephyr builds. |
Alternative to pull request #9598 and follow-up to discussion in #9598 (comment) .
This takes the decision to not support dma-trace (sof-logger) in Zephyr builds. and this allows to drop preproc.h family of preprocessor macro infra. In current CI, one IMX target is built with CONFIG_TRACE=n and CONFIG_ZEPHYR_LOG=n, and will hit build errors without the patches in this set to add maybe_unused attribute and a simple definition of SOF_TRACE_UNUSED().
Motivation for this work is #9015 and clean separate of XTOS and Zephyr headers.