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: deprecate z_arch_esf_t with struct arch_esf, introduce an arch-agnostic exception.h for it #73593

Merged
merged 9 commits into from
Jun 4, 2024
Merged

arch: deprecate z_arch_esf_t with struct arch_esf, introduce an arch-agnostic exception.h for it #73593

merged 9 commits into from
Jun 4, 2024

Conversation

ycsin
Copy link
Member

@ycsin ycsin commented May 31, 2024 •
edited
Loading

@ycsin ycsin requested review from andyross and Thalley May 31, 2024 16:33
@ycsin ycsin marked this pull request as ready for review May 31, 2024 16:42
@zephyrbot zephyrbot added area: Kernel area: Architectures area: SPARC SPARC Architecture area: NIOS2 NIOS2 Architecture area: X86 x86 Architecture (32-bit) area: native port Host native arch port (native_sim) labels May 31, 2024
@ycsin ycsin force-pushed the pr/arch_esf branch 2 times, most recently from 4c12eaf to fc67715 Compare June 1, 2024 02:28
@ycsin ycsin mentioned this pull request Jun 1, 2024
Merged
@ycsin ycsin force-pushed the pr/arch_esf branch 3 times, most recently from ad90a8d to 04b935a Compare June 2, 2024 07:32
@ycsin ycsin requested review from npitre and andyross June 4, 2024 04:54
@ycsin
Copy link
Member Author

ycsin commented Jun 4, 2024 •
edited
Loading

Rebased & updated the git revision of sof to point at the hash after the merge of zephyrproject-rtos/sof#46

aescolar
aescolar previously requested changes Jun 4, 2024
View reviewed changes
doc/releases/migration-guide-3.7.rst Show resolved Hide resolved
include/zephyr/arch/exception.h Outdated Show resolved Hide resolved
ycsin added 9 commits June 4, 2024 16:26
@ycsin
arch: Create include/zephyr/arch/exception.h
036b161 
Create `zephyr/include/zephyr/arch/exception.h`, which will
redirect to the corrent architecture-specific exception header
based on Kconfig.

Some of the architectures define their esf struct in
architecture-specific `arch.h`, refactor them out into a
separate `exception.h`.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
include: include exception.h in fatal.h
7eb27ab 
`fatal.h` has 2 functions that use the `z_arch_esf_t` type.

Include `exception.h`, which should have the `z_arch_esf_t`
defined.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
arch: rename all esf struct to struct arch_esf
0e4ab72 
Rename every architecture's esf struct to `struct esf`.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
arch: define struct arch_esf and deprecate z_arch_esf_t
0761307 
Make `struct arch_esf` compulsory for all architectures by
declaring it in the `arch_interface.h` header.

After this commit, the named struct `z_arch_esf_t` is only used
internally to generate offsets, and is slated to be removed
from the `arch_interface.h` header in the future.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
arch: remove the use of z_arch_esf_t completely from internal
1f41a98 
Created `GEN_OFFSET_STRUCT` & `GEN_NAMED_OFFSET_STRUCT` that
works for `struct`, and remove the use of `z_arch_esf_t`
completely.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
doc: releases: update migration guide for the struct arch_esf
7989211 
Add a note about the introduction of `struct arch_esf` and the
deprecation of `z_arch_esf_t`.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
manifest: update sof git revision
123924e 
Update the git revision of the `sof` module.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
include: optimize the order of includes for x86
47ca0bb 
x86 32bit defines `CONFIG_X86` while its 64bit counterpart
defines an additional `CONFIG_X86_64`, by reordering the
include order we can make it look a bit cleaner.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
include: stop breaking the compilation of OOT architectures
0382366 
Zephyr support out-of-tree architectures so we shouldn't
throw error for archs not listed here.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin ycsin requested a review from aescolar June 4, 2024 08:27
@aescolar aescolar dismissed their stale review June 4, 2024 08:35

Addressed

