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.

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

Kernel: Includes: fatal.h does not include a header file that defines z_arch_esf_t #73454

Closed
Thalley opened this issue May 29, 2024 · 4 comments · Fixed by #73593
Closed

Kernel: Includes: fatal.h does not include a header file that defines z_arch_esf_t #73454

Thalley opened this issue May 29, 2024 · 4 comments · Fixed by #73593
Assignees
Labels
area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@Thalley
Copy link
Collaborator

Thalley commented May 29, 2024

Describe the bug
fatal.h has 2 functions that use the z_arch_esf_t type:

void k_sys_fatal_error_handler(unsigned int reason, const z_arch_esf_t *esf);
void z_fatal_error(unsigned int reason, const z_arch_esf_t *esf);

but the type is not defined by any header files in the file.

The type seems to rely on the specific architecture, and have multiple different definitions:
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/arc/v2/exception.h#L23
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/arm/cortex_a_r/exception.h#L78
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/arm/cortex_m/exception.h#L122
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/posix/arch.h#L45

and several others.
Due to the different architectures, and due to the type not even being defined in similarly named files, it seems non-trivial to find the right header file to include.

The main issue about this bug is that it will cause issues with certain tools such as clangd that cannot find the right includes for static analysis.

Ideally fatal.h should include the files that it uses, and not rely on some specific header files being included before it.

To Reproduce
N/A

Expected behavior
fatal.h should include a header file that defines the types that it use.
Alternatively, fatal.h should not depend on any specific types for these functions.

Impact
Annoynace

Logs and console output
From clangd:

Unknown type name 'z_arch_esf_t'

Environment (please complete the following information):

  • Commit SHA or Version used: c140053

Additional context
N/A

@Thalley Thalley added bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug area: Kernel labels May 29, 2024
@andyross
Copy link
Contributor

Agreed. The fatal.h APIs are very old and really not well standardized. There's a strong need for a rework and normalization. @ycsin just submitted a PR in this space (doing kconfig cleanup, not C, but both need attention) and might be interested?

@ycsin
Copy link
Member

ycsin commented May 30, 2024

Sounds more like enhancements than bug - we probably just need to create something like include/zephyr/exception.h and include arch-specific exception.h based on precompiler macros from there, similar to how the zephyr/include/zephyr/arch/cpu.h is done?

#if defined(CONFIG_X86)
#include <zephyr/arch/x86/exception.h>
#elif defined(CONFIG_ARM64)
#include <zephyr/arch/arm64/exception.h>
#elif defined(CONFIG_ARM)
#include <zephyr/arch/arm/exception.h>
#elif defined(CONFIG_ARC)
#include <zephyr/arch/arc/exception.h>
#elif defined(CONFIG_NIOS2)
#include <zephyr/arch/nios2/exception.h>
#elif defined(CONFIG_RISCV)
#include <zephyr/arch/riscv/exception.h>
#elif defined(CONFIG_XTENSA)
#include <zephyr/arch/xtensa/exception.h>
#elif defined(CONFIG_MIPS)
#include <zephyr/arch/mips/exception.h>
#elif defined(CONFIG_ARCH_POSIX)
#include <zephyr/arch/posix/exception.h>
#elif defined(CONFIG_SPARC)
#include <zephyr/arch/sparc/exception.h>
#else
#error "Unknown Architecture"
#endif

@Thalley
Copy link
Collaborator Author

Thalley commented May 30, 2024

Sounds more like enhancements than bug - we probably just need to create something like include/zephyr/exception.h and include arch-specific exception.h based on precompiler macros from there, similar to how the zephyr/include/zephyr/arch/cpu.h is done?

Probably.
It's not completely ideal, as ideally you'd directly include the header file that defines the things you use (rather than a proxy), but may be the best option

@andyross
Copy link
Contributor

Yeah, exactly. There should be a single API exposed and documented in arch_interface.h which everyone implements (whether by that particular #if chain or by putting a known header name in the arch/ tree or whatever seems like an implementation detail). Just putting the esf in there as a named struct type would be good for a start, but almost all platforms have some level of introspection support on it for stack traces, etc... and it would be nice to unify those with the same API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
4 participants