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

cmake: add llext compilation module #67431

Merged
merged 3 commits into from
Jan 21, 2024

Conversation

pillo79
Copy link
Collaborator

@pillo79 pillo79 commented Jan 10, 2024

This PR abstracts the current llext compilation code used in the test to the add_llext_target function in the cmake/modules/llext.cmake module, integrating it with the current Zephyr build configuration as much as current llext code allows.

Of course the "extension" model is designed to allow third party, independent binaries to be separately compiled and dynamically linked with the Zephyr binary at run time only.
But having a fully integrated build of a Zephyr binary + a set of extensions has a number of advantages:

  • Extensions can be built with the same compilation options of the main Zephyr binary (ABI, floating point, warnings etc), minimizing the risk of incompatibilities;
  • Extensions will be able to use all existing Zephyr macrobatics (such as devicetree or zassert_* macros) in their code. This will enable them to adapt to the current build requirements, and Twister tests will be able to influence test outcome from within the extension code;
  • @marc-hb suggested being able to build code with different licenses and distribution restrictions in the same single build, and distribute them as separate entities.

Some restrictions on the set of flags that can be used are automatically enforced. This list is expected to be reduced as the Zephyr llext linker improves.


  • 12cdb55 moves the current llext compilation code to an add_llext_target CMake function, so CMakeFiles for llext tests can be greatly simplified;
  • 1be7909 reworks add_llext_target to use a standard Zephyr library compile step for the llext object files, allowing to reuse include directories and macro definitions;
  • bf2e24b completes the transition by moving all arch-specific flags to the cmake/compiler/ folder and reusing the toolchain compiler properties as much as possible.

Copy link
Collaborator

@teburd teburd left a comment

Choose a reason for hiding this comment

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

Some minor nits about where this lives, how its named, and what it does. I love the path this is going.

See thesofproject/sof#8180 and the comments by @marc-hb for additional commentary on why maybe some of this is needed.

subsys/llext/CMakeLists.txt Outdated Show resolved Hide resolved
tests/subsys/llext/hello_world/CMakeLists.txt Outdated Show resolved Hide resolved
subsys/llext/CMakeLists.txt Outdated Show resolved Hide resolved
@pillo79
Copy link
Collaborator Author

pillo79 commented Jan 10, 2024

Thanks for the pointers @teburd, will look into it tomorrow!

@pillo79 pillo79 force-pushed the llext-test-rework branch 3 times, most recently from 94b0bf1 to 8226c24 Compare January 12, 2024 10:32
@pillo79
Copy link
Collaborator Author

pillo79 commented Jan 12, 2024

@teburd I have addressed all notes, but I was not able to create the perfect zephyr_add_llext clone as I couldn't find out how to make the CMake generators (esp. $<TARGET_OBJECTS:target>) work with custom targets, so figuring out the full path to the output llext was clumsy.

Ended up defining an add_llext_target that mostly matches add_custom_target, and can be used like this:

add_llext_target(hello_world_llext
  OUTPUT  ${PROJECT_BINARY_DIR}/hello_world.llext
  SOURCES ${PROJECT_SOURCE_DIR}/src/llext/hello_world.c
  C_FLAGS -Werror
)

so precise path control is still in the hands of the developer (I have skimmed the SOF PR and I see that would be helpful).

This now uses the default Zephyr compile step for the actual llext sources, which should allow a lot more flexibility in llext code. It was a pain to make the Xtensa port work though, since the Zephyr build system forbids creating a dynamic library via add_library(... SHARED ...) - I had to compile as an OBJECT library and then manually invoke the compiler to create a shared object. There is no change in the test output, but @lyakh, please try if you have issues updating SOF to this and let me know.

@pillo79
Copy link
Collaborator Author

pillo79 commented Jan 12, 2024

Lets do this the right way, lets setup a target and add this as a cmake module under zephyr/cmake/modules/llext.cmake

Missed this bit! Will move the add_lext_target code there ASAP.

Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

Thanks for improving on this.

Some comments / observations from a first review round, but on a more general level I would like to know better the general intentions, perhaps some more documentation.

The llext feature was introduced here: #57086

As I see this commit it tries to improve the llext work 👍 by:

  • adding more tests / better test coverage
  • creating Zephyr specific functions for creation of llext files.

Unfortunately it's not fully clear to me which parts relates to improved testing, and which parts is for improved llext functionality in itself.

