Skip to content

arch: x86: fatal: If possible, print thread name in crash dump #14155

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

Merged
merged 2 commits into from
Apr 24, 2019
Merged

arch: x86: fatal: If possible, print thread name in crash dump #14155

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
799c9cd
arch: x86: fatal: If possible, print thread name in crash dump
pfalcon Mar 7, 2019
40a1f7e
testsuite: Include thread name in crash dump for tests
pfalcon Mar 13, 2019
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
13 changes: 12 additions & 1 deletion arch/x86/core/fatal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ static void unwind_stack(u32_t base_ptr, u16_t cs)
FUNC_NORETURN void z_NanoFatalErrorHandler(unsigned int reason,
const NANO_ESF *pEsf)
{
#ifdef CONFIG_THREAD_NAME
const char *thread_name = k_thread_name_get(k_current_get());
#endif

LOG_PANIC();

z_debug_fatal_hook(pEsf);
Expand Down Expand Up @@ -184,7 +188,11 @@ FUNC_NORETURN void z_NanoFatalErrorHandler(unsigned int reason,
break;
}

printk("Current thread ID = %p\n"
printk("Current thread ID = %p"
#ifdef CONFIG_THREAD_NAME
" (%s)"
#endif
"\n"
"eax: 0x%08x, ebx: 0x%08x, ecx: 0x%08x, edx: 0x%08x\n"
"esi: 0x%08x, edi: 0x%08x, ebp: 0x%08x, esp: 0x%08x\n"
"eflags: 0x%08x cs: 0x%04x\n"
Expand All @@ -193,6 +201,9 @@ FUNC_NORETURN void z_NanoFatalErrorHandler(unsigned int reason,
#endif
"eip: 0x%08x\n",
k_current_get(),
#ifdef CONFIG_THREAD_NAME
thread_name ? thread_name : "unknown",
#endif
pEsf->eax, pEsf->ebx, pEsf->ecx, pEsf->edx,
pEsf->esi, pEsf->edi, pEsf->ebp, pEsf->esp,
pEsf->eflags, pEsf->cs & 0xFFFFU, pEsf->eip);
Expand Down
3 changes: 3 additions & 0 deletions subsys/testsuite/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ source "subsys/testsuite/ztest/Kconfig"

config TEST
bool "Mark project as a test"
# For tests, store thread names in binary and dump them on crash to
Copy link
Member

@ioannisg ioannisg Mar 14, 2019

Choose a reason for hiding this comment

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

This is not really storing the thread names, more like it allows storing the names. Still, the user needs to do this, AFAICS.

Copy link
Contributor Author

@pfalcon pfalcon Mar 14, 2019

Choose a reason for hiding this comment

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

Then we would need to backtrack and consider why you asked me to add this unrelated change in this PR. For example if "Still, the user needs to do this", then they can also enable CONFIG_THREAD_NAME themselves - if they need it. And not enable, if they don't need it. While you for some reason asked me to force it upon them unsuspecting users. To make them less unsuspecting, I added a comment for a reason why such option would be force-enabled, but you don't even agree with that reason.

Copy link
Member

@ioannisg ioannisg Mar 14, 2019

Choose a reason for hiding this comment

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

Hey,so with THREAD_NAME, the test developers can simply call k_thread_name_set, to set names to the threads they wish. This patch saves us from the need of having CONFIG_THREAD_NAME=y in the prj.conf of each test.

So there is some value in adding the selection here, IMHO :) I just wanted to stress that selecting THREAD_NAME does not automatically add names to the threads :) It's just a style thing, anyways.

Copy link
Contributor Author

@pfalcon pfalcon Mar 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hey,so with THREAD_NAME, the test developers can simply call k_thread_name_set

They should call k_thread_name_set() regardless of CONFIG_THREAD_NAME. Or not call, if they choose so. (E.g. if they want to complicate diagnostics.)

I just wanted to stress that selecting THREAD_NAME does not automatically add names to the threads :)

Yep, but we can't include whole API reference/development primer in one comment. So, the comment emphasizes that for the standard system threads, if crash happens in them, the crash dump will include the thread name (because system threads of course call k_thread_name_set(), as that's the best practice). I'd assume that some 10% of tests would create a new user thread, while most will just use existing main thread and/or sys workq. And the idea behind the original patch was to allow to quickly diagnose stack overflow in one of system threads, because that's what happens regularly.

But again, we can't include all those details, if's, and but's in one comment. Hopefully, it'll give basic info and motivate interested folks to read help for CONFIG_THREAD_NAME.

Copy link
Member

@nashif nashif Apr 17, 2019

Choose a reason for hiding this comment

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

can you please put the comment the select on top of the help, this looks confusing and the comment seem to be part of the help.

Copy link
Contributor Author

@pfalcon pfalcon Apr 17, 2019

Choose a reason for hiding this comment

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

can you please put the comment the select on top of the help

Done.

# ease debugging.
select THREAD_NAME
help
Mark a project or an application as a test. This will enable a few
test defaults.
Expand Down