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

[RFC][PoC] Zephyr PreProcessor (ZPP) framework #82990

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pdgendt
Copy link
Collaborator

@pdgendt pdgendt commented Dec 14, 2024

Introduction

This is a proof-of-concept, namely the Zephyr Pre-Processor (ZPP) framework, an alternative to the syscall parsing for gathering function and struct information.

Problem description

The https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/build/parse_syscalls.py script has a few drawbacks that this PR tries to improve upon.

  • It's currently (ab)used to find __subsystem and __net_socket "tags" but this is a hidden implementation detail
  • It operates on header/source files before the C pre-processer so we can't rely on macro expansion
  • It generates more boiler plate than is needed

Proposed change

Run the C pre-processor, in the pre-build stage, on files that are part of the build and have the -DZPP compile definition (opt-in), and parse the output for annotated functions or structs.
Gather the information in a formatted (json) file that can be fed into other build steps.

Detailed RFC

In the aforementioned C pre-processor run, the compile definition -D__ZPP__ is added which enables the C23 annotations (not part of the regular build), for example:

#ifdef __ZPP__
#define __zpp(x) [[zephyr::x]]
#else
#define __zpp(...)
#endif

#define __syscall __zpp(func("syscall", __FILE__, __LINE__))
#define __subsystem __zpp(struct("__subsystem"))

The newly introduced scripts/zpp.py scans for these annotations and can output the structured data needed for the next build steps.

[
  {
    "attr": "func",
    "args": [
      "syscall",
      "WEST_TOPDIR/zephyr/include/zephyr/kernel.h",
      537
    ],
    "data": {
      "type": "int",
      "name": "k_thread_join",
      "args": "struct k_thread *thread, k_timeout_t timeout"
    }
  },
  {
    "attr": "struct",
    "args": [
      "__subsystem"
    ],
    "data": {
      "name": "gpio_driver_api"
    }
  }
]

Proposed change (Detailed)

  • Convert the __subsystem tags with ZPP (part of the PR already)
  • Convert the __net_socket tags with ZPP
  • Convert the __syscall tags with ZPP
  • Introduce __iterable_section_rom tags for structs to allow creating linker sections from source files, without the need for additional linker scripts (cleanup all common-rom.ld and common-rom.cmake files)

Dependencies

Affects downstream usage of the syscall mechanism.

Concerns and Unresolved Questions

  • Portability for different toolchains
  • Opt-in vs opt-out
  • Generated headers need include guards when running ZPP (part of the PR already)

Alternatives

Don't do this

@pdgendt pdgendt added the RFC Request For Comments: want input from the community label Dec 14, 2024
@pdgendt pdgendt force-pushed the poc-zpp branch 11 times, most recently from 5af477d to 185bcac Compare December 16, 2024 09:17
@henrikbrixandersen
Copy link
Member

That's a very interesting idea. I like it, my only concern is "Portability for different toolchains" - do you any further data points on this?

scripts/zpp.py Outdated Show resolved Hide resolved
@pdgendt
Copy link
Collaborator Author

pdgendt commented Dec 16, 2024

That's a very interesting idea. I like it, my only concern is "Portability for different toolchains" - do you any further data points on this?

It comes down to the C compiler support for the following options:

  • -E -P to only do the preprocessing step (not an issue, I think)
  • -M -MG print dependency list, as well as missing include headers (relative paths)

Documentation is scarce, I based the implementation on GCC options
Support for clang should be covered too.

scripts/zpp.py Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 18 changed files in this pull request and generated no comments.

Files not reviewed (16)
  • CMakeLists.txt: Language not supported
  • cmake/linker/ld/target.cmake: Language not supported
  • cmake/linker/linker_script_common.cmake: Language not supported
  • cmake/linker_script/common/common-rom.cmake: Language not supported
  • cmake/zpp.cmake: Language not supported
  • include/zephyr/linker/common-rom.ld: Language not supported
  • include/zephyr/linker/common-rom/common-rom-kernel-devices.ld: Language not supported
  • include/zephyr/linker/common-rom/common-rom-net.ld: Language not supported
  • include/zephyr/net/coap_service.h: Language not supported
  • include/zephyr/net/dns_sd.h: Language not supported
  • include/zephyr/net/http/service.h: Language not supported
  • include/zephyr/net/net_mgmt.h: Language not supported
  • include/zephyr/net/socket.h: Language not supported
  • include/zephyr/net/socket_service.h: Language not supported
  • include/zephyr/toolchain/common.h: Language not supported
  • subsys/net/l2/ppp/ppp_internal.h: Language not supported