As I understand llext, then ll code is generally expected to be built independently of the actual Zephyr build (because if it was built as part of the Zephyr build there would be little reason to use the llext feature).
But the add_llext_target() is created as part of a Zephyr build.

Also I notice that the add_llext_target() tries to use the zephyr_interface, for example as: $<TARGET_PROPERTY:zephyr_interface,INTERFACE_COMPILE_DEFINITIONS>.
My question therefore is: is a ll module expected to be built with same flag as Zephyr itself ?
I would assume some flags are important, like fp support, but others doesn't matter.
But if it is expected that users can build a ll module using Zephyr infrastructure (and re-use flags from a given Zephyr), but without building Zephyr itself, then I think a different principle is needed.

Also I think an update to the llext documentation would be nice.

help
Set to "m" to test hello-world loading
Set to "m" to test hello-world loading when modules are enabled.
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure I follow.

If LLEXT_TEST_HELLO=y, then you may also be testing hello world, just not as a module but as compiled in.

Could you rephrase ?

Copy link
Collaborator Author

@pillo79 pillo79 Jan 12, 2024

Choose a reason for hiding this comment

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

The original code for that file was from #67105. Until that got merged, Zephyr did not support tristate entries, so a quick example was added to that llext test to showcase the new functionality. Basically this is the only place where you can set a config value to m and expect it to work. The CMakeFile actually tests this functionality by requiring that if tristates (aka CONFIG_MODULES) are enabled, this is set to "m".

Edit: Removed the original check and replaced with a way simpler

This enables building the hello_world test case.

Comment on lines 10 to 12
elseif(CONFIG_MODULES AND NOT CONFIG_LLEXT_TEST_HELLO STREQUAL "m")
# Modules enabled, but hello_world is not a module
message(FATAL_ERROR "CONFIG_LLEXT_TEST_HELLO must be set to 'm' when CONFIG_MODULES is enabled")
Copy link
Collaborator

Choose a reason for hiding this comment

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

afaict, then in this test you want to verify that LLEXT_TEST_HELLO=m is working, but remember that people may see this CMake code and copy it elsewhere, and in such case the fact that CONFIG_MODULES=y should not require a tri-state symbol to be m, y can still be a valid value.

If you want to test the combination of CONFIG_MODULES=y and CONFIG_LLEXT_TEST_HELLO=m then that should be ensure through testcase.yml, as it already done:

- CONFIG_MODULES=y
- CONFIG_LLEXT_TEST_HELLO=m

if you believe the FATAL_ERROR should be kept, then please provide a good reason for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, I did not think about the fact that the YAML configs are actually enforced by Kconfig and any discrepancy is already considered an error. I will definitely remove this.

- CONFIG_LLEXT_STORAGE_WRITABLE=y
llext.simple.modules_enabled:
arch_allow: xtensa # only useful to test one arch, Xtensa is faster
Copy link
Collaborator

Choose a reason for hiding this comment

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

@PerMac is this a common filter pattern ?
Using arch_allow for only one arch because that arch is faster.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(that is definitely only a time saver to cut the number of tests down, and having to choose, I picked the one compiling faster 🙂)

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's the platform_key test attribute I created for this sort of thing, to build say once per architecture variant with a sim (e.g. build once per qemu we support).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awesome feature, thanks for pointing this out! 🙇
Will use platform_key: simulation instead. 👍

