cmake: Kernel and offset libraries linking flags #8445

tejlmand · 2018-06-18T08:21:41Z

This commit moves linking flags which are required when linking to
kernel and offset static libraries to be defined together with the
library definitions.

Thus, the library kernel / offset propagates the flags which are
required when linking to those libraries.
This places knowledge where it correctly belongs.

Signed-off-by: Torsten Rasmussen torsten.rasmussen@nordicsemi.no

SebastianBoe

This must be split up into smaller independent commits.

Re-organizing how the linking is done may result in obscure and hard-to-debug issues. It is therefore important to have a bisectable history to find the minimal patch that caused the regression.

Will review when the git history is rewritten.

SebastianBoe · 2018-06-18T10:34:37Z

kernel/CMakeLists.txt


 add_dependencies(kernel offsets_h)

-target_link_libraries(kernel zephyr_interface)
+target_link_libraries(kernel PUBLIC zephyr_interface)
+target_link_libraries(kernel INTERFACE ${E_KERNEL_ENTRY} ${TOOLCHAIN_LIBS})


Seems to me that TOOLCHAIN_LIBS is not specific to the kernel, but rather something that all Zephyr sources need. So it should be

sebo@mach:~/zephyr$ sebo@mach:~/zephyr$ git diff | cat diff --git a/CMakeLists.txt b/CMakeLists.txt index 1b66239f70..166075e087 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -693,6 +693,8 @@ set_property(TARGET get_property(E_KERNEL_ENTRY GLOBAL PROPERTY E_KERNEL_ENTRY) +zephyr_link_libraries(${TOOLCHAIN_LIBS}) + set(zephyr_lnk ${LINKERFLAGPREFIX},-Map=${PROJECT_BINARY_DIR}/${KERNEL_MAP_NAME} ${LINKERFLAGPREFIX},--start-group diff --git a/kernel/CMakeLists.txt b/kernel/CMakeLists.txt index 04111eaf6e..ae6de19ff6 100644 --- a/kernel/CMakeLists.txt +++ b/kernel/CMakeLists.txt @@ -57,5 +57,5 @@ get_property(E_KERNEL_ENTRY GLOBAL PROPERTY E_KERNEL_ENTRY) add_dependencies(kernel offsets_h) target_link_libraries(kernel PUBLIC zephyr_interface) -target_link_libraries(kernel INTERFACE ${E_KERNEL_ENTRY} ${TOOLCHAIN_LIBS}) +target_link_libraries(kernel INTERFACE ${E_KERNEL_ENTRY})

Point taken.
Smaller commits but single PR.

I added the TOOLCHAIN_LIBS to the kernel, as it appeared to me this was the lowest part of the Zephyr system which requires the TOOLCHAIN_LIBS.

From there, the TOOLCHAIN_LIBS is propagated to other who depends on kernel, as does libzephyr.a, and the rest depends on zephyr.

Normally I would have placed such flag on the CMAKE_EXE_LINKER_FLAGS, but this is not used with Zephyr, thus I found kernel linking dependency to be the second best option.
Of course this is open to other opinions.

Please note, the flag is not required for compiling, only linking.
So the all Zephyr sources doesn't need it.

This use-case is covered by zephyr_link_libraries().

So you either have to use zephyr_link_libaries, or block this PR on another PR that deprecates zephyr_link_libraries().

Well in that case, the zephyr_interface description should be
updated, as this is a link flag, which much be used by the
executable that links and not directly matching the description in
CMakeLists.txt today about zephyr_interface.
# zephyr_interface is a source-less library that has all the global
# compiler options needed by all source files. All zephyr libraries,
# including the library named "zephyr" link with this library to
# obtain these flags.

I can update the description and move the link flag here instead.

kernel/CMakeLists.txt

@@ -52,7 +52,10 @@ target_sources_ifdef(
  userspace.c
  )

+get_property(E_KERNEL_ENTRY GLOBAL PROPERTY E_KERNEL_ENTRY)


tejlmand · 2018-06-20T06:38:45Z

@SebastianBoe Did you see the new push ?

SebastianBoe · 2018-06-20T10:23:38Z

I did now :)

Will review when I have time.

SebastianBoe · 2018-06-22T13:28:01Z

CMakeLists.txt

@@ -40,7 +40,11 @@ set_property(   GLOBAL PROPERTY PROPERTY_OUTPUT_FORMAT elf32-little${ARCH})
 # compiler options needed by all source files. All zephyr libraries,


There is something wrong with the commit message.

It cuts off mid-sentence.

well, you wrote that comment yourself in commit 12f8f76, so I can't guess what your meaning were.
I only appended the text with linker flags appended afterwards.

But isn't the text just a new line and the sentence continues.
At least that's how I read it:
All zephyr libraries, including the library named "zephyr" link with this library to obtain these flags.

Commit message, not comment.

Ahh, ok.

I think the reason might be the reference to issue number was #8440 was moved to new line, due to 72 char limit.
But # at line start comments out the line :-s

SebastianBoe · 2018-06-22T13:32:13Z

CMakeLists.txt

 add_library(zephyr_interface INTERFACE)
+target_link_libraries(zephyr_interface INTERFACE ${TOOLCHAIN_LIBS})


The zephyr_link_libraries() API should be used when a linker flag needs to be added to all Zephyr sources.

so

zephyr_link_libraries(${TOOLCHAIN_LIBS})

SebastianBoe · 2018-06-25T10:10:33Z

CMakeLists.txt

@@ -464,7 +464,7 @@ set(OFFSETS_O_PATH ${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/offsets.dir/arch/${ARC
 set(OFFSETS_H_PATH ${PROJECT_BINARY_DIR}/include/generated/offsets.h)

 add_library(          offsets STATIC ${OFFSETS_C_PATH})
-target_link_libraries(offsets zephyr_interface)
+target_link_libraries(offsets zephyr_interface -u_OffsetAbsSyms -u_ConfigAbsSyms )


AFAICT _ConfigAbsSyms is not related to 'offsets', it is related to configs.c.

you're right.
and configs.c is a generated file which is placed inside libzephyr.a, which means the flag should go to zephyr lib and not even zephyr_interface.
Cause the flag is needed when you link to zephyr lib, not because you use zephyr_interface.
Other libraries should know about this flag during compilation, the flag is only required when linking the zephyr lib.

Example:
add_library(MyLib ....)
target_link_libraries(MyLib zephyr_interface)
add_executable(MyProgram)
target_link_libraries(MyProgram MyLib) # The flag is not needed cause libzephyr.a is not linked in this case.

but doing:
target_link_libraries(MyProgram MyLib zephyr) # Requires the flag when linking to zephyr, hence, Zephyr lib is the correct place for the flag

SebastianBoe · 2018-06-25T10:21:26Z

CMakeLists.txt

@@ -464,7 +464,7 @@ set(OFFSETS_O_PATH ${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/offsets.dir/arch/${ARC
 set(OFFSETS_H_PATH ${PROJECT_BINARY_DIR}/include/generated/offsets.h)

 add_library(          offsets STATIC ${OFFSETS_C_PATH})
-target_link_libraries(offsets zephyr_interface)
+target_link_libraries(offsets zephyr_interface -u_OffsetAbsSyms -u_ConfigAbsSyms )


This is out-of-scope so it doesn't have to block the PR, but if you wish you could instead of moving the -u_OffsetAbsSyms flag assist me in getting rid of it.

#8547

SebastianBoe · 2018-06-25T10:31:57Z

kernel/CMakeLists.txt


 add_dependencies(kernel offsets_h)

-target_link_libraries(kernel zephyr_interface)
+target_link_libraries(kernel PUBLIC zephyr_interface)
+target_link_libraries(kernel INTERFACE ${E_KERNEL_ENTRY})


It is better to get rid of this flag than to move it around. This PR will need to be updated when #8534 is merged.

Agree. If the flag can be removed and the functionality be part of the linker script, that is much better.
But until then, then flag would more correctly be a link flag propagated from kernel lib to those who links to it, i.e. all applications.

SebastianBoe · 2018-06-25T10:34:51Z

kernel/CMakeLists.txt


 add_dependencies(kernel offsets_h)

-target_link_libraries(kernel zephyr_interface)
+target_link_libraries(kernel PUBLIC zephyr_interface)


Changing the target_link_libraries signature of 'kernel' should be in it's own commit with an accompanying commit message that explains the reprecussions in case it breaks any code that does:

target_link_libraries(my_lib kernel)

that code needs to be ported to

target_link_libraries(my_lib PUBLIC kernel)

I don't expect it to break anything, as I can't imagine a reason users would be linking with the kernel, but it is best to be safe (and to separate changes),

Why would it need to do that ?
From:
https://cmake.org/cmake/help/v3.12/command/target_link_libraries.html

Library dependencies are transitive by default with this signature. When this target is linked into another target then the libraries linked to this target will appear on the link line for the other target too. This transitive “link interface” is stored in the INTERFACE_LINK_LIBRARIES target property and may be overridden by setting the property directly.

Because of a bug in CMake that they made into a feature ;)

https://cmake.org/cmake/help/v3.0/policy/CMP0023.html

Yes, but placing PUBLIC on the kernel library doesn't enforce that linking my_lib to kernel require other my_lib to start using PUBLIC, as the lines:

target_link_libraries(my_lib kernel)
that code needs to be ported to
target_link_libraries(my_lib PUBLIC kernel)

present.
target_link_libraries(my_lib kernel)
will still behave the same as before when PUBLIC is placed on kernel.

Correct.

I am not trying to say that this will transitively affect all other libraries.

I am trying to say that if someone has been doing

target_link_libraries(kernel some_lib)

then they will get an error until they port their code to look like this:

target_link_libraries(kernel PUBLIC some_lib).

Hmm, on second thought, I guess this is safe, because you can only modify kernel from the same CMakeLists.txt file (directory) that created it, which is zephyr/CMakeLists.txt.

I guess we are lucky we are using 'add_subdirectory(kernel)' and not 'include(kernel)' in this case ;)

EDIT: I see that I caused the confusion by writing the arguments the wrong way in the original comment. Sorry about that.

ok, now your comment makes much more sense.
and this is exactly why CMakeLists.txt files should be used in COMBINATION with include.
As I show in #8451

Libraries/subsystems, such as kernel, net, bluetooth, etc. should still be included using add_subdirectory, but INSIDE such library, the use of include([<subdir>/]<file>.cmake) can give a much better handling of the lib.

So the use of add_subdirectory in this case is not luck.

And btw. the library itself can still be modified, i.e.
you can do:
target_sources(kernel PRIVATE <somefile.c>) anywhere in the CMake tree, regardless of whether you use include or add_subdirectory.
add_subdirectory prevents only the use of:
target_link_libraries(kernel ...)
outside of the CMakeLists.txt directory.

SebastianBoe · 2018-06-25T10:40:43Z

CMakeLists.txt

@@ -50,6 +50,7 @@ target_link_libraries(zephyr_interface INTERFACE ${TOOLCHAIN_LIBS})
 # built purely with the include paths, defines, and other compiler
 # flags that come with zephyr_interface.
 zephyr_library_named(zephyr)
+target_link_libraries(zephyr kernel)


It seems artificial to link 'zephyr' with 'kernel'. Why the library 'zephyr' and not the library 'app'?

And why just one Zephyr CMake library ('zephyr') and not all of them?

perhaps

zephyr_link_libraries(kernel)

would be more appropriate.

SebastianBoe · 2018-06-25T10:42:50Z

CMakeLists.txt

@@ -50,7 +50,7 @@ target_link_libraries(zephyr_interface INTERFACE ${TOOLCHAIN_LIBS})
 # built purely with the include paths, defines, and other compiler
 # flags that come with zephyr_interface.
 zephyr_library_named(zephyr)
-target_link_libraries(zephyr kernel)
+target_link_libraries(zephyr kernel offsets)



Same comment as with e3cec73#r197751936

I'm not sure I fully agree here.
Currently it appears to me that not all libraries has a dependency to offsets.
And thus, it is not a good principle to place it on zephyr_interface which everyone is using.

For the kernel lib, things are different, as it appears that most, if not all, libraries depends upon kernel.

Will be looking bit more into offsets, but if anyone can point to a good description on who really depends on offsets, that would be nice.

Fixes: zephyrproject-rtos#8440 This commit move the link flag ${TOOLCHAIN_LIBS} from the global list to zephyr_interface and -u_ConfigAbsSyms to the zephyr library in order to give a better structure as described in zephyrproject-rtos#8440 Signed-off-by: Torsten Rasmussen <torsten.rasmussen@nordicsemi.no>

Fixes: zephyrproject-rtos#8440 This commit moves kernel from the list in <zephyr_base>/CMakeLists.txt and place it as a link dependency when linking to zephyr_interface. It also links kernel to offsets to ensure correct link dependencies. Signed-off-by: Torsten Rasmussen <torsten.rasmussen@nordicsemi.no>

Fixes: zephyrproject-rtos#8440 This commit moves the link flag -u_OffsetAbsSyms which is required when linking to offset static library. The library offset thus propagates the flags correctly during linking. It also ensures knowledge about link flags is present where it correctly belongs. Signed-off-by: Torsten Rasmussen <torsten.rasmussen@nordicsemi.no>

ioannisg · 2019-06-22T22:09:04Z

@tejlmand any plans to work on this one? Is this still applicable?

aescolar · 2019-09-18T15:35:44Z

Closing as there has been no response for 3 months now.
@tejlmand Please reopen if it is still relevant.

tejlmand requested review from andrewboie, andyross, nashif and SebastianBoe as code owners June 18, 2018 08:21

This comment was marked as outdated.

Sign in to view

SebastianBoe requested changes Jun 18, 2018

View reviewed changes

tejlmand force-pushed the link_dependency_cleanup branch 2 times, most recently from 74b82c0 to 884e153 Compare June 18, 2018 16:25

SebastianBoe reviewed Jun 22, 2018

View reviewed changes

SebastianBoe requested changes Jun 25, 2018

View reviewed changes

SebastianBoe reviewed Jun 25, 2018

View reviewed changes

tejlmand mentioned this pull request Jun 25, 2018

[DNM] LD: Don't force a reference to the symbol _OffsetAbsSyms #8547

Closed

tejlmand force-pushed the link_dependency_cleanup branch 3 times, most recently from 119aba6 to 32ce538 Compare July 4, 2018 07:12

tejlmand force-pushed the link_dependency_cleanup branch from 32ce538 to dc3a65c Compare July 4, 2018 08:24

tejlmand added 2 commits July 4, 2018 11:13

tejlmand force-pushed the link_dependency_cleanup branch from dc3a65c to 99856db Compare July 4, 2018 09:18

This was referenced Jul 4, 2018

cmake: Zephyr wrapped functions does not allow keywords on zephyr_link_libraries #8614

Closed

CMake overhaul and architecture #8827

Open

ioannisg added the Stale PR label Jun 22, 2019

aescolar added the area: Build System label Sep 18, 2019

aescolar closed this Sep 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Kernel and offset libraries linking flags #8445

cmake: Kernel and offset libraries linking flags #8445

tejlmand commented Jun 18, 2018

This comment was marked as outdated.

SebastianBoe left a comment

SebastianBoe Jun 18, 2018

tejlmand Jun 18, 2018

tejlmand Jun 18, 2018

SebastianBoe Jun 18, 2018 •

edited

Loading

tejlmand Jun 18, 2018 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

tejlmand commented Jun 20, 2018

SebastianBoe commented Jun 20, 2018

SebastianBoe Jun 22, 2018

tejlmand Jun 22, 2018 •

edited

Loading

SebastianBoe Jun 25, 2018

tejlmand Jun 25, 2018

SebastianBoe Jun 22, 2018

SebastianBoe Jun 25, 2018

tejlmand Jun 27, 2018

SebastianBoe Jun 25, 2018

SebastianBoe Jun 25, 2018

tejlmand Jun 27, 2018

SebastianBoe Jun 25, 2018 •

edited

Loading

tejlmand Jun 27, 2018

SebastianBoe Jun 27, 2018

tejlmand Jun 27, 2018 •

edited

Loading

SebastianBoe Jun 27, 2018 •

edited

Loading

tejlmand Jun 27, 2018

SebastianBoe Jun 25, 2018

SebastianBoe Jun 25, 2018

tejlmand Jun 27, 2018

ioannisg commented Jun 22, 2019

aescolar commented Sep 18, 2019

		@@ -40,7 +40,11 @@ set_property( GLOBAL PROPERTY PROPERTY_OUTPUT_FORMAT elf32-little${ARCH})
		# compiler options needed by all source files. All zephyr libraries,

		add_library(zephyr_interface INTERFACE)
		target_link_libraries(zephyr_interface INTERFACE ${TOOLCHAIN_LIBS})

cmake: Kernel and offset libraries linking flags #8445

cmake: Kernel and offset libraries linking flags #8445

Conversation

tejlmand commented Jun 18, 2018

This comment was marked as outdated.

SebastianBoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

tejlmand Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

tejlmand commented Jun 20, 2018

SebastianBoe commented Jun 20, 2018

Choose a reason for hiding this comment

tejlmand Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

SebastianBoe Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannisg commented Jun 22, 2019

aescolar commented Sep 18, 2019

SebastianBoe Jun 18, 2018 •

edited

Loading

tejlmand Jun 18, 2018 •

edited

Loading

tejlmand Jun 22, 2018 •

edited

Loading

SebastianBoe Jun 25, 2018 •

edited

Loading

tejlmand Jun 27, 2018 •

edited

Loading

SebastianBoe Jun 27, 2018 •

edited

Loading