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

llext: additional CMake harmonizations #67997

Merged
merged 4 commits into from
Apr 8, 2024
Merged

llext: additional CMake harmonizations #67997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2b6016
cmake: conditionally enable shared library support
pillo79 Mar 18, 2024
8c0a051
llext: use CMake shared library support on Xtensa
pillo79 Jan 22, 2024
947d3d0
llext: add llext_* configuration functions
pillo79 Feb 1, 2024
cf0b25e
llext: add support for build time custom commands
pillo79 Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion cmake/compiler/gcc/target_xtensa.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,4 @@ set(LLEXT_APPEND_FLAGS
-fPIC
-nostdlib
-nodefaultlibs
-shared
)
10 changes: 7 additions & 3 deletions cmake/modules/FindTargetTools.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ endif()
set(CMAKE_C_COMPILER_FORCED 1)
set(CMAKE_CXX_COMPILER_FORCED 1)

# No official documentation exists for the "Generic" value, except their wiki.
# https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_NAME.html:
# The name of the operating system for which CMake is to build.
#
# https://gitlab.kitware.com/cmake/community/wikis/doc/cmake/CrossCompiling:
# CMAKE_SYSTEM_NAME : this one is mandatory, it is the name of the target
Expand All @@ -40,7 +41,10 @@ set(CMAKE_CXX_COMPILER_FORCED 1)
# 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)
#
# This will force CMake to load cmake/modules/Platform/Zephyr.cmake,
# allowing Zephyr-specific embedded system features to be enabled.
set(CMAKE_SYSTEM_NAME Zephyr)

# https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_PROCESSOR.html:
# The name of the CPU CMake is building for.
Expand Down Expand Up @@ -74,7 +78,7 @@ else()
set(CMAKE_CXX_BYTE_ORDER LITTLE_ENDIAN)
endif()

# We are not building dynamically loadable libraries
# Do not build dynamically loadable libraries by default
set(BUILD_SHARED_LIBS OFF)

# Custom targets for compiler and linker flags.
Expand Down
13 changes: 13 additions & 0 deletions cmake/modules/Platform/Zephyr.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# SPDX-License-Identifier: Apache-2.0
#
# Copyright (c) 2024, Arduino SA

# Perform the same initialization as the Generic platform, then enable
# dynamic library support if CONFIG_LLEXT is enabled.

include(Platform/Generic)

# Enable dynamic library support when CONFIG_LLEXT is enabled.
if(CONFIG_LLEXT)
set_property(GLOBAL PROPERTY TARGET_SUPPORTS_SHARED_LIBS true)
endif()
255 changes: 209 additions & 46 deletions cmake/modules/extensions.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ include(CheckCXXCompilerFlag)
# 5.1. zephyr_linker*
# 6 Function helper macros
# 7 Linkable loadable extensions (llext)
# 7.1 llext_* configuration functions
# 7.2 add_llext_* build control functions

########################################################
# 1. Zephyr-aware extensions
Expand Down Expand Up @@ -5099,6 +5101,44 @@ endmacro()
# loadable extensions (llexts).
#

# 7.1 Configuration functions
#
# The following functions simplify access to the compilation/link stage
# properties of an llext using the same API of the target_* functions.
#

function(llext_compile_definitions target_name)
target_compile_definitions(${target_name}_llext_lib PRIVATE ${ARGN})
endfunction()

function(llext_compile_features target_name)
target_compile_features(${target_name}_llext_lib PRIVATE ${ARGN})
endfunction()

function(llext_compile_options target_name)
target_compile_options(${target_name}_llext_lib PRIVATE ${ARGN})
endfunction()

function(llext_include_directories target_name)
target_include_directories(${target_name}_llext_lib PRIVATE ${ARGN})
endfunction()

function(llext_link_options target_name)
target_link_options(${target_name}_llext_lib PRIVATE ${ARGN})
endfunction()

# 7.2 Build control functions
#
# The following functions add targets and subcommands to the build system
# to compile and link an llext.
#

