Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zephyr preproc.h implementation (OPTION B) #9603

Merged
merged 5 commits into from
Oct 25, 2024
Merged

Zephyr preproc.h implementation (OPTION B) #9603

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
915dc23
xtos: compiler_attributes: add definition for __maybe_unused
kv2019i Oct 21, 2024
5bbaa1e
posix: compiler_attributes: add definition for __maybe_unused
kv2019i Oct 21, 2024
6ec9590
trace: mark unused variables with __maybe_unused
kv2019i Oct 21, 2024
40f0911
zephyr: add sof/trace/preproc.h
kv2019i Oct 9, 2024
6e167ca
trace: make CONFIG_TRACE=n the default for Zephyr builds
kv2019i Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions posix/include/sof/compiler_attributes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@
#define __unused __attribute__((unused))
#endif

#ifndef __maybe_unused
#define __maybe_unused __attribute__((unused))
#endif

#endif

#ifndef __aligned
Expand Down
2 changes: 1 addition & 1 deletion src/audio/module_adapter/module_adapter_ipc3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


comp_dbg(dev, "module_adapter_ctrl_set_data() start, state %d, cmd %d",
mod->priv.state, cdata->cmd);
Expand Down
4 changes: 2 additions & 2 deletions src/audio/pipeline/pipeline-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,9 @@ int pipeline_complete(struct pipeline *p, struct comp_dev *source,
};

#if !UNIT_TEST && !CONFIG_LIBRARY
int freq = clock_get_freq(cpu_get_id());
int __maybe_unused freq = clock_get_freq(cpu_get_id());
#else
int freq = 0;
int __maybe_unused freq = 0;
#endif
int ret;

Expand Down
2 changes: 1 addition & 1 deletion src/ipc/ipc3/dai.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ int ipc_dai_data_config(struct dai_data *dd, struct comp_dev *dev)
int ipc_comp_dai_config(struct ipc *ipc, struct ipc_config_dai *common_config,
void *spec_config)
{
struct sof_ipc_dai_config *config = spec_config;
struct sof_ipc_dai_config __maybe_unused *config = spec_config;
bool comp_on_core[CONFIG_CORE_COUNT] = { false };
struct sof_ipc_reply reply;
struct ipc_comp_dev *icd;
Expand Down
4 changes: 4 additions & 0 deletions xtos/include/sof/compiler_attributes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
#define __unused __attribute__((unused))
#endif

#ifndef __maybe_unused
#define __maybe_unused __attribute__((unused))
#endif

#ifndef __aligned
#define __aligned(x) __attribute__((__aligned__(x)))
#endif
Expand Down
24 changes: 24 additions & 0 deletions zephyr/include/sof/trace/preproc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/* SPDX-License-Identifier: BSD-3-Clause
*
* Copyright(c) 2024 Intel Corporation.
*/

#ifndef __SOF_TRACE_PREPROC_H__
#define __SOF_TRACE_PREPROC_H__

#ifdef CONFIG_TRACE
#error "SOF dma-trace (CONFIG_TRACE) not supported in Zephyr builds. \
Please use CONFIG_ZEPHYR_LOG instead."
#endif

#ifndef CONFIG_ZEPHYR_LOG

/*
* Unlike the XTOS version of this macro, this does not silence all
* compiler warnings about unused variables.
*/
#define SOF_TRACE_UNUSED(arg1, ...) ((void)arg1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@kv2019i kv2019i Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. bring in the metaprocessor logic to support above kind of use (my OPTION A PR)
  2. use __maybe_unused and be aligned how Zephyr debugging stameemnts work (this PR)
  3. 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


#endif /* CONFIG_ZEPHYR_LOG */

#endif /* __SOF_TRACE_PREPROC_H__ */