Zephyr toolchain / linking improvements #78320

tejlmand · 2024-09-12T08:13:48Z

This is a series of linking improvements in Zephyr.

First part is moving linking of C and runtime libraries out away from zephyr_link_libraries().
Using zephyr_link_libraries() is vulnerable to link dependency and link ordering, which has been discussed multiple times in the past.
It has also resulted in CMake construct like this: zephyr_link_libraries(-lc) / zephyr_link_libraries(-lgcc) and thus making it harder to support non-gcc toolchains / libraries. This again has resulted in constructs like if(${ZEPHYR_TOOLCHAIN_VARIANT}) ...). All of this should be handled by Zephyr's toolchain infrastructure and normal CMake library creation should not need to know anything about C or runtime libraries, nor the toolchain in use.

By moving C / runtime library linking into the toolchain infrastructure and specify C / runtime libraries using CMake linker rules instead of zephyr_link_libraries() then we avoid CMake reordering the libraries, and CMake deduplication, and we ensure they are always linked at correct location.
To still allow Zephyr CMake code full flexibility and the use of Kconfig to select the libraries to use, then standard library selection has been moved to CMake linker properties, which provides a given set of defaults depending on the toolchain in use.
But the default can later be adjusted, based on other criteria.

This set of improvements allows for better LLVM support, especially when using LLVM with picolibc built-in support, but also when building picolibc as a module.

With this PR it is now possible to use LLVM for Arm for cross-compilation using both built-in Picolibc support as well as picolibc as module. LLVM for Arm with newlib is also supported.

LLVM for Arm:
https://github.com/ARM-software/LLVM-embedded-toolchain-for-Arm/releases/

In environment, set:

$ export ZEPHYR_TOOLCHAIN_VARIANT=llvm
$ export LLVM_TOOLCHAIN_PATH=/<path-to-LLVM-for_Arm_installation>/
# Example: export LLVM_TOOLCHAIN_PATH= /opt/LLVM-ET-Arm-18.1.3-Linux-x86_64/

To build Zephyr Hello world for an Arm target, such as nrf52840dk, using build-in picolibc support and lld with compiler rt, use:

$ west build -p -b nrf52840dk/nrf52840 samples/hello_world -- -DCONFIG_BUILD_NO_GAP_FILL=y -DCONFIG_COMPILER_RT_RTLIB=y -DCONFIG_LLVM_USE_LLD=y

Using picolibc as module:

$ west build -p -b nrf52840dk/nrf52840 samples/hello_world -- -DCONFIG_BUILD_NO_GAP_FILL=y -DCONFIG_COMPILER_RT_RTLIB=y -DCONFIG_LLVM_USE_LLD=y -DCONFIG_PICOLIBC_USE_MODULE=y

Using newlib as pre-built (remember to also install LLVM-ET-Arm-newlib-overlay-18.1.3.tar.xz ):

$ west build -p -b nrf52840dk/nrf52840 samples/hello_world -- -DCONFIG_BUILD_NO_GAP_FILL=y -DCONFIG_COMPILER_RT_RTLIB=y -DCONFIG_LLVM_USE_LLD=y -DCONFIG_NEWLIB_LIBC=y

As part of the LLVM improvement, then support for LLVM with C++ has also been improved, which means it's now possible to build the simply C++ hello world:

$ west build -p -b nrf52840dk/nrf52840 samples/cpp/hello_world -- -DCONFIG_BUILD_NO_GAP_FILL=y -DCONFIG_COMPILER_RT_RTLIB=y -DCONFIG_LLVM_USE_LLD=y
...
[137/137] Linking CXX executable zephyr/zephyr.elf
ld.lld: warning: /opt/LLVM-ET-Arm-18.1.3-Linux-x86_64/bin/../lib/clang-runtimes/arm-none-eabi/armv7em_soft_nofp_exn_rtti/lib/libc++abi.a(stdlib_new_delete.cpp.obj):(__lcxx_override) is being placed in '__lcxx_override'
ld.lld: warning: <internal>:(.eh_frame) is being placed in '.eh_frame'
ld.lld: warning: ignoring memory region assignment for non-allocatable section '.last_section'
Memory region         Used Size  Region Size  %age Used
           FLASH:      173484 B         1 MB     16.54%
             RAM:        8448 B       256 KB      3.22%
        IDT_LIST:          0 GB        32 KB      0.00%