# Usage:
# add_llext_target(<target_name>
# OUTPUT <output_file>
# SOURCES <source_file>
# )
#
# Add a custom target that compiles a single source file to a .llext file.
#
# Output and source files must be specified using the OUTPUT and SOURCES
Expand All @@ -5108,49 +5148,45 @@ endmacro()
# in the Zephyr build, but with some important modifications. The list of
# flags to remove and flags to append is controlled respectively by the
# 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.
#
# The following custom properties of <target_name> are defined and can be
# retrieved using the get_target_property() function:
#
# - lib_target Target name for the source compilation and/or link step.
# - lib_output The binary file resulting from compilation and/or
# linking steps.
# - pkg_input The file to be used as input for the packaging step.
# - pkg_output The final .llext file.
#
# Example usage:
# add_llext_target(hello_world
# OUTPUT ${PROJECT_BINARY_DIR}/hello_world.llext
# SOURCES ${PROJECT_SOURCE_DIR}/src/llext/hello_world.c
# C_FLAGS -Werror
# )
# will compile the source file src/llext/hello_world.c to a file
# ${PROJECT_BINARY_DIR}/hello_world.llext, adding -Werror to the compilation.
# named "${PROJECT_BINARY_DIR}/hello_world.llext".
#
function(add_llext_target target_name)
set(single_args OUTPUT)
set(multi_args SOURCES;C_FLAGS)
set(multi_args SOURCES)
cmake_parse_arguments(PARSE_ARGV 1 LLEXT "${options}" "${single_args}" "${multi_args}")

# Check that the llext subsystem is enabled for this build
if (NOT CONFIG_LLEXT)
message(FATAL_ERROR "add_llext_target: CONFIG_LLEXT must be enabled")
endif()

# Output file must be provided
if(NOT LLEXT_OUTPUT)
message(FATAL_ERROR "add_llext_target: OUTPUT argument must be provided")
endif()
# Source and output files must be provided
zephyr_check_arguments_required_all("add_llext_target" LLEXT OUTPUT SOURCES)

# Source list length must currently be 1
list(LENGTH LLEXT_SOURCES source_count)
if(NOT source_count EQUAL 1)
message(FATAL_ERROR "add_llext_target: only one source file is supported")
endif()

set(output_file ${LLEXT_OUTPUT})
set(llext_pkg_output ${LLEXT_OUTPUT})
set(source_file ${LLEXT_SOURCES})
get_filename_component(output_name ${output_file} NAME)

# Add user-visible target and dependency
add_custom_target(${target_name}
COMMENT "Compiling ${output_name}"
DEPENDS ${output_file}
)

# Convert the LLEXT_REMOVE_FLAGS list to a regular expression, and use it to
# filter out these flags from the Zephyr target settings
Expand All @@ -5166,62 +5202,189 @@ function(add_llext_target target_name)
"$<FILTER:${zephyr_flags},EXCLUDE,${llext_remove_flags_regexp}>"
)