@ycsin ycsin added this to the v3.7.0 milestone Jun 4, 2024
@carlescufi carlescufi removed the Breaking API Change Breaking changes to stable, public APIs label Jun 4, 2024
@ycsin
Copy link
Member Author

ycsin commented Jun 4, 2024

ping @andyross could you please take another look? Thanks

@MaureenHelm MaureenHelm merged commit a2b0c2c into zephyrproject-rtos:main Jun 4, 2024
34 checks passed
@ycsin ycsin deleted the pr/arch_esf branch June 5, 2024 00:52
@Thalley
Copy link
Collaborator

Thalley commented Jun 5, 2024

Thanks for the quick fix @ycsin

One thing I can see today is that now zephyr/include/zephyr/arch/arm/cortex_m/exception.h may be included before sys_define_gpr_with_alias has been defined.

This effectively means that the new generic exception.h header cannot be included for ARM before zephyr/include/zephyr/arch/arm/arch.h has been included, as zephyr/include/zephyr/arch/arm/arch.h defines the sys_define_gpr_with_alias macro.

I almost expect that if we fix the above, yet another include issue will appear. These type of include issues will continue to appear unless verified by a tool (attempting to fix or verify manually will be nearly impossible).

@Thalley
Copy link
Collaborator

Thalley commented Jun 11, 2024

Thanks for the quick fix @ycsin

One thing I can see today is that now zephyr/include/zephyr/arch/arm/cortex_m/exception.h may be included before sys_define_gpr_with_alias has been defined.

This effectively means that the new generic exception.h header cannot be included for ARM before zephyr/include/zephyr/arch/arm/arch.h has been included, as zephyr/include/zephyr/arch/arm/arch.h defines the sys_define_gpr_with_alias macro.

I almost expect that if we fix the above, yet another include issue will appear. These type of include issues will continue to appear unless verified by a tool (attempting to fix or verify manually will be nearly impossible).

@ycsin are you looking into this? Otherwise I'll go ahead and create another issue :)

If you are not already, you may want to use a tool like the clang includer-cleaner or clangd to catch future issues.

@ycsin
Copy link
Member Author

ycsin commented Jun 11, 2024

@ycsin are you looking into this? Otherwise I'll go ahead and create another issue :)

Not looking into this, please feel free to create another issue

@Thalley
Copy link
Collaborator

Thalley commented Jun 11, 2024

@ycsin are you looking into this? Otherwise I'll go ahead and create another issue :)

Not looking into this, please feel free to create another issue

The fix for this was quite simple, so decided to take a look at fixing it myself: #74106

@ycsin ycsin mentioned this pull request Sep 5, 2024
Closed
@carlescufi carlescufi mentioned this pull request Sep 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nashif nashif nashif approved these changes

@andyross andyross andyross approved these changes

@npitre npitre npitre approved these changes

@ceolin ceolin Awaiting requested review from ceolin

@cfriedt cfriedt Awaiting requested review from cfriedt

@dcpleung dcpleung Awaiting requested review from dcpleung

@edersondisouza edersondisouza Awaiting requested review from edersondisouza

@julius-barendt julius-barendt Awaiting requested review from julius-barendt

@laurenmurphyx64 laurenmurphyx64 Awaiting requested review from laurenmurphyx64

@najumon1980 najumon1980 Awaiting requested review from najumon1980

@peter-mitsis peter-mitsis Awaiting requested review from peter-mitsis

@TaiJuWu TaiJuWu Awaiting requested review from TaiJuWu

@Thalley Thalley Awaiting requested review from Thalley

@aescolar aescolar Awaiting requested review from aescolar

Assignees

@andyross andyross

Labels
area: Architectures area: Kernel area: native port Host native arch port (native_sim) area: NIOS2 NIOS2 Architecture area: SPARC SPARC Architecture area: X86 x86 Architecture (32-bit) manifest manifest-sof
Projects
None yet
Milestone
v3.7.0
10 participants
@ycsin @Thalley @nashif @zephyrbot @andyross @npitre @aescolar @edersondisouza @MaureenHelm @carlescufi