Skip to content
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

Jump to bottom

arch: stack trace Kconfig & dependencies cleanup #73288

Merged
merged 6 commits into from
Jun 3, 2024
Merged

arch: stack trace Kconfig & dependencies cleanup #73288

merged 6 commits into from
Jun 3, 2024

Conversation

ycsin
Copy link
Member

@ycsin ycsin commented May 25, 2024 •
edited
Loading

Overview

  • Create CONFIG_ARM64_EXCEPTION_STACK_TRACE that depends on CONFIG_ARM64_ENABLE_FRAME_POINTER
  • Move x86 specific Kconfig of CONFIG_EXCEPTION_STACK_TRACE to CONFIG_X86_EXCEPTION_STACK_TRACE
  • CONFIG_EXCEPTION_STACK_TRACE now depends on architecture-specific EXCEPTION_STACK_TRACE Kconfig to be enabled
  • Compile stack trace implementation only when user select CONFIG_EXCEPTION_STACK_TRACE

Current

RISC-V / X86

Screenshot 2024-05-25 at 6 08 12 PM

ARM64

Screenshot 2024-05-25 at 6 08 57 PM

This PR (all supported archs)

Screenshot 2024-05-25 at 5 50 31 PM

@ycsin ycsin force-pushed the pr/exception_stack_trace_cleanup branch from c9da9ce to 8e25c72 Compare May 25, 2024 09:43
@ycsin ycsin marked this pull request as ready for review May 25, 2024 10:10
@zephyrbot zephyrbot added area: X86 x86 Architecture (32-bit) area: RISCV RISCV Architecture (32-bit & 64-bit) area: Debugging area: Architectures area: ARM64 ARM (64-bit) Architecture labels May 25, 2024
@ycsin ycsin requested review from cfriedt and removed request for najumon1980 May 25, 2024 10:11
@ycsin ycsin force-pushed the pr/exception_stack_trace_cleanup branch from 8e25c72 to 9168c66 Compare May 25, 2024 17:15
@ycsin ycsin marked this pull request as draft May 25, 2024 17:18
@ycsin ycsin force-pushed the pr/exception_stack_trace_cleanup branch from 9168c66 to d9f017a Compare May 25, 2024 17:57
@ycsin ycsin mentioned this pull request May 25, 2024
Merged
@ycsin ycsin marked this pull request as ready for review May 27, 2024 15:50
@dcpleung
Copy link
Member

Hm... I think you can keep the arch specific kconfig without renaming them, or remove the arch specific ones entirely.

Also, might be a good idea to add a CONFIG_ARCH_HAS_EXCEPTION_STACK_TRACE so CONFIG_EXCEPTION_STACK_TRACE can depend on that, which eliminates the need to update the depend line in the future for other archs.

@ycsin
Copy link
Member Author

ycsin commented May 28, 2024

Hm... I think you can keep the arch specific kconfig without renaming them, or remove the arch specific ones entirely.

Also, might be a good idea to add a CONFIG_ARCH_HAS_EXCEPTION_STACK_TRACE so CONFIG_EXCEPTION_STACK_TRACE can depend on that, which eliminates the need to update the depend line in the future for other archs.

Sounds like you kinda prefer #72863? But,

Hm... usually CONFIG_ARCH_HAS_* are enabled by select, so that they cannot be "disabled" by later kconfig entries.

select would force CONFIG_ARCH_HAS_EXCEPTION_STACK_TRACE to be on even if the dependencies are not met. In that case, where should I place the dependencies in your opinion?

Maybe a top-down approach?

  • User enables CONFIG_EXCEPTION_STACK_TRACE
  • CONFIG_X86_EXCEPTION_STACK_TRACE default y if CONFIG_EXCEPTION_STACK_TRACE
  • CONFIG_X86_EXCEPTION_STACK_TRACE select all its dependencies?

@ycsin
Copy link
Member Author

ycsin commented May 28, 2024 •
edited
Loading

Moved back to draft for now, opened #73376 specifically for the cleanup

@ycsin ycsin marked this pull request as draft May 28, 2024 02:55
@dcpleung
Copy link
Member

Hm... usually CONFIG_ARCH_HAS_* are enabled by select, so that they cannot be "disabled" by later kconfig entries.

select would force CONFIG_ARCH_HAS_EXCEPTION_STACK_TRACE to be on even if the dependencies are not met. In that case, where should I place the dependencies in your opinion?

Maybe a top-down approach?

  • User enables CONFIG_EXCEPTION_STACK_TRACE
  • CONFIG_X86_EXCEPTION_STACK_TRACE default y if CONFIG_EXCEPTION_STACK_TRACE
  • CONFIG_X86_EXCEPTION_STACK_TRACE select all its dependencies?

I think that would be a better approach. A developer wants to enable such feature, so any dependencies should be pulled in. Or else one has to do dependency hunting through kconfig.

andyross
andyross previously approved these changes May 29, 2024
View reviewed changes
@ycsin ycsin force-pushed the pr/exception_stack_trace_cleanup branch from a278c27 to b63973b Compare May 30, 2024 06:53
@ycsin
Copy link
Member Author

