Skip to content

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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 8 additions & 7 deletions cmake/linker/ld/target.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@ set(CMAKE_LINKER ${GNULD_LINKER})

set_ifndef(LINKERFLAGPREFIX -Wl)

if((${CMAKE_LINKER} STREQUAL "${CROSS_COMPILE}ld.bfd") OR
${GNULD_LINKER_IS_BFD})
# ld.bfd was found so let's explicitly use that for linking, see #32237
list(APPEND TOOLCHAIN_LD_FLAGS -fuse-ld=bfd)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think moving this here is valid but I'm not entirely sure. IIUC, previously -fuse-ld=bfd would only be added when toolchain_ld_link_elf is called for a given target (and it's dependencies). By adding this to TOOLCHAIN_LD_FLAGS instead (which will then be added via zephyr_ld_options when toolchain_ld_base is invoked), I think we'll effectively add -fuse-ld=bfd everywhere we link. As toolchain_ld_link_elf is (IIUC) invoked for each of the link stages (pre0/1, final) it seems like this change should be equivalent--are there use cases where we're both linking other things and truly don't want to use the same linker variant as the "main" .elfs?

I don't see anything concerning in 39f06e0 where this was first added, but I'm not sure if I'm missing/misunderstanding something.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a fine location.

list(APPEND CMAKE_REQUIRED_FLAGS -fuse-ld=bfd)
string(REPLACE ";" " " CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS}")
endif()

if(NOT "${ZEPHYR_TOOLCHAIN_VARIANT}" STREQUAL "host")
if(CONFIG_CPP_EXCEPTIONS AND LIBGCC_DIR)
# When building with C++ Exceptions, it is important that crtbegin and crtend
Expand Down Expand Up @@ -118,16 +126,9 @@ function(toolchain_ld_link_elf)
${ARGN} # input args to parse
)

if((${CMAKE_LINKER} STREQUAL "${CROSS_COMPILE}ld.bfd") OR
${GNULD_LINKER_IS_BFD})
# ld.bfd was found so let's explicitly use that for linking, see #32237
set(use_linker "-fuse-ld=bfd")
endif()

target_link_libraries(
${TOOLCHAIN_LD_LINK_ELF_TARGET_ELF}
${TOOLCHAIN_LD_LINK_ELF_LIBRARIES_PRE_SCRIPT}
${use_linker}
${TOPT}
${TOOLCHAIN_LD_LINK_ELF_LINKER_SCRIPT}
${TOOLCHAIN_LD_LINK_ELF_LIBRARIES_POST_SCRIPT}
Expand Down
4 changes: 4 additions & 0 deletions cmake/linker/lld/target.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ set(CMAKE_LINKER ${LLVMLLD_LINKER})

set_ifndef(LINKERFLAGPREFIX -Wl)

list(APPEND TOOLCHAIN_LD_FLAGS -fuse-ld=lld)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two thoughts here:

  1. Conceptually, setting -fuse-ld here rather than target_baremetal.cmake makes sense to me as I don't think -fuse-ld should be considered a baremetal-specific flag. I don't think this should cause issues for other toolchains (if they relied on it not being specified for non-baremetal cases for some reason, for example) but I'm not sure if there is something I'm missing. oneAPI, for example, seems to use lld but -fuse-ld=lld should work as expected as best I can tell from the docs.
  2. I dropped the if(NOT CONFIG_LLVM_USE_LD) check from the old location. This is an unrelated change I guess, but since I was moving this code it didn't make sense to me to keep it--I don't think we should be in lld/target.cmake unless a) CONFIG_LLVM_USE_LLD was selected or b) we're using a non-LLVM toolchain at which point I don't think CONFIG_LLVM_USE_LD should be set, so the check is always true. I can readd the if if desired/if I'm misunderstanding though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Conceptually, setting -fuse-ld here rather than target_baremetal.cmake makes sense to me as I don't think -fuse-ld should be considered a baremetal-specific flag.

agree. unfortunately the existing toolchain_ld_base and toolchain_ld_baremetal macros has become a bit messy over time and thus diverted from their initial purpose.

A first approach in cleaning up this without introducing functional changes is done here #77887.
Although that cleanup doesn't target the mis-placement of -fuse-ld=lld, then it might have your interest.

I dropped the if(NOT CONFIG_LLVM_USE_LD) check from the old location. ...
I don't think we should be in lld/target.cmake unless a) CONFIG_LLVM_USE_LLD was selected

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree. unfortunately the existing toolchain_ld_base and toolchain_ld_baremetal macros has become a bit messy over time and thus diverted from their initial purpose.

A first approach in cleaning up this without introducing functional changes is done here #77887. Although that cleanup doesn't target the mis-placement of -fuse-ld=lld, then it might have your interest.

Thank you for the pointer (and for the patch itself too)! That makes sense to me--I'll fix this up accordingly soon now that that one has landed!

list(APPEND CMAKE_REQUIRED_FLAGS -fuse-ld=lld)
string(REPLACE ";" " " CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS}")

# Run $LINKER_SCRIPT file through the C preprocessor, producing ${linker_script_gen}
# NOTE: ${linker_script_gen} will be produced at build-time; not at configure-time
macro(configure_linker_script linker_script_gen linker_pass_define)
Expand Down
7 changes: 0 additions & 7 deletions cmake/linker/lld/target_baremetal.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,6 @@ macro(toolchain_ld_baremetal)
${LINKERFLAGPREFIX},-N
)

# Force LLVM to use built-in lld linker
if(NOT CONFIG_LLVM_USE_LD)
zephyr_ld_options(
-fuse-ld=lld
)
endif()

# Funny thing is if this is set to =error, some architectures will
# skip this flag even though the compiler flag check passes
# (e.g. ARC and Xtensa). So warning should be the default for now.
Expand Down
Loading