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

pfalcon · 2019-03-07T14:32:52Z

It's relatively hard to figure out what thread a crash happens in
from the crash dump. E.g, it's usually not immediately possible to
find it out from linker map due to the fact that static symbols are
not there (https://sourceware.org/bugzilla/show_bug.cgi?id=16566).

So, try to do it as easy if possible, by just printing thread name
in a dump, if thread names are enabled at all.

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

pfalcon · 2019-03-07T14:35:12Z

This comes from looking into issues like #13897 - there should be a way to get a crash and immediately know what's wrong and have idea where to look for resolving it.

E.g., with issue in that ticket, I get:

[00:00:00.120,000] <inf> net_config: IPv6 address: fe80::200:5eff:fe00:536f
uart:~$ ***** Stack Check Fail! *****
Current thread ID = 0x004013a0 (net_mgmt)
eax: 0x0040a42c, ebx: 0x00000247, ecx: 0x00000000, edx: 0x004021c4
esi: 0x00000247, edi: 0x00402140, ebp: 0x0040a574, esp: 0x0040a570
eflags: 0x00000007 cs: 0x0008

The issue is that I get that only if I enable additional options. But again, there should be such a way.

pfalcon · 2019-03-07T14:41:23Z

Risks: well, printing a thread name can lead to a fault itself, if things are really bad in memory. Well, it's controlled by CONFIG_THREAD_NAME, and it's not even the default (but I may imagine later there may be talk about enabling it for a good share of samples for example).

pfalcon · 2019-03-07T14:47:39Z

Alternatives: I personally would rather prefer to un-static thread data definitions, to be able to look up thread id's in a map file. These 2 things are of course not mutually exclusive, but if I were to choose only one, I'd choose to un-static things to workaround stupid gnu linker. Obviously, I don't propose that. Yet.

ioannisg

Could this be done for all architectures?

pfalcon · 2019-03-07T14:56:19Z

Could this be done for all architectures?

Not at once. Let's see if that can be done for one arch. Otherwise, yeah, arm waits for its hero to implement stack trace printing. And I don't even talk about less loved archs where e.g. newlib doesn't work too well.

pfalcon · 2019-03-07T14:57:02Z

Could this be done for all architectures?

Maybe that code should be factored out to be arch-independent for example?

codecov-io · 2019-03-07T15:17:16Z

Codecov Report

Merging #14155 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #14155   +/-   ##
=======================================
  Coverage   51.99%   51.99%           
=======================================
  Files         308      308           
  Lines       45572    45572           
  Branches    10553    10553           
=======================================
  Hits        23695    23695           
  Misses      17069    17069           
  Partials     4808     4808

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbe6fa0...c7edc49. Read the comment docs.

andrewboie

patch looks fine to me.

andrewboie · 2019-03-07T18:25:20Z

recheck

ioannisg

Could we select THREAD_NAME in the test suites, @andrewboie ?
Then this would all work for the existing target tests.

andrewboie · 2019-03-07T19:37:54Z

Could we select THREAD_NAME in the test suites, @andrewboie ?
Then this would all work for the existing target tests.

Yes, I think it's reasonable for CONFIG_TEST to select this.

ioannisg · 2019-03-08T08:23:36Z

Could we select THREAD_NAME in the test suites, @andrewboie ?
Then this would all work for the existing target tests.

Yes, I think it's reasonable for CONFIG_TEST to select this.

Could @pfalcon do this in the context of this PR?

pfalcon · 2019-03-13T19:58:54Z

Could @pfalcon do this in the context of this PR?

@ioannisg, Done.

pfalcon · 2019-03-14T10:34:29Z

@andyross: FYI, there's regular flakiness in qemu_x86_64 tests. I had 2 cases in row with #14490 , and this failed too: https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/37140/5/tests (IIRC, #14490 cases also involved tests/kernel/fifo/fifo_usage).

ioannisg · 2019-03-14T10:52:03Z

subsys/testsuite/Kconfig

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

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.

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.

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.

ioannisg

Looks fine to me. Left a minor comment regarding the added help text

pfalcon · 2019-03-15T18:03:44Z

@galak, this is ready to merge.

andrewboie · 2019-03-15T18:47:31Z

This patch is a useful enhancement, but the tree is locked down for bug fixes and documentation only. I think this can go in once the merge window opens again.

nashif · 2019-04-17T14:48:07Z

subsys/testsuite/Kconfig

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.

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

Done.

It's relatively hard to figure out what thread a crash happens in from the crash dump. E.g, it's usually not immediately possible to find it out from linker map due to the fact that static symbols are not there (https://sourceware.org/bugzilla/show_bug.cgi?id=16566). So, try to do it as easy if possible, by just printing thread name in a dump, if thread names are enabled at all. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

By selecting CONFIG_THREAD_NAME. It makes sense to have this enabled by default for tests, to easy debugging in a case of crash. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

pfalcon · 2019-04-24T11:20:15Z

@nashif, @carlescufi, @andrewboie: There was CI timeout on this PR, now cleared. Please merged.

pfalcon added area: Kernel area: Debugging labels Mar 7, 2019

pfalcon requested review from andrewboie and nashif March 7, 2019 14:32

pfalcon mentioned this pull request Mar 7, 2019

CONFIG_LOG_IMMEDIATE leads to unobvious faults in unrelated rotines due to stack overflow #13897

Closed

ioannisg reviewed Mar 7, 2019

View reviewed changes

pfalcon force-pushed the crash-print-thread-name branch from ae63b24 to 2d010ef Compare March 7, 2019 15:43

andrewboie approved these changes Mar 7, 2019

View reviewed changes

ioannisg reviewed Mar 7, 2019

View reviewed changes

pfalcon force-pushed the crash-print-thread-name branch from 2d010ef to c7edc49 Compare March 13, 2019 19:58

pfalcon requested a review from galak March 13, 2019 19:59

ioannisg reviewed Mar 14, 2019

View reviewed changes

ioannisg approved these changes Mar 14, 2019

View reviewed changes

andrewboie added this to the v1.15.0 milestone Mar 15, 2019

ioannisg mentioned this pull request Apr 11, 2019

kernel: Print thread name in fatal error handler #15344

Closed

nashif reviewed Apr 17, 2019

View reviewed changes

pfalcon force-pushed the crash-print-thread-name branch from c7edc49 to 78822a6 Compare April 17, 2019 15:08

pfalcon added 2 commits April 24, 2019 10:22

testsuite: Include thread name in crash dump for tests

40a1f7e

By selecting CONFIG_THREAD_NAME. It makes sense to have this enabled by default for tests, to easy debugging in a case of crash. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

pfalcon force-pushed the crash-print-thread-name branch from 78822a6 to 40a1f7e Compare April 24, 2019 07:22

andrewboie merged commit 2a798bb into zephyrproject-rtos:master Apr 24, 2019

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

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

Uh oh!

Conversation

pfalcon commented Mar 7, 2019

Uh oh!

pfalcon commented Mar 7, 2019

Uh oh!

pfalcon commented Mar 7, 2019

Uh oh!

pfalcon commented Mar 7, 2019

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Mar 7, 2019

Uh oh!

pfalcon commented Mar 7, 2019

Uh oh!

codecov-io commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andrewboie left a comment

Choose a reason for hiding this comment

Uh oh!

andrewboie commented Mar 7, 2019

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

andrewboie commented Mar 7, 2019

Uh oh!

ioannisg commented Mar 8, 2019

Uh oh!

pfalcon commented Mar 13, 2019

Uh oh!

pfalcon commented Mar 14, 2019

Uh oh!

ioannisg Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

pfalcon Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

ioannisg Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

pfalcon Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Mar 15, 2019

Uh oh!

andrewboie commented Mar 15, 2019

Uh oh!

nashif Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

pfalcon Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Apr 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Mar 7, 2019 •

edited

Loading

pfalcon Mar 14, 2019 •

edited

Loading