K_THREAD_DEFINE() uses const in a wrong way

[holgerschurig]

Describe the bug
Hi, I have the following code:

void storage_thread(void *unused, void *unused2, void *unused3)
{
   ...
}

K_THREAD_DEFINE(storage_save,
                128,                     // stack size
                storage_thread,
                NULL, NULL, NULL,        // arguments
                13,                      // priority
                K_USER,                  // thread options
                0);                      // scheduling delay

which compiles and works fine. However, there's still a subtle problem hidden, misuse of const.

See the warning "storage_save' declared with a const-qualified typedef; results in the type being 'struct k_thread *const' instead of 'const struct k_thread" in this output:

$ clang-tidy-11 -p build storage.c 
1716 warnings and 2 errors generated.
Error while processing /REDACTED/storage.c.
warning: macro replacement list should be enclosed in parentheses [bugprone-macro-parentheses]
error: unknown argument: '-fno-freestanding' [clang-diagnostic-error]
error: unknown argument: '-fno-reorder-functions' [clang-diagnostic-error]
/home/holger/d/v73/storage.c:136:17: warning: 'storage_save' declared with a const-qualified typedef; results in the type being 'struct k_thread *const' instead of 'const struct k_thread *' [misc-misplaced-const]
K_THREAD_DEFINE(storage_save,
                ^
../zephyr/include/kernel.h:355:26: note: typedef declared here
typedef struct k_thread *k_tid_t;
                         ^
Suppressed 1714 warnings (1714 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Found compiler error(s).

See also
https://clang.llvm.org/extra/clang-tidy/checks/misc-misplaced-const.html

[pabigot]

AFAICT the intent of:

  const k_tid_t name = (k_tid_t)&_k_thread_obj_##name

is to declare struct_k_thread * const name, so this is doing what it's supposed to.

I do wonder why the generated name declaration is not static: seems to contaminate the global namespace.

[andrewboie]

I do wonder why the generated name declaration is not static: seems to contaminate the global namespace.

This is intended. Forcing const k_tid_t name to have static scope would preclude anyone from referencing the thread externally. The actual stack and thread objects generated could be static though.

About the original report:

The declared struct k_thread is not and must not be const. It's the thread control block. const struct k_thread * would imply we are declaring a pointer to a read-only thread struct which would be wrong.

The declared k_tid_t is intended to be a read-only pointer to the thread object. This must end up in rodata or a lot of stuff will fall over (user thread wouldn't be able to read it). In practice this is working as expected unless I'm missing something.

There's no way this is a "medium" severity bug. I'm not even sure this is a bug. I think your lint tool is too aggressive.

[nashif]

not a bug.