ycsin commented May 30, 2024

Hm... usually CONFIG_ARCH_HAS_* are enabled by select, so that they cannot be "disabled" by later kconfig entries.

select would force CONFIG_ARCH_HAS_EXCEPTION_STACK_TRACE to be on even if the dependencies are not met. In that case, where should I place the dependencies in your opinion?
Maybe a top-down approach?

  • User enables CONFIG_EXCEPTION_STACK_TRACE
  • CONFIG_X86_EXCEPTION_STACK_TRACE default y if CONFIG_EXCEPTION_STACK_TRACE
  • CONFIG_X86_EXCEPTION_STACK_TRACE select all its dependencies?

I think that would be a better approach. A developer wants to enable such feature, so any dependencies should be pulled in. Or else one has to do dependency hunting through kconfig.

There are some dependencies that can't be simply selected without getting more involved, like NO_OPTIMIZATIONS & !OMIT_FRAME_POINTER, so I'm going to keep this PR as-is, which will at least make the dependencies consistent across archs.

@ycsin ycsin force-pushed the pr/exception_stack_trace_cleanup branch from b63973b to e13d3c3 Compare May 30, 2024 07:07
@ycsin ycsin marked this pull request as ready for review May 30, 2024 09:55
@ycsin ycsin mentioned this pull request May 30, 2024
Closed
@ycsin ycsin requested a review from andyross May 30, 2024 09:56
carlocaione
carlocaione previously approved these changes May 30, 2024
View reviewed changes
ycsin added 6 commits May 30, 2024 18:03
@ycsin
debug: remove PRINTK from EXCEPTION_STACK_TRACE
2a99472 
The `PRINTK` was required for `EXCEPTION_STACK_TRACE` because
it's initial implementation for x86 in zephyrproject-rtos#6653 uses `printk()`.
We are using `LOG_ERR()` now, so this is not required anymore.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
debug: remove DEBUG_INFO from EXCEPTION_STACK_TRACE
f748de2 
The `DEBUG_INFO` in the `EXCEPTION_STACK_TRACE` is only
required by x86. Move that to `X86_EXCEPTION_STACK_TRACE`
instead.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
debug: remove !OMIT_FRAME_POINTER from EXCEPTION_STACK_TRACE
5b022d3 
Not all stack trace implementation requires frame pointer, move
that dependency to architecture Kconfig.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
arch: x86: select DEBUG_INFO in X86_EXCEPTION_STACK_TRACE
04309e0 
Select `DEBUG_INFO` instead of depending on it.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
arch: arm64: create ARM64_EXCEPTION_STACK_TRACE
3154f96 
Currently, the stack trace in ARM64 implementation depends on
frame pointer Kconfigs combo to be enabled. Create a dedicated
Kconfig for that instead, so that it is consistent with x86 and
riscv, and update the source accordingly.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
debug: EXCEPTION_STACK_TRACE should depend on arch Kconfigs
fbe2021 
Fix the dependencies of `CONFIG_EXCEPTION_STACK_TRACE`:
- Architecture-specific Kconfig, i.e.
  `X86_EXCEPTION_STACK_TRACE`, will be enabled automatically
  when all the dependencies are met.
- `EXCEPTION_STACK_TRACE` depends on architecture-specific
  Kconfig to be enabled.
- The stack trace implementations should be compiled only if
  user enables `CONFIG_EXCEPTION_STACK_TRACE`.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin ycsin force-pushed the pr/exception_stack_trace_cleanup branch from e13d3c3 to fbe2021 Compare May 30, 2024 10:07
@ycsin ycsin requested a review from carlocaione May 30, 2024 10:08
andyross
andyross approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Still +1

@carlescufi carlescufi merged commit 02770ad into zephyrproject-rtos:main Jun 3, 2024
28 checks passed
@ycsin ycsin deleted the pr/exception_stack_trace_cleanup branch June 3, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfriedt cfriedt cfriedt approved these changes

@carlocaione carlocaione carlocaione approved these changes

@andyross andyross andyross approved these changes

@nashif nashif nashif approved these changes

@ceolin ceolin Awaiting requested review from ceolin

@dcpleung dcpleung Awaiting requested review from dcpleung

@edersondisouza edersondisouza Awaiting requested review from edersondisouza

@fkokosinski fkokosinski Awaiting requested review from fkokosinski

@katsuster katsuster Awaiting requested review from katsuster

@kgugala kgugala Awaiting requested review from kgugala

@laurenmurphyx64 laurenmurphyx64 Awaiting requested review from laurenmurphyx64

@mgielda mgielda Awaiting requested review from mgielda

@npitre npitre Awaiting requested review from npitre

@povergoing povergoing Awaiting requested review from povergoing

@tgorochowik tgorochowik Awaiting requested review from tgorochowik

Assignees

@nashif nashif

Labels
area: Architectures area: ARM64 ARM (64-bit) Architecture area: Debugging area: RISCV RISCV Architecture (32-bit & 64-bit) area: X86 x86 Architecture (32-bit)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ycsin @nashif @dcpleung @andyross @carlocaione @cfriedt @carlescufi @fkokosinski @zephyrbot