# Compile the source file to an object file using current Zephyr settings
# but a different set of flags
add_library(${target_name}_lib OBJECT ${source_file})
target_compile_definitions(${target_name}_lib PRIVATE
# Compile the source file using current Zephyr settings but a different
# set of flags.
# This is currently arch-specific since the ARM loader for .llext files
# expects object file format, while the Xtensa one uses shared libraries.
set(llext_lib_target ${target_name}_llext_lib)
if(CONFIG_ARM)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this and the elseif(CONFIG_XTENSA) below are really not something we should be checking in a common function.

I notice it was introduced in #67431 to support shared libraries for the xtensa arch, but now it seems to be spreading more, and that is not good.

So while this is good:

This change minimizes the differences in llext handling by defining
appropriate CMake targets for the different architectures.

then having (too much) CONFIG_<feature> is a bad sign.

Copy link
Collaborator

@marc-hb marc-hb Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

then having (too much) CONFIG_ is a bad sign.

It is not good but "spreading" is IMHO slightly exaggerated and @pillo79 has committed above to fix this as soon as the corresponding C code is fixed - if nothing else for the simple reason that this conditional makes his own maintenance work harder.

The LLEXT paint is still wet and we need more early adopters (like SOF) to starting using LLEXT in anger and provide feedback to make progress. One thing slowing down adoption and progress has been (as often) the lack of a build system, forcing everyone to write their own, different, custom build script. So @pillo79 stepped up and spent an unbelievable amount of time and patience addressing that gap in the CMake area where extremely few people ever volunteer. Blocking the (extension of) this small, documented CMake hack can re-inforce a chicken-and-egg situation where the corresponding C code lacks users and does not get fixed either.

From experience I know very well that "Fix it later" usually means either "never" or "someone else can do it" but I think this case deserves an exception because:

  • LLEXT is still at the (late) prototype stage so there's barely any user base and nothing "stable" to break yet.
  • It's not even a new hack, it's already there
  • @pillo79 's dedication is a good indication we can trust him to actually fix this later
  • we have no one else :-D

cc: @selescop, @bjarki-trackunit

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not good but "spreading" is IMHO slightly exaggerated

So which better word would you have in mind to describe the fact that the if(CONFIG_...) was used in one location before, and now two more locations are added ?
Or should I not point to the fact that such testing is not desired ?

Also, pointing at this could even have @pillo79 taking a second look at the code to see if this could be combined under the existing single if(CONFIG_...) test.

Remember, others may even look at this code and think this is an acceptable way, but for such cases it's always good to have review comments to reference and stating, yes, but his pattern was accepted under specific conditions, see PR xyz.

The LLEXT paint is still wet and we need more early adopters (like SOF) to starting using LLEXT in anger and provide feedback to make progress.

and I fully agree, and also a reason why I very early suggested that llext functions were made first class citizen and placed in extensions.cmake. But with that we should also require a bit higher level compared to code in a Zephyr module 🙂

@pillo79 's dedication is a good indication we can trust him to actually fix this later

and did I anywhere indicate that this was not the case ?

Copy link
Collaborator

@tejlmand tejlmand Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@marc-hb the fact that something is observed in a PR doesn't mean that it cannot be accepted if reasonable explanation / answers are given.

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 "spreading" is due to the fact that build stages are now clearly separated, thus there are multiple (but closely packed) locations in which I need arch-specific behavior. I could probably minimize it somehow, but I chose readability over minimum code.

We can and probably will get rid of the arch-specific tags by defining behavior-specific Kconfigs (for example an enum like CONFIG_LLEXT_TYPE_DYNAMIC/OBJECT). But this is definitely out of scope of this PR, which is not changing the "hack", just moving it around to make it more explicit.


# Create an object library to compile the source file
add_library(${llext_lib_target} OBJECT ${source_file})
set(llext_lib_output $<TARGET_OBJECTS:${llext_lib_target}>)

elseif(CONFIG_XTENSA)

# Create a shared library
add_library(${llext_lib_target} SHARED ${source_file})
set(llext_lib_output $<TARGET_FILE:${llext_lib_target}>)

# Add the llext flags to the linking step as well
target_link_options(${llext_lib_target} PRIVATE
${LLEXT_APPEND_FLAGS}
pillo79 marked this conversation as resolved.
Show resolved Hide resolved
)

endif()

target_compile_definitions(${llext_lib_target} PRIVATE
$<TARGET_PROPERTY:zephyr_interface,INTERFACE_COMPILE_DEFINITIONS>
)
target_compile_options(${target_name}_lib PRIVATE
target_compile_options(${llext_lib_target} PRIVATE
${zephyr_filtered_flags}
${LLEXT_APPEND_FLAGS}
${LLEXT_C_FLAGS}
)
target_include_directories(${target_name}_lib PRIVATE
target_include_directories(${llext_lib_target} PRIVATE
$<TARGET_PROPERTY:zephyr_interface,INTERFACE_INCLUDE_DIRECTORIES>
)
target_include_directories(${target_name}_lib SYSTEM PUBLIC
target_include_directories(${llext_lib_target} SYSTEM PUBLIC
$<TARGET_PROPERTY:zephyr_interface,INTERFACE_SYSTEM_INCLUDE_DIRECTORIES>
)
add_dependencies(${target_name}_lib
add_dependencies(${llext_lib_target}
zephyr_interface
zephyr_generated_headers
)

# Arch-specific conversion of the object file to an llext
# Set up an intermediate processing step between compilation and packaging
# to be used to support POST_BUILD commands on targets that do not use a
# dynamic library.
set(llext_proc_target ${target_name}_llext_proc)
set(llext_pkg_input ${PROJECT_BINARY_DIR}/${target_name}.llext.pkg_input)
add_custom_target(${llext_proc_target} DEPENDS ${llext_pkg_input})
set_property(TARGET ${llext_proc_target} PROPERTY has_post_build_cmds 0)

# By default this target must copy the `lib_output` binary file to the
# expected `pkg_input` location. If actual POST_BUILD commands are defined,
# they will take care of this and the default copy is replaced by a no-op.
set(has_post_build_cmds "$<TARGET_PROPERTY:${llext_proc_target},has_post_build_cmds>")
set(noop_cmd ${CMAKE_COMMAND} -E true)
set(copy_cmd ${CMAKE_COMMAND} -E copy ${llext_lib_output} ${llext_pkg_input})
add_custom_command(
OUTPUT ${llext_pkg_input}
COMMAND "$<IF:${has_post_build_cmds},${noop_cmd},${copy_cmd}>"
DEPENDS ${llext_lib_target} ${llext_lib_output}
COMMAND_EXPAND_LISTS
)

# Arch-specific packaging of the built binary file into an .llext file
if(CONFIG_ARM)

# No conversion required, simply copy the object file
# No packaging required, simply copy the object file
add_custom_command(
OUTPUT ${output_file}
COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_OBJECTS:${target_name}_lib> ${output_file}
DEPENDS ${target_name}_lib $<TARGET_OBJECTS:${target_name}_lib>
OUTPUT ${llext_pkg_output}
COMMAND ${CMAKE_COMMAND} -E copy ${llext_pkg_input} ${llext_pkg_output}
DEPENDS ${llext_proc_target} ${llext_pkg_input}
)

elseif(CONFIG_XTENSA)

# Generate an intermediate file name
get_filename_component(output_dir ${output_file} DIRECTORY)
get_filename_component(output_name_we ${output_file} NAME_WE)
set(pre_output_file ${output_dir}/${output_name_we}.pre.llext)

# Need to convert the object file to a shared library, then strip some sections
# Need to strip the shared library of some sections
add_custom_command(
OUTPUT ${output_file}
BYPRODUCTS ${pre_output_file}
COMMAND ${CMAKE_C_COMPILER} ${LLEXT_APPEND_FLAGS}
-o ${pre_output_file}
$<TARGET_OBJECTS:${target_name}_lib>
OUTPUT ${llext_pkg_output}
COMMAND $<TARGET_PROPERTY:bintools,strip_command>
$<TARGET_PROPERTY:bintools,strip_flag>
$<TARGET_PROPERTY:bintools,strip_flag_remove_section>.xt.*
$<TARGET_PROPERTY:bintools,strip_flag_infile>${pre_output_file}
$<TARGET_PROPERTY:bintools,strip_flag_outfile>${output_file}
$<TARGET_PROPERTY:bintools,strip_flag_infile>${llext_pkg_input}
$<TARGET_PROPERTY:bintools,strip_flag_outfile>${llext_pkg_output}
$<TARGET_PROPERTY:bintools,strip_flag_final>
DEPENDS ${target_name}_lib $<TARGET_OBJECTS:${target_name}_lib>
DEPENDS ${llext_proc_target} ${llext_pkg_input}
)

else()
message(FATAL_ERROR "add_llext_target: unsupported architecture")
endif()

# Add user-visible target and dependency, and fill in properties
get_filename_component(output_name ${llext_pkg_output} NAME)
add_custom_target(${target_name}
COMMENT "Generating ${output_name}"
DEPENDS ${llext_pkg_output}
)
set_target_properties(${target_name} PROPERTIES
lib_target ${llext_lib_target}
lib_output ${llext_lib_output}
pkg_input ${llext_pkg_input}
pkg_output ${llext_pkg_output}
)
endfunction()

# Usage:
# add_llext_command(
# TARGET <target_name>
# PRE_BUILD | POST_BUILD | POST_PKG
# COMMAND <command> [...]
# )
#
# Add a custom command to an llext target that will be executed during
# 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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

# - PRE_BUILD: Before the llext code is linked, if the architecture uses
# dynamic libraries. This step can access `lib_target` and
# its own properties.
# - POST_BUILD: After the llext code is built, but before packaging
# it in an .llext file. This step is expected to create a
# `pkg_input` file by reading the contents of `lib_output`.
# - POST_PKG: After the .llext file has been created. This can operate on
# the final llext file `pkg_output`.
#
# Anything else after COMMAND will be passed to add_custom_command() as-is
# (including multiple commands and other options).
function(add_llext_command)
set(options PRE_BUILD POST_BUILD POST_PKG)
set(single_args TARGET)
# COMMAND and other options are passed to add_custom_command() as-is

cmake_parse_arguments(PARSE_ARGV 0 LLEXT "${options}" "${single_args}" "${multi_args}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

curious, why the PARSE_ARGV 0 in this case ?

Are we expecting special characters in the arguments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly? What lies after COMMAND is really open to the user - I thought doing it like this gave maximum flexibility (and avoided compatibility issues as much as possible).

zephyr_check_arguments_required_all("add_llext_command" LLEXT TARGET)

# Check the target exists and refers to an llext target
set(target_name ${LLEXT_TARGET})
set(llext_lib_target ${target_name}_llext_lib)
set(llext_proc_target ${target_name}_llext_proc)
if(NOT TARGET ${llext_lib_target})
message(FATAL_ERROR "add_llext_command: not an llext target: ${target_name}")
endif()

# ARM uses an object file representation so there is no link step.
if(CONFIG_ARM AND LLEXT_PRE_BUILD)
message(FATAL_ERROR
"add_llext_command: PRE_BUILD not supported on this arch")
endif()

# Determine the build step and the target to attach the command to
# based on the provided options
if(LLEXT_PRE_BUILD)
# > before the object files are linked:
# - execute user command(s) before the lib target's link step.
set(cmd_target ${llext_lib_target})
set(build_step PRE_LINK)
elseif(LLEXT_POST_BUILD)
# > after linking, but before llext packaging:
# - stop default file copy to prevent user files from being clobbered;
# - execute user command(s) after the (now empty) `llext_proc_target`.
set_property(TARGET ${llext_proc_target} PROPERTY has_post_build_cmds 1)
set(cmd_target ${llext_proc_target})
set(build_step POST_BUILD)
elseif(LLEXT_POST_PKG)
# > after the final llext binary is ready:
# - execute user command(s) after the main target is done.
set(cmd_target ${target_name})
set(build_step POST_BUILD)
else()
message(FATAL_ERROR "add_llext_command: build step must be provided")
endif()

# Check that the first unparsed argument is the word COMMAND
list(GET LLEXT_UNPARSED_ARGUMENTS 0 command_str)
if(NOT command_str STREQUAL "COMMAND")
message(FATAL_ERROR "add_llext_command: COMMAND argument must be provided")
marc-hb marked this conversation as resolved.
Show resolved Hide resolved
endif()

# Add the actual command(s) to the target
add_custom_command(
TARGET ${cmd_target} ${build_step}
${LLEXT_UNPARSED_ARGUMENTS}
COMMAND_EXPAND_LISTS
)
endfunction()
Loading
Loading