Generating files from /projects/github/ncs/zephyr/build/zephyr/zephyr.elf for board: nrf52840dk

although some warnings are currently seen.

nordicjm · 2024-09-12T08:19:11Z

west.yml

@@ -315,7 +315,8 @@ manifest:
        - debug
    - name: picolibc
      path: modules/lib/picolibc
-      revision: 764ef4e401a8f4c6a86ab723533841f072885a5b
+      revision: b9fae840f916881cd0e36b6d9883300a3da7a52a
+#      revision: pull/<no>/head


zephyrbot · 2024-09-12T08:26:36Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
picolibc	zephyrproject-rtos/picolibc@`e15656f` (revert-zephyr-linker-fix)	zephyrproject-rtos/picolibc@`27746bb` (main)	zephyrproject-rtos/[email protected]
zephyr-lang-rust	zephyrproject-rtos/zephyr-lang-rust@`d2734f4`	zephyrproject-rtos/zephyr-lang-rust@`f20afb5` (main)	zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

tejlmand · 2024-09-12T09:27:54Z

@evgeniy-paltsev / @abrodkin can you please verify the changes with arc mwdt ?

keith-packard · 2024-09-12T16:42:56Z

It doesn't look like this solves the problem of placing libc/minimal and libc/common inside the whole-archive set of libs (as in #65434)? Should we try to make that work in this series or do you think that can be done after this lands?

evgeniy-paltsev · 2024-09-12T16:43:35Z

@tejlmand

evgeniy-paltsev / abrodkin can you please verify the changes with arc mwdt ?

Sure, I've started the verification so, I'll update about the results when they are ready. Thanks for the mentioning!

go2sh · 2024-09-12T18:37:29Z

I has the same problem with the tricore llvm toolchain. Will test this pr.

tejlmand · 2024-09-12T21:02:56Z

It doesn't look like this solves the problem of placing libc/minimal and libc/common inside the whole-archive set of libs (as in #65434)? Should we try to make that work in this series or do you think that can be done after this lands?

@keith-packard thanks for directing my attention to that PR.
You're correct that this PR is not trying to solve the whole-archive problem because my focus so far has been addressing some of the older and more fundamental library link issues we have had.

But I think that the strategy used in this PR will be beneficial for #65434, so let me take a look and see if I can cover #65434 or as minimum facilitate the work you have done in #65434

carlescufi · 2024-10-03T08:57:49Z

@keith-packard @stephanosio could you take another look?

stephanosio · 2024-10-03T09:00:44Z

Looks ok to me. zephyrproject-rtos/zephyr-lang-rust#9 needs to be merged before this can be merged.

The Zephyr PR zephyrproject-rtos/zephyr#78320 clean up and improves link handling in Zephyr. One improvement is the deterministic and correct link location of runtime and C library linking at the end of link arguments to ensure all libraries are able to use standard functions. However the librustapp.a library created by the cargo tool also contains runtime functions. It would in most cases be considered correct that a library, such as librustapp.a, which provides functions are linked first so that those functions provided as part of rust toolchain takes precedence over the runtime library provided by the C / C++ toolchain. However on the riscv32 / riscv64 architecture the rust prebuilt libraries makes use of relocation types in the clzsi2 object which are unknown to the ld linker in use. Thus linking librustapp.a first, for example like `librustapp.a -lgcc` results in the following warning (and thus CI failures): > <path>/ld.bfd: <path>/librustapp.a(45c91108d938afe8-clzdi2.o): > unsupported relocation type 0x3d Therefore we ensure that the runtime library is linked first, for example `-lgcc librustapp.a`. Signed-off-by: Torsten Rasmussen <[email protected]>

carlescufi · 2024-10-03T16:28:30Z

Looks ok to me. zephyrproject-rtos/zephyr-lang-rust#9 needs to be merged before this can be merged.

Merged @tejlmand

keith-packard

The picolibc changes look reasonable to me.