if(CONFIG_ARM)
list(PREPEND LLEXT_C_FLAGS "-mlong-calls" "-mthumb")
add_custom_command(OUTPUT ${output_file}
COMMAND ${CMAKE_C_COMPILER} ${LLEXT_C_FLAGS} -c
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this probably will only work / been verified with gcc (and maybe clang ?) , but we try to define common names and use properties to make it easier to work towards better support of 3rd party toolchains.

https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/compiler/compiler_flags_template.cmake

Please take a look.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 I will add gcc properties to the target_specific files in cmake/compiler/gcc.

# will compile the source file src/llext/hello_world.c to a file
# ${PROJECT_BINARY_DIR}/hello_world.llext, adding -Werror to the compilation.
#
function(add_llext_target target_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally Zephyr defined functions are placed in cmake/modules/extensions.cmake as to make it easier for developers to know where to find those, but I also see the add_llext_target() is a bit special, however I would still like a good reason why to have this function here.

Copy link
Collaborator Author

@pillo79 pillo79 Jan 12, 2024

Choose a reason for hiding this comment

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

That has now moved to cmake/modules/llext.cmake as per an earlier suggestion. Is this new location acceptable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, I noticed later, and actually made a comment, but seems I missed adding the comment to the review 😞 .
Please see followup comment here : #67431 (comment)

get_filename_component(output_name ${output_file} NAME)

# Add user-visible target and dependency
add_custom_target(${target_name} ALL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this added to ALL ?

I haven't yet gotten to the exact / expected workflow when embedding llext files in a regular build, but I assume the Zephyr kernel build and the llext builds will usually be done independently, else there seems to be little reason to support loading obj as module.

In which case next question arises, why is a full Zephyr build done, just to get the llext files ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ALL is probably redundant here. I will remove it so it only gets made when needed to create the source for the test code.

I agree building Zephyr and extensions them separately is a very important use case. However, there are 2 instances off the top of my head where it makes sense to have them integrated into a single build run:

  • test cases, such as the one here 🙂
  • (going out on a limb, maybe @lyakh can confirm) e.g. for SOF to build and release a new audio firmware + a set of filters.

@pillo79
Copy link
Collaborator Author

pillo79 commented Jan 12, 2024

Thanks @tejlmand for the in-depth review and insights. I will address them and provide a revised version.

Regarding your general questions on llext, there is still a lot of work to do before this can be made easy to use (currently, global and static variables are mostly unhandled... 😅). I already have some of these linker features working in a draft PR, but being able to test them was equally important, therefore I started on this "easier" road.

This series does not add any test code or llext feature, it just refactors tests/subsys/llext so an llext test is cleaner and simpler to replicate (removing per-arch YAML tests, providing a generic build machinery, etc).

is a ll module expected to be built with same flag as Zephyr itself ?

It may be or may be not. Of course a fully decoupled, independent build is possible, but having a sort of "overlay" with tightly integrated Zephyr code may be really useful. For example, my next PR will include a new test llext that uses zassert_equal from inside the llext. This requires most of the flags, include directories etc to be lifted from the main build directly to the extension file.

I promise I will address the appalling documentation as soon as some more features are in. 🤞

@teburd
Copy link
Collaborator

teburd commented Jan 12, 2024

Thanks @tejlmand for the in-depth review and insights. I will address them and provide a revised version.

Regarding your general questions on llext, there is still a lot of work to do before this can be made easy to use (currently, global and static variables are mostly unhandled... 😅). I already have some of these linker features working in a draft PR, but being able to test them was equally important, therefore I started on this "easier" road.

This series does not add any test code or llext feature, it just refactors tests/subsys/llext so an llext test is cleaner and simpler to replicate (removing per-arch YAML tests, providing a generic build machinery, etc).

is a ll module expected to be built with same flag as Zephyr itself ?

I would like to see this happen yes, most of the compilation flags should be replicated with perhaps a few differences. Mostly because of the way llext's need to be built today as partially linked elfs for arm with limited relocations supported. It would be great to replicate most of the warning flags, optimization level, call abi, various math flags, etc. I'm glad to see this is being worked out as I puzzled over how to to reuse the zephyr compiler flags myself, they aren't a simple cmake variables as I gather but instead attached to a target (zephyr_interface) and this frankly made things a little awkward with my limited cmake hackery.

It may be or may be not. Of course a fully decoupled, independent build is possible, but having a sort of "overlay" with tightly integrated Zephyr code may be really useful. For example, my next PR will include a new test llext that uses zassert_equal from inside the llext. This requires most of the flags, include directories etc to be lifted from the main build directly to the extension file.

I have one user that will most likely need to build things outside of zephyr. Related is the compiler flags which I'd like to export into a generated set of files, @edersondisouza might have better ideas here though perhaps. But imagine the scenario of base firmware being created alongside a plugin/extension/whatever SDK including headers, cmake files, linker scripts, etc etc.

I promise I will address the appalling documentation as soon as some more features are in. 🤞

Documentation improvements are very much appreciated.

@@ -0,0 +1,111 @@
# SPDX-License-Identifier: Apache-2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to this specific line, but comment relating to ee9a514 + 113902e vs d435f7b:
reviewing PR's where code is moved around between commits makes it very hard to review a PR commit-by-commit.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

First, I want to second @teburd and state how grateful we are that you are doing this. It's incredibly rare to find people with the CMake skills and patience to do something like this (not me!) which makes this PR even more valuable. This is amazing.

Answering more than one single person below:

Unfortunately it's not fully clear to me which parts relates to improved testing, and which parts is for improved llext functionality in itself.

Also, tests are only a means to an end. The most valuable part of this PR is IMHO NOT the tests but the generic CMake code. Please update the currently unassuming PR title to reflect that! Drop the word "tests" from it. Ideally, every code addition or change should have corresponding tests; this should go without even saying :-)

I agree building Zephyr and extensions them separately is a very important use case. However, there are 2 instances off the top of my head where it makes sense to have them integrated into a single build run...

Maybe add this one: you can have a mix of sources with different licences in a single build. These different licences may require you to release binaries from the same build separately. Building everything at once make sure everything is in sync and matches: sources, build flags, toolchains, etc.

Of course building separately is desirable too - but not just.

Due to some file moves it is best to review each commit independently:

reviewing PR's where code is moved around between commits makes it very hard to review a PR commit-by-commit.

Have you considered splitting this into smaller PRs? They could be merged faster.

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

I promise I will address the appalling documentation as soon as some more features are in.

Hey, maybe something I can help with in the future? I would recommend not spending much time on it yet, better wait for the dust to settle a bit first. llext seems very young yet, probably does not need a huge crowd of users yet...

DEPENDS ${target_name}_lib $<TARGET_OBJECTS:${target_name}_lib>
)

endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
endif()
else()
message(FATAL_ERROR "unsupported...
endif()

project(llext_hello_world_test)

if(NOT CONFIG_LLEXT_TEST_HELLO)
# test disabled, nothing to do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pardon my ignorance but is this really how/where tests get enabled/disabled? In Kconfig and in their CMakeLists.txt file? This looks strange, I would have expected testcase.yaml to be enough...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That code was introduced in aee2d1a as the only example usage of tristate in the Zephyr code. There is no actual need to do that, but I did not want to remove it either, so I just moved that around.

@pillo79 pillo79 changed the title llext: test rework to enable multiple tests cmake: add llext compilation module Jan 17, 2024
@pillo79
Copy link
Collaborator Author

pillo79 commented Jan 17, 2024

Thanks @marc-hb for your kind words - my goal was just to refactor the current llext test so it's easier to add more; I have bigger llext changes waiting in the pipe, but I wanted to start with the easy stuff. But then @teburd casually mentioned moving the code to cmake/ and I fell in the trap! 🤣

I have split away the (original!) llext test refactors and concentrated on the cmake infrastructure. Hope this eases the review process and sorry for the noise.

@pillo79 pillo79 requested a review from marc-hb January 17, 2024 11:04
teburd
teburd previously approved these changes Jan 17, 2024
marc-hb
marc-hb previously approved these changes Jan 17, 2024
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

My CMake expertise is just above the average (which is itself close to zero) but this looks very clean, readable and maintainable. thanks! Great starting point for sure.

If @tejlmand, @teburd and @lyakh are happy then I am too.

@tejlmand
Copy link
Collaborator

tejlmand commented Jan 18, 2024

I have one user that will most likely need to build things outside of zephyr. Related is the compiler flags which I'd like to export into a generated set of files, @edersondisouza might have better ideas here though perhaps.

Zephyr already supports https://docs.zephyrproject.org/latest/kconfig.html#CONFIG_MAKEFILE_EXPORTS.
That feature exports Zephyr's build flags to files named: Makefile.exports.<lang>

That feature goes a long time back, #22020, and maybe we should consider to support an equivalent CONFIG_CMAKE_BUILD_FLAGS_EXPORTS setting which does the same, but in a format which can be directly consumed by another CMake build system.
Perhaps with some more configuration info.

As another piece of info I can mention that Zephyr default enables CMAKE_EXPORT_COMPILE_COMMANDS, although that feature probably contains a bit more info than what is needed in this case.

Feel free to open an enhancement issue describing the exact needs / use-case, and feel free to refer to some of the existing features I mention above.

I even think providing such a feature could allow to have an even cleaner split between the build of Zephyr and the build of llext module in the future.

Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

implementation looks nice.

A single comment with regards to the new llext.cmake module, which I would like to address.
Both to get the reasoning described, but also for future reference as the reason for allowing an exception in the llext case.

# will compile the source file src/llext/hello_world.c to a file
# ${PROJECT_BINARY_DIR}/hello_world.llext, adding -Werror to the compilation.
#
function(add_llext_target target_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/modules/extensions.cmake for Zephyr specific functions.

Part of the reason is having a single location where developers will know where to find functions defined by the project, but there are of course exceptions, but before accepting llext.cmake as it's own module then I would like to know the reasons for not having it in extensions.cmake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right! I was not sure about merging this into extensions.cmake for two reasons:

  • llext is quite young and I assume a lot of work will be done on that function (and possibly others), so I wanted to make it clear that work on that file was not interfering with the rest of Zephyr developments;
  • the functions in extensions.cmake are absolutely generic and really useful to every part of Zephyr,
    while llext is (currently!) a very specific functionality, so it looked early to have this so front and center.

Still, no fundamental objections to moving, of course. If you wish so, should I create a "7. Linkable loadable extensions" section at the end?

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for the reasoning.

While it's true that majority of functions in extensions.cmake are generic / useful to most parts of Zephyr development, then there does exists some functions which are more dedicated to certain areas.

Although young, then let's consider the llext feature an integral part of the Zephyr build system, and giving it is own section in extensions.cmake 🎉

This patch defines a generic function that encapsulates all the
architecture-specific machinery needed to compile llexts from source
files. Current tests are updated to use this function.

Output and source files must be specified using the OUTPUT and SOURCES
arguments. Only one source file is currently supported.

Arch-specific flags will be added automatically. The C_FLAGS argument
can be used to pass additional compiler flags to the compilation of
the source file.

Signed-off-by: Luca Burelli <l.burelli@arduino.cc>
Separate the compilation of the source file from the conversion to an
llext file. This allows to reuse the include directories and macro
definitions of the current Zephyr build while compiling the llext source.

Signed-off-by: Luca Burelli <l.burelli@arduino.cc>
Minimize the amount of flags that are hardcoded in the llext.cmake
module by moving them to the compiler specific cmake files.

The llext.cmake module will now use the new LLEXT_REMOVE_FLAGS and
LLEXT_APPEND_FLAGS global variables to adjust the set of flags used
to compile the llext code. This will make it easier to add support
for new architectures and compilers.

Signed-off-by: Luca Burelli <l.burelli@arduino.cc>
)
else()
message(FATAL_ERROR "add_llext_target: unsupported architecture")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you really mean to drop this line?

# LLEXT_REMOVE_FLAGS and LLEXT_APPEND_FLAGS global variables.

# The C_FLAGS argument can be used to pass additional compiler flags to the
# compilation of this particular llext.
Copy link
Collaborator

@marc-hb marc-hb Jan 19, 2024

Choose a reason for hiding this comment

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

in CI we use command-line parameters like -DEXTRA_CXXFLAGS=-Werror a lot. Will they work with llext too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am no CMake guru so I just tested - it does (well, with EXTRA_CFLAGS as it's a C file, but it does indeed use command line flags in the llext as well).

"$<TARGET_PROPERTY:zephyr_interface,INTERFACE_COMPILE_OPTIONS>"
)
set(zephyr_filtered_flags
"$<FILTER:${zephyr_flags},EXCLUDE,${llext_remove_flags_regexp}>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is scary stuff :-) How about adding a couple message(DEBUG $llext_remove_flags_regexp) and message(DEBUG zephyr_filtered_flags)?
I'm surprised you didn't need them for yourself!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used plenty of them for debugging indeed 😇 but I did not push them, as they are quite cumbersome: to see the end result to that gibberish you need to add a custom_target that echoes the flags on the console. The generator syntax is next-level painful to me!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yes of course, it's build-time. Got it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

cmake is next level painful, thanks for taking this on

DEPENDS ${target_name}_lib $<TARGET_OBJECTS:${target_name}_lib>
)

else()
message(FATAL_ERROR "add_llext_target: unsupported architecture")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed and added back, looks like a small git glitch.

@henrikbrixandersen henrikbrixandersen merged commit c5b7aab into zephyrproject-rtos:main Jan 21, 2024
19 checks passed
@pillo79 pillo79 deleted the llext-test-rework branch January 22, 2024 14:41
Comment on lines +4827 to +4829
if (NOT CONFIG_LLEXT)
message(FATAL_ERROR "add_llext_target: CONFIG_LLEXT must be enabled")
endif()
Copy link
Collaborator

@kartben kartben Feb 14, 2024

Choose a reason for hiding this comment

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

Just curious, what would be wrong with trying/wanting to generate an llext for a project that doesn't have LLEXT enabled. One needn't enable LLEXT in a project if they only care about having "snippets" of it built as llext, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, indeed you could compile only the llext, since there's no real technical barrier. However, the generated file would not work with the same Zephyr core with which it was built (no exported symbols or machinery to load it), so it looked like a config error in my eyes.


Now that I look at it again, the converse is useful though - if you know what you are doing and purposefully disable CONFIG_LLEXT on a project that defines such targets, it makes sense not to build them and not complain, unless they are required by other parts in the project.

Taking into account your other comment on dependencies ("why isn't a llext built if it's defined as part of a project?"), I think the most appropriate way to deal with all this should be:

  • if CONFIG_LLEXT is not enabled, do not complain in add_llext_target and define an empty target instead, so other llext_* APIs don't break as well.
  • automatically add the llext target to ALL so it's always built if it's defined in the CMake.

This would result in an actual build error only when LLEXT is disabled and the output file is a required dependency (for example by generate_inc_file_for_target) - or if the Zephyr code actually calls llext APIs, obviously.

@teburd, what do you think about this?

Copy link
Collaborator

@marc-hb marc-hb Mar 2, 2024

Choose a reason for hiding this comment

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

Sorry for the belated reply and - much worse - sorry for not being @teburd.

disable CONFIG_LLEXT on a project that defines such targets, it makes sense not to build them and not complain,

I probably (!) disagree with that? The main purpose of Kconfig is to turn software features on and off. But "llext" isn't really a software feature, it's more about HOW you want some of those features to be delivered: as a separate module or built-in? So CONFIG_LLEXT is more like a "dependency" of real features.

Consider the Linux kernel, which many llext aspects take inspiration from. It had to already deal with this problem. If you turn off Linux CONFIG_MODULES in make menuconfig then it immediately forces you to make all features built-in. Editing the .config and running make oldefconfig does the same thing.

Of course there's a massive difference: the Linux kernel can very quickly and easily flip drivers that support between module and built-in, whereas llext does not do that all. But that "inspiration" is still useful IMHO.

So if you:

  • enable some particular llext feature in Kconfig, AND
  • you turn off CONFIG_LLEXT

... then you have a configuration error and the build should 1) fail, 2) fail as soon as possible, not wait until compilation whether some symbol is used maybe, maybe not.

In other words, if you turn off CONFIG_LLEXT then you MUST also turn off all features that depend on it - and maybe Kconfig can/should help with that but either way CMake should fail ASAP if you feed it an inconsistent configuration.

Copy link
Collaborator Author

@pillo79 pillo79 Mar 4, 2024

Choose a reason for hiding this comment

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

[...] "llext" isn't really a software feature, it's more about HOW you want some of those features to be delivered [...]

In fact, llext is only the implementation enabling the "dynamic loading" feature. The HOW you want [features] to be delivered part is CONFIG_MODULES, which was recently merged in Zephyr. No actual in-tree users yet though - having dynamically loadable parts of the Zephyr core is still far out.

Given that discussion and the existence of both MODULES and LLEXT, I think what you are describing above is a strict dependency between them: either MODULES implies LLEXT, or depends on it, or there's a config error. Absolutely agree with that!

However, and this is when MODULES=n, I still maintain there is merit in building llext-related projects with LLEXT=n without complaining. This would enable scenario 1 of the following 3:

  1. if projects want optional extension support, with a few strategic #ifdef CONFIG_LLEXT they are able to build either way;
  2. if they need extension support and do not care about optionality, they just enable it in their prj.conf;
  3. if they require the functionality but there is a config error, no core functions are defined so the mistake is definitely detected at link time.

OT: We need an RFC / implementation plan on llext pronto! 😅 . I'll try to draft one shortly and share it so we can all agree on the long term path. 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll try to draft one shortly and share it so we can all agree on the long term path

Thank you so much for leading here again. Whatever are the use cases and intended Kconfig interactions, I think we all agree they cannot just be inferred from scattered comments in source and code reviews. Some high-level design documentation is required indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: llext Linkable Loadable Extensions area: Samples Samples area: Toolchains Toolchains
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants