Skip to content

ll_schedule.c: fix "uninitialized task_take" warning in CONFIG_TRACEV #3944

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
marc-hb wants to merge 1 commit into from
Closed

ll_schedule.c: fix "uninitialized task_take" warning in CONFIG_TRACEV #3944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions src/include/sof/trace/trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ static inline struct trace *trace_get(void)
return sof_get()->trace;
}

/* Silences compiler warnings about unused variables */
#define trace_unused(class, ctx, id_1, id_2, format, ...) \
UNUSED(ctx, id_1, id_2, ##__VA_ARGS__)

Expand Down Expand Up @@ -377,6 +378,7 @@ struct tr_ctx {
_TRACE_INV_ID, _TRACE_INV_ID, \
fmt, ##__VA_ARGS__)

/* tracev_ output depends on CONFIG_TRACEV=y */
#define tr_dbg(ctx, fmt, ...) \
tracev_event_with_ids(_TRACE_INV_CLASS, ctx, \
_TRACE_INV_ID, _TRACE_INV_ID, fmt, ##__VA_ARGS__)
Expand Down
10 changes: 6 additions & 4 deletions src/schedule/ll_schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,24 +159,26 @@ static void schedule_ll_clients_reschedule(struct ll_schedule_data *sch)
{
struct list_item *wlist;
struct list_item *tlist;
struct task *task, *task_take;
struct task *task;
uint64_t next_tick = UINT64_MAX;

/* rearm only if there is work to do */
if (atomic_read(&sch->domain->total_num_tasks)) {
struct task *task_take_dbg = NULL;
Copy link
Contributor

@keyonjie keyonjie Mar 23, 2021

Choose a reason for hiding this comment

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

Is this moving from the line 162 needed or we can keep it in line 162 (with NULL initialization added)? If the moving needed, so as other items like wlist/tlist/task?

Copy link
Collaborator Author

@marc-hb marc-hb Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The moving is not required to fix the warning but it's a good opportunity. Why have variables with a wider scope than needed? More complexity, confusion and more chances to leave variables unitialized or wrongly initialized, to shadow, harder to const, easier to misuse, etc.

C99 even relaxed the obsolete requirement for variables to be at the top of the block, C is like any other more modern language now. I digress.

Copy link
Member

@lgirdwood lgirdwood Mar 23, 2021

Choose a reason for hiding this comment

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

@marc-hb the descision for variable scope at the start of the function was based on the MISRA C safety critical requirments. https://en.wikipedia.org/wiki/MISRA_C. This is needed for use in motor vehicles.
Any MISRA C complience change here would need to be done by the TSC.

Copy link
Collaborator Author

@marc-hb marc-hb Mar 23, 2021

Choose a reason for hiding this comment

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

@lgirdwood I have done some research and asked my favorite MISRA expert and there does not seem to be any MISRA guideline that this PR violates. Can you be more specific?

On the contrary, there are many security guidelines that this PR helps with:
https://wiki.sei.cmu.edu/confluence/display/c/DCL19-C.+Minimize+the+scope+of+variables+and+functions

Variables and functions should be declared in the minimum scope from which all references to the identifier are still possible.

It find it relatively obvious that smaller scopes, lower uninitialized risk and (when possible) more const/immutability help with safety.

Copy link
Member

@lgirdwood lgirdwood Mar 24, 2021

Choose a reason for hiding this comment

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

@marc-hb I also like limited scope, but we made a descision to follow MISRA C (and this was our understanding at the time). This can be a topic at next TSC. Can you update.

Copy link
Contributor

@keyonjie keyonjie Mar 26, 2021

Choose a reason for hiding this comment

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

@cujomalainey It's actually C99 recommended while breaking C90 per my understanding.

@marc-hb basically I agree with what you change here but as stated in my first comment, since we use C90 style in all other places the single moving of 'task_take' here looks somewhat odd to me.

Copy link
Contributor

@cujomalainey cujomalainey Mar 26, 2021

Choose a reason for hiding this comment

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

I knew it broke a C standard, thanks for checking :)

Copy link
Collaborator Author

@marc-hb marc-hb Mar 29, 2021

Choose a reason for hiding this comment

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

it's actually C99 recommended while breaking C90 per my understanding

No, you're confusing:

main()
{
  printf("");
  int a = 0;
}

with:

main()
{
  printf("");
  {
     int a = 0;
  }
}

Only the former fails to compile with:

gcc -ansi -pedantic
warning: ISO C90 forbids mixing declarations and code [-Wdeclaration-after-statement]

The latter compiles just fine. Variables at the start of a block have been possible since forever.

Copy link
Contributor

@ujfalusi ujfalusi Mar 29, 2021

Choose a reason for hiding this comment

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

All of the variable declarations at function level used only within the if (atomic_read(&sch->domain->total_num_tasks)) {}

Moving one declaration under to limit the scope does not help readability (it might even make it worst by implying the the variables at top are used elsewhere - they don't) and when it comes to limiting the scope it is not much effective since the only non scope thing is the call:
platform_shared_commit(sch->domain, sizeof(*sch->domain));
at the end.

I time to time prefer to declare locally in a function to limit the scope and in hope for better readability, in this case I don't see why it would help.

Copy link
Collaborator Author

@marc-hb marc-hb Mar 29, 2021

Choose a reason for hiding this comment

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

Thanks @ujfalusi , that makes sense I will keep everything at the top.

/* traverse to set timer according to the earliest task */
list_for_item_safe(wlist, tlist, &sch->tasks) {
task = container_of(wlist, struct task, list);

/* update to use the earlier tick */
if (task->start < next_tick) {
next_tick = task->start;
task_take = task;
task_take_dbg = task;
}
}

tr_dbg(&ll_tr, "schedule_ll_clients_reschedule next_tick %u task_take %p",
(unsigned int)next_tick, task_take);
tr_dbg(&ll_tr,
"schedule_ll_clients_reschedule next_tick %u task_take %p",
(unsigned int)next_tick, task_take_dbg);
domain_set(sch->domain, next_tick);
}

Expand Down
3 changes: 2 additions & 1 deletion src/trace/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ config TRACEV
depends on TRACE
default n
help
Enabling verbose traces.
Enabling verbose traces. tr_dbg() statements depend on this at
compile-time and on run-time filtering below too.

config TRACEE
bool "Trace error"
Expand Down