Update zephyr-lang-rust module to include commit for linking the runtime library before the librustapp.a. Signed-off-by: Torsten Rasmussen <[email protected]>

keith-packard · 2024-10-03T20:31:07Z

cmake/linker/ld/linker_libraries.cmake

+  # This is needed because when linking with newlib on aarch64, then libgcc has a
+  # link dependency to libc (strchr), but libc also has dependencies to libgcc.
+  # Lib C depends on libgcc. e.g. libc.a(lib_a-fvwrite.o) references __aeabi_idiv
+  set_property(TARGET linker APPEND PROPERTY link_order_library "math;c;rt;c")


Would it be reasonable to use --start-group -lc -lgcc --end-group instead of listing libc twice?

Personally I think that it's good to make it visible that there are indeed circular dependencies, and that we can see the number of loops.
--start/end-group are useful when you really start to have complex circular dependencies, but it easily turn into cargo-cult which no one dares to cleanup, and in future easily ends in a bunch of libs inside those flags.

As side-note, from the ld manual:

--start-group archives --end-group
...
Using this option has a significant performance cost. It is best to use it only when there are unavoidable circular references between two or more archives.

keith-packard · 2024-10-03T20:33:55Z

cmake/linker/lld/linker_libraries.cmake

+set_linker_property(NO_CREATE TARGET linker PROPERTY c_library "-lc")
+# Default per standard, will be populated by clang/target.cmake based on clang output.
+set_linker_property(NO_CREATE TARGET linker PROPERTY rt_library "")
+set_linker_property(TARGET linker PROPERTY c++_library "-lc++;-lc++abi")


should this mention math_library somehow? Either noting that it's not needed or setting to the empty string?

II don't think so.
The lib names are not fixed in that sense, a linker implementation can define extra libraries to always link in, as described here:

zephyr/cmake/linker/linker_libraries_template.cmake

Lines 9 to 10 in 3b8e621

# It is also possible to define extra libraries of the form `<name>_library`, and then include

# Fixed library search path can be defined in the `lib_include_dir` property if needed.

I will be working on a docset to describe the whole toolchain infrastructure and how to implement support for a custom one.
And there the math lib could be a good candidate to show.

keith-packard · 2024-10-03T20:54:56Z

cmake/compiler/compiler_flags_template.cmake

@@ -139,3 +139,7 @@ set_compiler_property(PROPERTY warning_shadow_variables)
 # Compiler flags to avoid recognizing built-in functions
 set_compiler_property(PROPERTY no_builtin)
 set_compiler_property(PROPERTY no_builtin_malloc)
+
+# Compiler flag for defining specs. Used only by gcc, other compilers may keep


Should we eventually plan on supporting clang's --config using this same mechanism?

I was thinking of that, and some clang/LLVM releases, like LLVM for Arm, does provide dedicated configs for this, but i'm unsure how well established / standardized this principle is, so for a start it's kept a bit independent in cmake/compiler/clang/target.cmake.
and also because it's needed for the existing compile check functionality.

keith-packard · 2024-10-03T21:19:58Z

cmake/compiler/clang/target.cmake

+  endif()
+
+  if(CONFIG_PICOLIBC)
+    file(GLOB_RECURSE picolibc_cfg ${LLVM_TOOLCHAIN_PATH}/picolibc.cfg)


maybe share the --specs mechanism somehow?

see other answer related to this. But good to have in mind for further improvements 👍

nashif · 2024-10-07T13:32:49Z

@tejlmand This change is breaking building with cadence xtensa toolchains:


): in function `intel_adsp_ipc_init':
/home/to/zephyrproject/zephyr/soc/intel/intel_adsp/common/ipc.c:101: undefined reference to `memset'
/home/nashif/xtensa/XtDevTools/install/tools/RI-2022.10-linux/XtensaTools/bin/xtensa-elf-ld: zephyr/kernel/libkernel.a(thread.c.obj):(.literal.z_impl_k_thread_name_set+0x0): undefined reference to `strncpy'
/home/nashif/xtensa/XtDevTools/install/tools/RI-2022.10-linux/XtensaTools/bin/xtensa-elf-ld: zephyr/kernel/libkernel.a(thread.c.obj): in function `z_impl_k_thread_name_set':
/home/to/zephyrproject/zephyr/kernel/thread.c:145: undefined reference to `strncpy'
/home/nashif/xtensa/XtDevTools/install/tools/RI-2022.10-linux/XtensaTools/bin/xtensa-elf-ld: zephyr/kernel/libkernel.a(thread.c.obj): in function `z_setup_new_thread':
/home/to/zephyrproject/zephyr/kernel/thread.c:610: undefined reference to `strncpy'
/home/nashif/xtensa/XtDevTools/install/tools/RI-2022.10-linux/XtensaTools/bin/xtensa-elf-ld: zephyr/kernel/libkernel.a(pipes.c.obj): in function `pipe_xfer':
/home/to/zephyrproject/zephyr/kernel/pipes.c:205: undefined reference to `memcpy'
/home/nashif/xtensa/XtDevTools/install/tools/RI-2022.10-linux/XtensaTools/bin/xtensa-elf-ld: /home/to/zephyrproject/zephyr/kernel/pipes.c:212: undefined reference to `memcpy'
clang-10: error: Xtensa-ld command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.
FATAL ERROR: command exited with status 1: /home/nashif/.local/bin/cmake --build /home/to/zephyrproject/zephyr/build --target run

Follow-up: zephyrproject-rtos#78320 Create linker_libraries.cmake for the Cadence Xtensa xt-ld linker to ensure correct linking of runtime and C libraries as well as correct link order. Signed-off-by: Torsten Rasmussen <[email protected]>

Follow-up: #78320 Create linker_libraries.cmake for the Cadence Xtensa xt-ld linker to ensure correct linking of runtime and C libraries as well as correct link order. Signed-off-by: Torsten Rasmussen <[email protected]>

marc-hb · 2024-10-10T23:19:02Z

cmake/linker/ld/target.cmake

    -L${PROJECT_BINARY_DIR}
-    ${TOOLCHAIN_LIBS}


Now -lhal is missing when using xt-xcc and compilation fails with undefined xthal_window_spill (xthal_window_spill is provided by the xt-xcc toolchain in libhal.a)

If I restore this specific line then xt-xcc compilation succeeds again.

I wonder why was this ${TOOLCHAIN_LIBS} line removed from SOME target.cmake files by commit 2e3873a but as of today's commit 27456ed it is still present in the following files? This seems inconsistent.

cmake/compiler/icx/target.cmake cmake/linker/arcmwdt/target.cmake cmake/linker/xt-ld/target.cmake

Indeed, if I remove ${TOOLCHAIN_LIBS} from cmake/linker/xt-ld/target.cmake then xt-clang fails with the same error.

cc: @kv2019i , @andyross, @cujomalainey, @dcpleung

FYI:

cmake/compiler/xcc/target.cmake:list(APPEND TOOLCHAIN_LIBS cmake/compiler/xcc/target.cmake- gcc cmake/compiler/xcc/target.cmake- hal cmake/compiler/xcc/target.cmake- )

an oversight.

I initially wanted to still allow for the old use of TOOLCHAIN_LIBS for toolchains I didn't have available, but later changes related to picolibc has actually resulted in the situation that all toolchains must be updated.

Thanks @tejlmand !

actually resulted in the situation that all toolchains must be updated.

While @dcpleung is removing the obsolete TOOLCHAIN_LIBS for Xtensa in #79731, as of today I see it still present in the main branch for cmake/compiler/icx/target.cmake and cmake/linker/arcmwdt/target.cmake. I think it would be good to file a bug and use git blame to warn the owners of these files that TOOLCHAIN_LIBS does not work anymore in case they still try and fail to use it downstream somehow. Something I did not mention before: it took me HOURS to debug this Xtensa compilation failure and find that it was down to this TOOLCHAIN_LIBS variable. Funny enough, git grep hal does not work very well! It would be nice to avoid others' a similar... effort.

In case these toolchains have no owner left, then it is a great opportunity to notice that and delete them entirely.

There is also a mysterious TOOLCHAIN_LIBS_OBJECTS in cmake/linker/armlink/target.cmake... My 2 cents.

zephyrbot added area: C Library C Standard Library area: Toolchains Toolchains area: Build System labels Sep 12, 2024

zephyrbot requested review from 57300, aescolar, cfriedt, jeremybettis, keith-packard, nashif, nordicjm and stephanosio September 12, 2024 08:14

zephyrbot assigned stephanosio Sep 12, 2024

nordicjm reviewed Sep 12, 2024

View reviewed changes

tejlmand mentioned this pull request Sep 12, 2024

cmake: linker: Use the same linker for cmake checks and final build #77666

Closed

tejlmand force-pushed the toolchain/linker_library_improvements branch from 6cda54c to bc7c49a Compare September 12, 2024 08:25

zephyrbot added manifest manifest-picolibc DNM This PR should not be merged (Do Not Merge) labels Sep 12, 2024

tejlmand requested review from carlescufi, evgeniy-paltsev, abrodkin and daor-oti September 12, 2024 09:26

tejlmand force-pushed the toolchain/linker_library_improvements branch from bc7c49a to cfe7a16 Compare September 12, 2024 12:48

tejlmand force-pushed the toolchain/linker_library_improvements branch from cfe7a16 to 2eba348 Compare September 12, 2024 20:55

carlescufi requested review from keith-packard and stephanosio October 3, 2024 08:56

keith-packard previously approved these changes Oct 3, 2024

View reviewed changes

manifest: update zephyr-lang-rust to always link runtime libs first

3b8e621

Update zephyr-lang-rust module to include commit for linking the runtime library before the librustapp.a. Signed-off-by: Torsten Rasmussen <[email protected]>

tejlmand dismissed keith-packard’s stale review via 3b8e621 October 3, 2024 19:18

tejlmand force-pushed the toolchain/linker_library_improvements branch from 2e7d667 to 3b8e621 Compare October 3, 2024 19:18

zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Oct 3, 2024

keith-packard reviewed Oct 3, 2024

View reviewed changes

keith-packard approved these changes Oct 3, 2024

View reviewed changes

cfriedt approved these changes Oct 3, 2024

View reviewed changes

stephanosio approved these changes Oct 4, 2024

View reviewed changes

fabiobaltieri merged commit 6fc8563 into zephyrproject-rtos:main Oct 4, 2024
25 of 26 checks passed

tejlmand mentioned this pull request Oct 7, 2024

cmake: add linker_libraries.cmake for Cadence Xtensa linker #79521

Merged

tejlmand mentioned this pull request Oct 8, 2024

manifest: update zephyr-lang-rust Zephyr module #79296

Closed

marc-hb reviewed Oct 10, 2024

View reviewed changes

marc-hb mentioned this pull request Oct 11, 2024

cmake: xcc: fix compilation issues #79731

Merged

axelnxp mentioned this pull request Nov 28, 2024

linker: ld: toolchain_linker_finalize duplicates <LINKER_FLAGS> leading to link issues in some situations #82281

Closed

tejlmand mentioned this pull request Dec 10, 2024

Remove WHOLE_ARCHIVE_LIBS in Zephyr #82780

Open

	# It is also possible to define extra libraries of the form `<name>_library`, and then include
	# Fixed library search path can be defined in the `lib_include_dir` property if needed.

Zephyr toolchain / linking improvements #78320

Zephyr toolchain / linking improvements #78320

Uh oh!

Conversation

tejlmand commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zephyrbot commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tejlmand commented Sep 12, 2024

Uh oh!

keith-packard commented Sep 12, 2024

Uh oh!

evgeniy-paltsev commented Sep 12, 2024

Uh oh!

go2sh commented Sep 12, 2024

Uh oh!

tejlmand commented Sep 12, 2024

Uh oh!

carlescufi commented Oct 3, 2024

Uh oh!

stephanosio commented Oct 3, 2024

Uh oh!

carlescufi commented Oct 3, 2024

Uh oh!

keith-packard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nashif commented Oct 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tejlmand commented Sep 12, 2024 •

edited

Loading

zephyrbot commented Sep 12, 2024 •

edited

Loading