@pdgendt
Copy link
Collaborator Author

pdgendt commented Dec 16, 2024

Copilot reviewed 2 out of 18 changed files in this pull request and generated no comments.

Thanks Copilot

@pdgendt pdgendt force-pushed the poc-zpp branch 2 times, most recently from 575cd12 to f0d72d0 Compare December 16, 2024 16:08
Comment on lines +160 to +165
#ifndef __ZPP__
#define __syscall static inline
#define __syscall_always_inline static inline __attribute__((always_inline))
#else
#define __syscall __zpp(func("syscall", __FILE__, __LINE__))
#define __syscall_always_inline __zpp(func("syscall", __FILE__, __LINE__))
Copy link
Member

Choose a reason for hiding this comment

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

question: both expand to the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The __syscall part hasn't been refactored yet, and this is mostly to demonstrate part of the output.

But I think the current syscall parsing also behaves the same, they end up different when compiling only.

Comment on lines 177 to 181
/* Indicates this is a driver subsystem */
#define __subsystem
#define __subsystem __zpp(struct("__subsystem"))

/* Indicates this is a network socket object */
#define __net_socket
#define __net_socket __zpp(struct("__net_socket"))
Copy link
Member

Choose a reason for hiding this comment

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

for future: these should likely be moved somewhere else

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed

Comment on lines +5 to +9
# Generate compile_commands.json in the output directory
set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE CACHE BOOL
"Export CMake compile commands. Used by zpp.py script"
FORCE
)
Copy link
Member

Choose a reason for hiding this comment

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

this is already set in cmake/modules/kernel.cmake

Copy link
Collaborator Author

@pdgendt pdgendt Dec 17, 2024

Choose a reason for hiding this comment

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

I copied it from there, it should be merged somewhere indeed, but for the PoC it was easier to have it here with less chance of getting conflicts.

@bjarki-andreasen
Copy link
Collaborator

bjarki-andreasen commented Dec 17, 2024

Overall this seems nice, making the compiler do some of the parsing work we do by "hand" now, just hope these attributes are well supported by all the compilers we support :)

@pdgendt
Copy link
Collaborator Author

pdgendt commented Dec 23, 2024

That's a very interesting idea. I like it, my only concern is "Portability for different toolchains" - do you any further data points on this?

It comes down to the C compiler support for the following options:

  • -E -P to only do the preprocessing step (not an issue, I think)
  • -M -MG print dependency list, as well as missing include headers (relative paths)

Documentation is scarce, I based the implementation on GCC options Support for clang should be covered too.

@RobinKastberg does IAR support the C compiler flags -E -P -M -MG?

@RobinKastberg
Copy link
Contributor

@pdgendt Without looking too closely I think this should be no problem (It seems to map to the --preprocess and --dependencies flag)

@RobinKastberg
Copy link
Contributor

@pdgendt C23 annotations might be harder for us right now, but that should be possible to work around?

@pdgendt
Copy link
Collaborator Author

pdgendt commented Jan 8, 2025

@pdgendt C23 annotations might be harder for us right now, but that should be possible to work around?

The C23 annotations aren't used after the pre-processor step currently, they are simply to make it parse-able.

Add a script to act as a pre-processor to capture functions and
structs that require special attention.

Signed-off-by: Pieter De Gendt <[email protected]>
Update some directives to add a C23 annotation instead.

Signed-off-by: Pieter De Gendt <[email protected]>
Create a CMake target to build the annotations together with a depfile.

Signed-off-by: Pieter De Gendt <[email protected]>
Rework gen_iter_sections.py to use the annotations json file generated
by ZPP.

Signed-off-by: Pieter De Gendt <[email protected]>
TO BE REMOVED
For testing enable the Zephyr PreProcessor on all source files.

Signed-off-by: Pieter De Gendt <[email protected]>
Add a new tag for generating linker sections, namely __iterable_section_rom
and rename the device API logic to a more generic one.

Signed-off-by: Pieter De Gendt <[email protected]>
Add __iterable_section_rom to network struct types and remove ld/cmake
entries.

Signed-off-by: Pieter De Gendt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System RFC Request For Comments: want input from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants