llext: additional CMake harmonizations #67997

pillo79 · 2024-01-23T11:50:06Z

This PR reworks add_llext_target to more cleanly mimic existing CMake machinery, and support adding custom commands to the extension building stages (as turned out to be useful in this SOF PR).

The C_FLAGS argument to add_llext_target has been removed. Flags, defines, paths and similar configuration options can now be set using functions that mimic CMake's target_* API.

Custom build steps can be requested via the new add_llext_command function, that shares the API with add_custom_command:

add_llext_command(
   TARGET llext_target
   PRE_BUILD | POST_BUILD | POST_PKG
   COMMAND ...
)

Intermediate file or target names are exported via properties on the main target and can be accessed via the get_target_property() function:
- lib_target: Target name for the compilation and/or link step.
- lib_output: Name of the binary file resulting from compilation and/or linking steps.
- pkg_input: Name of the binary file to be used as input to the packaging stage.
- pkg_output: Name of the final llext file.

The phase options have natural meanings:

PRE_BUILD runs after compilation, but before any linking step takes place (as CMake); might use lib_target to get additional variables.
POST_BUILDruns after all code-related compile/link stages have been executed (as CMake), but before packaging; this is expected to create pkg_input by processing the contents of lib_output.
POST_PKG is a new stage after the packaging in .llext is complete; might access the contents of the file pkg_output.

pillo79 · 2024-01-23T11:54:53Z

@lyakh I believe you could use something like this:

add_llext_target(hello_world
  OUTPUT  ${llext_bin_file}
  SOURCES ${llext_src_file}
  POSTPROC_CMD  echo
  POSTPROC_ARGS ${CMAKE_C_COMPILER} --
                $<TARGET_PROPERTY:hello_world_lib,COMPILE_FLAGS>
                $<TARGET_PROPERTY:hello_world_lib,LINK_FLAGS>
                $<TARGET_PROPERTY:hello_world_lib,LINK_OPTIONS>
                $<TARGET_OBJECTS:hello_world_lib>
)

which results in the following command being run:

echo zephyr/twister-out/qemu_xtensa/tests/subsys/llext/llext.simple.xtensa/hello_world/libhello_world_lib.so hello_world_pp.llext zephyr-sdk-0.16.4/xtensa-dc233c_zephyr-elf/bin/xtensa-dc233c_zephyr-elf-gcc --   -gdwarf-4;-fPIC;-nostdlib;-nodefaultlibs zephyr/twister-out/qemu_xtensa/tests/subsys/llext/llext.simple.xtensa/hello_world/CMakeFiles/hello_world_lib.dir/./hello_world.c.obj

pillo79 · 2024-01-23T11:58:11Z

@tejlmand feedback welcome on this - these lines are a global CMake config but need to be run after project() is executed as far as I understood. Having it in cmake/modules or cmake/compiler means it is executed before and gets erased. Is this location acceptable?

lyakh · 2024-01-24T14:48:54Z

-gdwarf-4;-fPIC;-nostdlib;-nodefaultlibs

@pillo79 some list-conversion missing?

pillo79 · 2024-01-24T15:26:35Z

@pillo79 some list-conversion missing?

@lyakh thanks for noticing, indeed missed a COMMAND_EXPAND_LISTS. Fixed!

tejlmand · 2024-01-25T12:03:49Z

cmake/modules/extensions.cmake

+#                    OUTPUT  <output_file>
+#                    SOURCES <source_file>
+#                    [C_FLAGS <flags> ...]
+#                    [POSTPROC_CMD <cmd> [POSTPROC_ARGS <args> ...]]


not fond of this, let's split the functions.

add_llext_target() creates the target and accepts source files, similar to CMake's add_custom_target / add_library.

But we should consider which one add_llext_target() resembles. With this PR I'm starting to think it probably more resembles add_library than custom_target.

Therefore I believe that instead of having POSTPROC_CMD then we should be have a function which resembles add_custom_command(TARGET <target> PRE_/POST_BUILD).

As I see, we need finer granularity, so an idea could be:

add_llext_command(TARGET <llext_target> [ PRE_BUILD | PRE_STRIP | POST_BUILD | <other> ] # Choose what is appropriate in this context COMMAND .... )

That will also allow to gradually build the target and extra steps based on various other setting and not having to know everything from start.

Like:

add_llext_target(...) if(<something>) add_llext_command(...) endif()

Note: should we consider to rename add_llext_target() to add_llext_library() to make it more clear that we actually produce a lib.

The product is an llext, not necessarily a library, it could be a partially linked elf representing a whole application to load, so I think adding the term library is a misrepresentation in this case. If shorter and closer to add _library is needed, then abbreviating to add_llext is fine, its a target to build.

The rest makes good sense to me!

The product is an llext, not necessarily a library, it could be a partially linked elf representing a whole application to load, so I think adding the term library is a misrepresentation in this case.

and yet the term lib is used all over the place in this PR:

zephyr/cmake/modules/extensions.cmake

Line 5172 in 37f60ed

set(llext_lib_target ${target_name}_llext_lib)

zephyr/cmake/modules/extensions.cmake

Line 5177 in 37f60ed

set(llext_lib_output $<TARGET_OBJECTS:${llext_lib_target}>)

zephyr/cmake/modules/extensions.cmake

Line 5195 in 37f60ed

target_compile_definitions(${llext_lib_target} PRIVATE

etc.

tejlmand · 2024-01-25T12:05:45Z

cmake/modules/extensions.cmake

+#   add_llext_target(<target_name>
+#                    OUTPUT  <output_file>
+#                    SOURCES <source_file>
+#                    [C_FLAGS <flags> ...]


let's have a dedicate function for C_FLAGS, perhaps llext_compile_options(<target> <options>)

marc-hb

For testing convenience and coverage, could you add some dummy post-processing in one of the samples or tests?

lyakh · 2024-01-26T06:52:41Z

Hi, I was able to use this for building an SOF LLEXT binary and it worked well! So you can add my "Tested-by" to your PR if that's something commonly done in Zephyr. But I did have to add C_FLAGS "-std=gnu99" and -shared to linking flags, maybe they should be added automatically for Xtensa

pillo79 · 2024-01-31T16:49:54Z

( DNM since I can't have this reviewed by the release window closing )

pillo79 · 2024-02-01T14:23:43Z

@tejlmand @teburd @lyakh I should have addressed all concerns.

The function added to customize llext build steps is now exactly like add_custom_command:

add_llext_command(
   TARGET llext_target
   PRE_BUILD | PRE_PACK | POST_BUILD
   COMMAND ...
)

PRE_BUILD and POST_BUILD have the usual meanings, while the new PRE_PACK stage is after linking but before "converting to llext" (currently only that strip, but maybe in the future more processing will be required? signing?).
Everything after COMMAND is passed verbatim to the actual add_custom_command, so it should be as flexible as the CMake function.
Since it is not possible to make custom targets work like an add_library/add_executable, I created custom target properties that can be read by user code to get the intermediate stages:
- LinkTarget: Target name for the compilation and/or link step.
- LinkedFile: Object file resulting from compilation and/or linking steps.
- PostProcTarget: Target name for the post-processing step.
- PostProcFile: Post-processed file, before the conversion to an llext file.
- OutputFile: Final llext file.

The C_FLAGS argument has also been removed. To set flags, paths, defines et al I have added a set of llext_ functions that mimic the CMake standard target_ API but work on the correct private target.

pillo79 · 2024-02-01T14:27:39Z

@lyakh your use case is also better supported now, since:

the full command is available for customizations (you can re-add -i/-o, sorry for that!);
no need to hardcode internal target names, they are available via generic properties.

I would rewrite the above example as:

# Generate the llext target
add_llext_target(hello_world
  OUTPUT  ${llext_bin_file}
  SOURCES ${llext_src_file}
)

# Get internal file and target names 
get_target_property(link_target hello_world LinkTarget)
get_target_property(linked_file hello_world LinkedFile)
get_target_property(postproc_file hello_world PostProcFile)

# Add the custom command using the above variables
add_llext_command(
  TARGET hello_world
  PRE_PACK
  COMMAND echo ${linked_file} ${postproc_file} ${CMAKE_C_COMPILER} --
                $<TARGET_PROPERTY:${link_target},COMPILE_FLAGS>
                $<TARGET_PROPERTY:${link_target},LINK_FLAGS>
                $<TARGET_PROPERTY:${link_target},LINK_OPTIONS>
                $<TARGET_OBJECTS:${link_target}>
)

pillo79

New update addressing (I hope) all open points:

The global property change is now addressed at its root, by defining a custom Zephyr SYSTEM_NAME, to properly shadow the original dynamic libs override in Generic.cmake.
Reworked llext_proc_target's custom command so its contents and logic is unambiguous.
Made the PRE_BUILD custom step fail on ARM, since it has no linking step and there is no way to properly implement a workaround for it.
Additional function comments explaining expected behavior of the custom commands.

@tejlmand please let me know your feedback.

pillo79 · 2024-03-13T16:52:19Z

cmake/modules/extensions.cmake

+  elseif(CONFIG_XTENSA)
+
+    # Ensure shared library support is enabled
+    set_property(GLOBAL PROPERTY TARGET_SUPPORTS_SHARED_LIBS true)


Finally found it! 🥳

It's getting cleared in upstream Generic.cmake, a file which CMake loads as the "system name" set here:

zephyr/cmake/modules/FindTargetTools.cmake

Lines 36 to 43 in 4045975

# https://gitlab.kitware.com/cmake/community/wikis/doc/cmake/CrossCompiling:

# CMAKE_SYSTEM_NAME : this one is mandatory, it is the name of the target

# system, i.e. the same as CMAKE_SYSTEM_NAME would have if CMake would run

# on the target system. Typical examples are "Linux" and "Windows". This

# variable is used for constructing the file names of the platform files

# like Linux.cmake or Windows-gcc.cmake. If your target is an embedded

# system without OS set CMAKE_SYSTEM_NAME to "Generic".

set(CMAKE_SYSTEM_NAME Generic)

So this is part of upstream CMake, and not modifiable per se.
I addressed it by doing set (CMAKE_SYSTEM_NAME Zephyr) in FindTargetTools.cmake and providing a simple Platform/Zephyr.cmake that reuses Generic, but enables dynamic linking support when LLEXT is enabled.

marc-hb · 2024-03-19T14:56:43Z

I just noticed by chance that a (probably small) conflict appeared in tests/subsys/llext/simple/CMakeLists.txt. Github does unfortunately not notify about new conflicts.

When CONFIG_LLEXT is enabled, the Zephyr platform needs to enable dynamic library support. This is done by setting the `TARGET_SUPPORTS_SHARED_LIBS` property to `TRUE` in the global property scope. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

This change reworks the Xtensa support in llext to use CMake's native shared library support, instead of manually running "gcc -shared". This change minimizes the differences in llext handling by defining appropriate CMake targets for the different architectures. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

Remove the C_FLAGS argument from add_llext_target() and add a set of functions to configure the compilation and linking of an llext using the same API of the target_* functions. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

This patch adds support for custom commands to be executed during the build of an llext target. The commands can be executed at different points in the build process: - PRE_BUILD: Before the llext code is linked. - POST_BUILD: After the llext code is built, but before packaging it in an .llext file. - POST_PKG: After the llext file has been created. Note that PRE_BUILD is not supported for ARM targets, as in that case object files are used directly and there is no actual linking step. The commands can be added using the new add_llext_command() function. An example usage of it, along with some target properties, is added to the hello_world test case. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 · 2024-03-20T11:59:58Z

a (probably small) conflict appeared in tests/subsys/llext/simple/CMakeLists.txt.

Thanks. Yes, the sample code needs to be included inside the new if block. Rebased to latest main to fix this, no functional change.

teburd · 2024-03-20T15:48:26Z

@tejlmand any further feedback? There is other work depending on this

teburd · 2024-03-26T13:48:22Z

@tejlmand arguably this is an LLEXT set of changes so I've re-assigned to myself, if this is wrong please do correct it

Review is stale with several weeks given to update and comments seem to have been addressed

pillo79 · 2024-04-04T14:20:30Z

@marc-hb @lyakh one more review required for this to finally end in the merge queue 🥳

marc-hb

I don't understand all the CMake subtleties but everything I understand makes sense. Moreover this PR is very clear, well structured and well commented = highly "maintainable".

I also wonder about the relationship with #70793 but if @lyakh can test this and is happy with it then I'm happy too.

marc-hb · 2024-04-05T17:33:41Z

cmake/modules/extensions.cmake

+# the build. The command will be executed at the specified build step and
+# can refer to <target>'s properties for build-specific details.
+#
+# The differrent build steps are:


Minor typo in comment. Obviously not blocking. Do not force-push and lose approvals ONLY for this.

lyakh · 2024-04-08T06:09:04Z

I also wonder about the relationship with #70793 but if @lyakh can test this and is happy with it then I'm happy too.

@marc-hb It is my goal and wish to first establish a PR CI chain for both Zephyr twister and SOF LLEXT use-case. After that I'll gladly continue with my attempt to move over SOF LLEXT cmake code to this new base. I tried it before and it worked well, but we have changed a couple of things since then, so, need to try again. Will work with @pillo79 if any new requirements arise. Yes, I definitely agree that switching over to using these functions would be a good improvement!

…roduction of cmake/modules/Platform/Zephyr.cmake This commit reverts a breaking change in CMAKE_SYSTEM_NAME introduced by zephyrproject-rtos#67997 (changing it from "Zephyr" back to "Generic") and removes the file `cmake/modules/Platform/Zephyr`. Both changes in the aforementioned PR were only introduced to ultimately modify the value of the global CMake property TARGET_SUPPORTS_SHARED_LIBS for the special case of building for Xtensa with LLEXT. The modification of CMAKE_SYSTEM_NAME is considered a breaking change because it has the potential to alter the build of any non-trivial project that previously checked for the "Generic" system identifier as corresponding to Zephyr - for example by doing `if (CMAKE_SYSTEM_NAME STREQUAL "Generic")`. Such builds may now break in many ways including silently when there is no `else()` clause with a `message()` to alert the user that a whole configuration block had been skipped. In essesnce, that CMAKE_SYSTEM_NAME modification was only introduced in order to have CMake to load `cmake/modules/Platform/Zephyr.cmake` which in turn adjusted the value of TARGET_SUPPORTS_SHARED_LIBS. But the use of a CMake platform file like this is ineffective for non-trivial projects where one or more top level CMake `project()` calls may happen before the first call to `find_package(Zephyr)` because in such cases CMAKE_MODULE_PATH will not have been modified yet to contain the path to <Zephyr_ROOT>/cmake/modules and thus no platform file will be include by CMake. This patch moves the conditional override of TARGET_SUPPORTS_SHARED_LIBS into the `kernel.cmake` module which is known to be the first call to `project()` that enables any language and thus the one that must come before any artifact target can be defined. Signed-off-by: Nicolas Lebedenco <nicolas@lebedenco.net>

…roduction of cmake/modules/Platform/Zephyr.cmake This commit reverts a breaking change in CMAKE_SYSTEM_NAME introduced by zephyrproject-rtos#67997 (changing it from "Zephyr" back to "Generic") and removes the file `cmake/modules/Platform/Zephyr`. Both changes in the aforementioned PR were only introduced to ultimately modify the value of the global CMake property TARGET_SUPPORTS_SHARED_LIBS for the special case of building for Xtensa with LLEXT. The modification of CMAKE_SYSTEM_NAME is considered a breaking change because it has the potential to alter the build of any non-trivial project that previously checked for the "Generic" system identifier as corresponding to Zephyr - for example by doing `if (CMAKE_SYSTEM_NAME STREQUAL "Generic")`. Such builds may now break in many ways including silently when there is no `else()` clause with a `message()` to alert the user that a whole configuration block had been skipped. In essence, that CMAKE_SYSTEM_NAME modification was only introduced in order to have CMake to load `cmake/modules/Platform/Zephyr.cmake` which in turn adjusted the value of TARGET_SUPPORTS_SHARED_LIBS. But the use of a CMake platform file like this is ineffective for non-trivial projects where one or more top level CMake `project()` calls may happen before the first call to `find_package(Zephyr)` because in such cases CMAKE_MODULE_PATH will not have been modified yet to contain the path to <Zephyr_ROOT>/cmake/modules and thus no platform file will be include by CMake. This patch moves the conditional override of TARGET_SUPPORTS_SHARED_LIBS into the `kernel.cmake` module which is known to be the first call to `project()` that enables any language and thus the one that must come before any artifact target can be defined. Signed-off-by: Nicolas Lebedenco <nicolas@lebedenco.net>

pillo79 added the area: Linkable Loadable Extensions label Jan 23, 2024

pillo79 requested review from teburd, lyakh, tejlmand and marc-hb January 23, 2024 11:50

zephyrbot added area: Toolchains Toolchains area: Build System labels Jan 23, 2024

zephyrbot requested review from 57300, jeremybettis, nashif, nordicjm and stephanosio January 23, 2024 11:50

zephyrbot assigned tejlmand Jan 23, 2024

pillo79 force-pushed the llext-post-processor branch from 6873e7d to 101a21b Compare January 24, 2024 15:25

tejlmand requested changes Jan 25, 2024

View reviewed changes

marc-hb mentioned this pull request Jan 25, 2024

llext: add support for signing additional binaries #66573

Closed

marc-hb reviewed Jan 25, 2024

View reviewed changes

marc-hb mentioned this pull request Jan 26, 2024

loadable libraries and smart-amp-test as an example thesofproject/sof#8180

Merged

pillo79 added the DNM This PR should not be merged (Do Not Merge) label Jan 31, 2024

pillo79 force-pushed the llext-post-processor branch from 101a21b to 1032c24 Compare February 1, 2024 11:15

pillo79 removed the DNM This PR should not be merged (Do Not Merge) label Feb 1, 2024

pillo79 changed the title ~~llext: add custom post-processor step to add_llext_target~~ llext: additional CMake harmonizations Feb 1, 2024

pillo79 dismissed teburd’s stale review via 2500c59 March 18, 2024 18:22

pillo79 force-pushed the llext-post-processor branch from 21b29c3 to 2500c59 Compare March 18, 2024 18:22

marc-hb mentioned this pull request Mar 18, 2024

LLEXT full ARM relocation support (adding executable) #70171

Closed

pillo79 commented Mar 18, 2024

View reviewed changes

pillo79 requested a review from tejlmand March 18, 2024 19:27

pillo79 added 4 commits March 20, 2024 12:52

llext: add llext_* configuration functions

947d3d0

Remove the C_FLAGS argument from add_llext_target() and add a set of functions to configure the compilation and linking of an llext using the same API of the target_* functions. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 force-pushed the llext-post-processor branch from 2500c59 to cf0b25e Compare March 20, 2024 11:57

pillo79 requested review from teburd and marc-hb March 20, 2024 13:04

teburd assigned teburd and unassigned tejlmand Mar 26, 2024

teburd approved these changes Mar 26, 2024

View reviewed changes

marc-hb approved these changes Apr 5, 2024

View reviewed changes

carlescufi merged commit 313c09b into zephyrproject-rtos:main Apr 8, 2024
24 checks passed

pillo79 mentioned this pull request Apr 9, 2024

llext: add build commands and tests for relocatable ELFs #71279

Merged

nlebedenco mentioned this pull request Apr 14, 2024

Revert breaking change to CMAKE_SYSTEM_NAME and unnecessary introduction of cmake/modules/Platform/Zephyr.cmake #71468

Merged

pillo79 deleted the llext-post-processor branch April 22, 2024 15:29

henrikbrixandersen added the area: llext Linkable Loadable Extensions label Jul 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llext: additional CMake harmonizations #67997

llext: additional CMake harmonizations #67997

pillo79 commented Jan 23, 2024 •

edited

Loading

pillo79 commented Jan 23, 2024

pillo79 commented Jan 23, 2024

lyakh commented Jan 24, 2024

pillo79 commented Jan 24, 2024 •

edited

Loading

tejlmand Jan 25, 2024

teburd Jan 26, 2024 •

edited

Loading

tejlmand Mar 11, 2024

tejlmand Jan 25, 2024

marc-hb left a comment

lyakh commented Jan 26, 2024

pillo79 commented Jan 31, 2024

pillo79 commented Feb 1, 2024

pillo79 commented Feb 1, 2024

pillo79 left a comment

pillo79 Mar 13, 2024 •

edited

Loading

marc-hb commented Mar 19, 2024

pillo79 commented Mar 20, 2024 •

edited

Loading

teburd commented Mar 20, 2024

teburd commented Mar 26, 2024

pillo79 commented Apr 4, 2024

marc-hb left a comment

marc-hb Apr 5, 2024

lyakh commented Apr 8, 2024

	# https://gitlab.kitware.com/cmake/community/wikis/doc/cmake/CrossCompiling:
	# CMAKE_SYSTEM_NAME : this one is mandatory, it is the name of the target
	# system, i.e. the same as CMAKE_SYSTEM_NAME would have if CMake would run
	# on the target system. Typical examples are "Linux" and "Windows". This
	# variable is used for constructing the file names of the platform files
	# like Linux.cmake or Windows-gcc.cmake. If your target is an embedded
	# system without OS set CMAKE_SYSTEM_NAME to "Generic".
	set(CMAKE_SYSTEM_NAME Generic)

llext: additional CMake harmonizations #67997

llext: additional CMake harmonizations #67997

Conversation

pillo79 commented Jan 23, 2024 • edited Loading

pillo79 commented Jan 23, 2024

pillo79 commented Jan 23, 2024

lyakh commented Jan 24, 2024

pillo79 commented Jan 24, 2024 • edited Loading

tejlmand Jan 25, 2024

Choose a reason for hiding this comment

teburd Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

tejlmand Mar 11, 2024

Choose a reason for hiding this comment

tejlmand Jan 25, 2024

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

lyakh commented Jan 26, 2024

pillo79 commented Jan 31, 2024

pillo79 commented Feb 1, 2024

pillo79 commented Feb 1, 2024

pillo79 left a comment

Choose a reason for hiding this comment

pillo79 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

marc-hb commented Mar 19, 2024

pillo79 commented Mar 20, 2024 • edited Loading

teburd commented Mar 20, 2024

teburd commented Mar 26, 2024

pillo79 commented Apr 4, 2024

marc-hb left a comment

Choose a reason for hiding this comment

marc-hb Apr 5, 2024

Choose a reason for hiding this comment

lyakh commented Apr 8, 2024

pillo79 commented Jan 23, 2024 •

edited

Loading

pillo79 commented Jan 24, 2024 •

edited

Loading

teburd Jan 26, 2024 •

edited

Loading

pillo79 Mar 13, 2024 •

edited

Loading

pillo79 commented Mar 20, 2024 •

edited

Loading