Skip to content

trace: avoid passing va_list #4574

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

Closed
ranj063 wants to merge 2 commits into from
Closed

trace: avoid passing va_list #4574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c4744ab
trace: avoid passing va_list
ranj063 Jul 29, 2021
757755a
APL defconfig: remove some unnecessary components
ranj063 Jul 29, 2021
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 src/arch/xtensa/configs/apollolake_defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,7 @@ CONFIG_PERFORMANCE_COUNTERS=y
CONFIG_COMP_SRC_SMALL=y
CONFIG_COMP_TDFB=n
CONFIG_COMP_TONE=n
CONFIG_COMP_KPB=n
Copy link
Collaborator

@lyakh lyakh Jul 30, 2021

Choose a reason for hiding this comment

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

btw, we have a weird sof-apl-keyword-detect.m4 which presumably uses KPB on APL, although FWIW my brief test of that topology on UP2 wasn't very successful, so maybe it's broken anyway

CONFIG_COMP_SEL=n
CONFIG_COMP_CROSSOVER=n
CONFIG_COMP_DRC=n
Copy link
Member

@plbossart plbossart Jul 30, 2021

Choose a reason for hiding this comment

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

A change to be va_list leads to a build failure?
Something's not right here...

Copy link
Collaborator Author

@ranj063 ranj063 Jul 30, 2021

Choose a reason for hiding this comment

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

@plbossart i've made the _log_message() macro heavier with this change and so it breaks the bss section for APL

36 changes: 24 additions & 12 deletions src/include/sof/trace/trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ struct trace_filter {

#if CONFIG_TRACE

#include <stdarg.h>
#include <user/trace.h> /* LOG_LEVEL_... */

/*
Expand Down Expand Up @@ -147,13 +146,10 @@ void trace_off(void);
void trace_init(struct sof *sof);

/* All tracing macros in this file end up calling these functions in the end. */
typedef void (*log_func_t)(bool send_atomic, const void *log_entry, const struct tr_ctx *ctx,
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count, va_list args);

void trace_log_filtered(bool send_atomic, const void *log_entry, const struct tr_ctx *ctx,
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count, va_list args);
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count, ...);
void trace_log_unfiltered(bool send_atomic, const void *log_entry, const struct tr_ctx *ctx,
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count, va_list args);
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count, ...);
struct sof_ipc_trace_filter_elem *trace_filter_fill(struct sof_ipc_trace_filter_elem *elem,
struct sof_ipc_trace_filter_elem *end,
struct trace_filter *filter);
Expand Down Expand Up @@ -217,11 +213,6 @@ void mtrace_event(const char *complete_packet, uint32_t length);
# define MTRACE_DUPLICATION_LEVEL LOG_LEVEL_ERROR
#endif /* CONFIG_TRACEM */

/* This function is _not_ passed the format string to save space */
void _log_sofdict(log_func_t sofdict_logf, bool atomic, const void *log_entry,
const struct tr_ctx *ctx, const uint32_t lvl,
uint32_t id_1, uint32_t id_2, int arg_count, ...);

/* _log_message() */

#ifdef CONFIG_LIBRARY
Expand Down Expand Up @@ -264,6 +255,26 @@ _thrown_from_macro_BASE_LOG_in_trace_h
* linker section and replaced by a &log_entry pointer to it. This must
* be a macro for the source location to be meaningful.
*/
#ifndef __ZEPHYR__
#define _log_message(log_func, atomic, lvl, comp_class, ctx, id_1, id_2, format, ...) \
do { \
_DECLARE_LOG_ENTRY(lvl, format, comp_class, \
META_COUNT_VARAGS_BEFORE_COMPILE(__VA_ARGS__)); \
STATIC_ASSERT_ARG_SIZE(__VA_ARGS__); \
STATIC_ASSERT(_TRACE_EVENT_MAX_ARGUMENT_COUNT >= \
META_COUNT_VARAGS_BEFORE_COMPILE(__VA_ARGS__), \
BASE_LOG_ASSERT_FAIL_MSG \
); \
(lvl <= MTRACE_DUPLICATION_LEVEL ? \
mtrace_dict_entry(true, (uint32_t)&log_entry, \
META_COUNT_VARAGS_BEFORE_COMPILE(__VA_ARGS__), \
##__VA_ARGS__) : 0); \
Copy link
Collaborator

@lyakh lyakh Jul 30, 2021

Choose a reason for hiding this comment

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

The ?: operator is (usually) used for returning a value. Do I understand it correctly that you just meant an if here? Also it would be nice to keep indentation for end-of-line backslashes

log_func(atomic, &log_entry, ctx, lvl, id_1, id_2, \
META_COUNT_VARAGS_BEFORE_COMPILE(__VA_ARGS__), ##__VA_ARGS__); \
_log_nodict(atomic, META_COUNT_VARAGS_BEFORE_COMPILE(__VA_ARGS__), \
lvl, format, ##__VA_ARGS__); \
} while (0)
#else
#define _log_message(log_func, atomic, lvl, comp_class, ctx, id_1, id_2, format, ...) \
do { \
_DECLARE_LOG_ENTRY(lvl, format, comp_class, \
Expand All @@ -273,11 +284,12 @@ do { \
META_COUNT_VARAGS_BEFORE_COMPILE(__VA_ARGS__), \
BASE_LOG_ASSERT_FAIL_MSG \
); \
_log_sofdict(log_func, atomic, &log_entry, ctx, lvl, id_1, id_2, \
log_func(atomic, &log_entry, ctx, lvl, id_1, id_2, \
META_COUNT_VARAGS_BEFORE_COMPILE(__VA_ARGS__), ##__VA_ARGS__); \
_log_nodict(atomic, META_COUNT_VARAGS_BEFORE_COMPILE(__VA_ARGS__), \
lvl, format, ##__VA_ARGS__); \
} while (0)
#endif

#ifdef __ZEPHYR__
/* Just like XTOS, only the most urgent messages go to limited
Expand Down
44 changes: 12 additions & 32 deletions src/trace/trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,21 +249,25 @@ static void dma_trace_log(bool send_atomic, uint32_t log_entry, const struct tr_
}

void trace_log_unfiltered(bool send_atomic, const void *log_entry, const struct tr_ctx *ctx,
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count, va_list vl)
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count, ...)
{
struct trace *trace = trace_get();
va_list vl;

if (!trace->enable) {
return;
}

va_start(vl, arg_count);
dma_trace_log(send_atomic, (uint32_t)log_entry, ctx, lvl, id_1, id_2, arg_count, vl);
va_end(vl);
}

void trace_log_filtered(bool send_atomic, const void *log_entry, const struct tr_ctx *ctx,
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count, va_list vl)
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count, ...)
{
struct trace *trace = trace_get();
va_list vl;

#if CONFIG_TRACE_FILTERING_ADAPTIVE
uint64_t current_ts;
Expand All @@ -289,7 +293,9 @@ void trace_log_filtered(bool send_atomic, const void *log_entry, const struct tr
}
#endif /* CONFIG_TRACE_FILTERING_ADAPTIVE */

va_start(vl, arg_count);
dma_trace_log(send_atomic, (uint32_t)log_entry, ctx, lvl, id_1, id_2, arg_count, vl);
va_end(vl);
}

struct sof_ipc_trace_filter_elem *trace_filter_fill(struct sof_ipc_trace_filter_elem *elem,
Expand Down Expand Up @@ -509,9 +515,9 @@ void trace_init(struct sof *sof)
dma_trace_init_early(sof);
}

static void mtrace_dict_entry_vl(bool atomic_context, uint32_t dict_entry_address,
int n_args, va_list ap)
void mtrace_dict_entry(bool atomic_context, uint32_t dict_entry_address, int n_args, ...)
{
va_list ap;
int i;
char packet[MESSAGE_SIZE(_TRACE_EVENT_MAX_ARGUMENT_COUNT)];
uint32_t *args = (uint32_t *)&packet[MESSAGE_SIZE(0)];
Expand All @@ -520,8 +526,10 @@ static void mtrace_dict_entry_vl(bool atomic_context, uint32_t dict_entry_addres
put_header(packet, dt_tr.uuid_p, _TRACE_INV_ID, _TRACE_INV_ID,
dict_entry_address, tstamp);

va_start(ap, n_args);
for (i = 0; i < MIN(n_args, _TRACE_EVENT_MAX_ARGUMENT_COUNT); i++)
args[i] = va_arg(ap, uint32_t);
va_end(ap);

if (atomic_context) {
mtrace_event(packet, MESSAGE_SIZE(n_args));
Expand All @@ -534,31 +542,3 @@ static void mtrace_dict_entry_vl(bool atomic_context, uint32_t dict_entry_addres
spin_unlock_irq(&trace->lock, saved_flags);
}
}

void mtrace_dict_entry(bool atomic_context, uint32_t dict_entry_address, int n_args, ...)
{
va_list ap;

va_start(ap, n_args);
mtrace_dict_entry_vl(atomic_context, dict_entry_address, n_args, ap);
va_end(ap);
}

void _log_sofdict(log_func_t sofdict_logf, bool atomic, const void *log_entry,
const struct tr_ctx *ctx, const uint32_t lvl,
uint32_t id_1, uint32_t id_2, int arg_count, ...)
{
va_list ap;

#ifndef __ZEPHYR__ /* for Zephyr see _log_nodict() in trace.h */
if (lvl <= MTRACE_DUPLICATION_LEVEL) {
va_start(ap, arg_count);
mtrace_dict_entry_vl(atomic, (uint32_t)log_entry, arg_count, ap);
va_end(ap);
}
#endif

va_start(ap, arg_count);
sofdict_logf(atomic, log_entry, ctx, lvl, id_1, id_2, arg_count, ap);
va_end(ap);
}
10 changes: 1 addition & 9 deletions test/cmocka/src/common_mocks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,7 @@ void WEAK __panic(uint32_t p, char *filename, uint32_t linenum)

#if CONFIG_TRACE
void WEAK trace_log_filtered(bool send_atomic, const void *log_entry, const struct tr_ctx *ctx,
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count,
va_list args)
uint32_t lvl, uint32_t id_1, uint32_t id_2, int arg_count, ...)
{
(void) send_atomic;
(void) log_entry;
Expand All @@ -112,13 +111,6 @@ void WEAK trace_log_filtered(bool send_atomic, const void *log_entry, const stru
(void) id_1;
(void) id_2;
(void) arg_count;
(void) args;
}

void WEAK _log_sofdict(log_func_t sofdict_logf, bool atomic, const void *log_entry,
const struct tr_ctx *ctx, const uint32_t lvl,
uint32_t id_1, uint32_t id_2, int arg_count, ...)
{
}

void WEAK trace_flush_dma_to_mbox(void)
Expand Down