cmake: toolchain: cache property GNULD_LINKER_IS_BFD #56547

dcpleung · 2023-04-04T21:37:32Z

Cache the property GNULD_LINKER_IS_BFD between cmake invocations. It is observed that, in repeated builds (2nd time and later), this property becomes true even for non-bfd compatible linker. So cache it to avoid any surprises.

Fixes #56501

marc-hb

I confirm a (small variation of) this fixes #56501. Thanks for the very prompt fix!

I would still like some clarification about https://docs.zephyrproject.org/latest/develop/west/build-flash-debug.html#one-time-cmake-arguments : is it "safe" or not? I mean as long as the same CMake arguments are used every time. Just slower sometimes?

Nit: instead of duplicating them exactly, I would make the doc strings slightly different and unique so they can be used as "breadcrumbs".

tejlmand

minor adjustments requested, but generally looks good.

tejlmand · 2023-04-05T09:45:48Z

cmake/modules/FindGnuLd.cmake

The lookup at those locations has already set the cache variable:
https://github.com/zephyrproject-rtos/zephyr/blob/bbc4f11fb3a35d84568c53340a6a934777f71461/cmake/modules/FindGnuLd.cmake#L46
https://github.com/zephyrproject-rtos/zephyr/blob/bbc4f11fb3a35d84568c53340a6a934777f71461/cmake/modules/FindGnuLd.cmake#L49

so only chance that GNULD_LINKER_IS_BFD has already been set, but the corresponding GNULD_LINKER_IS_BFD is undefined is when a user has specifically pointed to a ld through the use of: -DGNULD_LINKER_IS_BFD=<forced-linker-to-use>.

In such a case we cannot really know if linker is bfd compatible or not, as we have no proper detection mechanism.

So in the scenario described we must rely on user to provide us the correct setting, so I propose issue a warning in such case.

Something like:

if(EXISTS "${GNULD_LINKER}") if(NOT DEFINED GNULD_LINKER_IS_BFD) message(WARNING "GNULD_LINKER specified directly in cache, but GNULD_LINKER_IS_BFD is not defined." "Assuming GNULD_LINKER_IS_BFD as OFF, please set GNULD_LINKER_IS_BFD to correct value in cache to silence this warning" ) set(GNULD_LINKER_IS_BFD OFF) endif() endif()

Added the warning and also to return early if variable is already set.

tejlmand · 2023-04-05T09:49:44Z

cmake/modules/FindGnuLd.cmake

please add FORCE.

If this code ever executes a second time, then it means that someone purposely has cleared the GNULD_LINKER setting, and therefor we should ensure to update the corresponding GNULD_LINKER_IS_BFD to the new lookup we perform.

Suggested change

set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Is linker ld.bfd compatible")

set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Is linker ld.bfd compatible" FORCE)

I added the code to return early if GNULD_LINKER_IS_BFD is already set. So no need to FORCE a change.

@dcpleung your "return early" is currently when GNULD_LINKER exists. @tejlmand describes a case where someone deleted it.

Added back.

My rule of thumb for FORCE is to add it first and then think later
https://stackoverflow.com/questions/39588836/why-do-i-need-force-to-override-a-cmake-option

tejlmand · 2023-04-05T09:50:29Z

cmake/modules/FindGnuLd.cmake

Suggested change

set(GNULD_LINKER_IS_BFD OFF CACHE BOOL "Is linker ld.bfd compatible")

set(GNULD_LINKER_IS_BFD OFF CACHE BOOL "Is linker ld.bfd compatible" FORCE)

I added the code to return early if GNULD_LINKER_IS_BFD is already set. So no need to FORCE a change.

marc-hb · 2023-04-05T17:42:49Z

Thanks again @tejlmand for your time and critical/"bus factor" expertise. Since you have a very good understanding of these CMake issues and while you're here, could you please answer this earlier, more generic and related question of mine:

I would still like some clarification about https://docs.zephyrproject.org/latest/develop/west/build-flash-debug.html#one-time-cmake-arguments : is it "safe" or not? I mean as long as the same CMake arguments are used every time. Just slower sometimes?

@mbolivar-nordic I think you mentioned once some past discussions on this topic, were they on Github, Slack or Discord?

As usual I'm volunteering for a minor doc fix if needed but I need to know what it should be :-)

I'm among the few people who know the secrets to make the doc build usable (#55708)

marc-hb

Tested again and fixes the issue again.

Nit: instead of duplicating them exactly, I would make the doc strings slightly different and unique so they can be used as "breadcrumbs".

Ping? Even more relevant now that the logic is becoming more complex.

cmake/modules/FindGnuLd.cmake

dcpleung · 2023-04-05T20:28:14Z

Nit: instead of duplicating them exactly, I would make the doc strings slightly different and unique to they can be used as "breadcrumbs".

Ping? Even more relevant now that the logic is becoming more complex.

Forgot to change those while juggling other changes.

marc-hb · 2023-04-07T20:00:23Z

I would still like some clarification about https://docs.zephyrproject.org/latest/develop/west/build-flash-debug.html#one-time-cmake-arguments

With @mbolivar-nordic's help, partly answered myself in the bug for this:

Since GNULD_LINKER_IS_BFD, incremental SOF build fails with a very confusing "-fuse-ld=bfd: unknown flag" #56501 (comment)

Small doc improvement submitted in

docs/build-flash-debug: expand one-time CMake arguments section #56671

tejlmand

I believe the return should also cover the case where the GNULD_LINKER_IS_BFD as there seems to be no reason to start detecting / manipulating GNULD_LINKER` setting when already set.

And while at it, please align the CMakeCache description text.

tejlmand · 2023-04-11T12:51:24Z

cmake/modules/FindGnuLd.cmake

I assume you would want to move the return down, so that if ever GNULD_LINKER is defined then the look-up of ld linker is aborted.

Other comment will be made to follow up this comment.

tejlmand · 2023-04-11T12:53:33Z

cmake/modules/FindGnuLd.cmake

I believe the early return should happen here.

If for any reason GNULD_LINKER has been pointed out manually by a user, then we should not try to detect GNULD_LINKER_IS_BFD based on current compiler, hence I believe we should move the return here:

Suggested change

endif()

endif()

return()

tejlmand · 2023-04-11T12:58:15Z

cmake/modules/FindGnuLd.cmake

maybe try to make the CMake cache comment more aligned. (see also following comments)

Proposal:

Suggested change

set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Compiler says linker is ld.bfd compatible" FORCE)

set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Linker BFD compatibility (compiler reported)" FORCE)

tejlmand · 2023-04-11T12:59:30Z

cmake/modules/FindGnuLd.cmake

Suggested change

set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Found ld.bfd compatible linker" FORCE)

set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Linker BFD compatibility (inferred from binary)" FORCE)

tejlmand · 2023-04-11T13:00:27Z

cmake/modules/FindGnuLd.cmake

Suggested change

set(GNULD_LINKER_IS_BFD OFF CACHE BOOL "ld.bfd compatible linker not found" FORCE)

set(GNULD_LINKER_IS_BFD OFF CACHE BOOL "Linker BFD compatibility (inferred from binary)" FORCE)

tejlmand · 2023-04-11T13:23:05Z

I would still like some clarification about https://docs.zephyrproject.org/latest/develop/west/build-flash-debug.html#one-time-cmake-arguments : is it "safe" or not? I mean as long as the same CMake arguments are used every time. Just slower sometimes?

been out for Easter 🐰 , but happy to give an answer.
From CMake perspective, then it should always be valid to set a public CMake cache variable during a new CMake run.
Internal CMake cache variables should not be set by users because as the name implies, they are internal.

When it comes to west, then west will detect that -- is used and forcefully invoke a CMake re-run even if such re-run is not required.
This is perfectly valid but it will of course waste resources if a CMake re-run was not required.
For example a CMake re-run is not needed when the "new" CMake cache value is identical to the existing value.

Some settings can be passed multiple times only if the value is identical to the existing value in the cache, for example BOARD.
So doing this is allowed:

$ cmake -DBOARD=nrf52840dk_nrf52840 -Bbuild samples/hello_world
$ cmake -DBOARD=nrf52840dk_nrf52840 build
$ cmake -DBOARD=nrf52840dk_nrf52840 build

when the passed board is identical to existing value.

But changing the board is prohibited, so if you try to do:

$ cmake -DBOARD=nrf52840dk_nrf52811 build
...
CMake Warning at /projects/github/ncs/zephyr/cmake/modules/extensions.cmake:2617 (message):
  The build directory must be cleaned pristinely when changing board,

  Current value="nrf52840dk_nrf52840", Ignored value="nrf52840dk_nrf52811"

the old value will be used.

That is not a CMake limitation, but a Zephyr limitation because some settings are so fundamental that we cannot allow them to be changed without a pristine build.

Cache the property GNULD_LINKER_IS_BFD between cmake invocations. It is observed that, in repeated builds (2nd time and later), this property becomes true even for non-bfd compatible linker. So cache it to avoid any surprises. Fixes zephyrproject-rtos#56501 Signed-off-by: Daniel Leung <[email protected]>

dcpleung · 2023-04-11T21:07:34Z

Updated according to comments.

tejlmand · 2023-04-12T12:14:10Z

@dcpleung thanks for the patience 👍

nashif · 2023-04-12T14:25:28Z

Please do not mark PRs with issue labels, that does not help anyone.

dcpleung requested review from nashif and tejlmand as code owners April 4, 2023 21:37

dcpleung requested a review from marc-hb April 4, 2023 21:37

zephyrbot added the area: Build System label Apr 4, 2023

zephyrbot requested review from jeremybettis and nordicjm April 4, 2023 21:38

zephyrbot assigned tejlmand Apr 4, 2023

marc-hb previously approved these changes Apr 4, 2023

View reviewed changes

nashif previously approved these changes Apr 4, 2023

View reviewed changes

This was referenced Apr 5, 2023

west.yml: upgrade zephyr to 3024bc5b19 thesofproject/sof#7366

Closed

[IGNORE for now] xtensa-build-zephyr.py: forbid overlays in incremental builds thesofproject/sof#7365

Closed

tejlmand requested changes Apr 5, 2023

View reviewed changes

tmleman previously approved these changes Apr 5, 2023

View reviewed changes

dcpleung dismissed stale reviews from tmleman, nashif, and marc-hb via 8f5d46b April 5, 2023 17:46

dcpleung force-pushed the toolchain/cache_bfd_flag branch from bbc4f11 to 8f5d46b Compare April 5, 2023 17:46

nashif requested a review from tejlmand April 5, 2023 20:19

nashif previously approved these changes Apr 5, 2023

View reviewed changes

marc-hb reviewed Apr 5, 2023

View reviewed changes

cmake/modules/FindGnuLd.cmake Outdated Show resolved Hide resolved

dcpleung dismissed nashif’s stale review via d2d9a2f April 5, 2023 20:27

dcpleung force-pushed the toolchain/cache_bfd_flag branch from 8f5d46b to d2d9a2f Compare April 5, 2023 20:27

nashif previously approved these changes Apr 5, 2023

View reviewed changes

dcpleung dismissed nashif’s stale review via ca84c55 April 6, 2023 16:04

dcpleung force-pushed the toolchain/cache_bfd_flag branch 2 times, most recently from ca84c55 to e444f68 Compare April 6, 2023 16:04

marc-hb previously approved these changes Apr 6, 2023

View reviewed changes

nashif previously approved these changes Apr 6, 2023

View reviewed changes

tejlmand requested changes Apr 11, 2023

View reviewed changes

marc-hb added the platform: Intel ADSP Intel Audio platforms label Apr 11, 2023

dcpleung dismissed stale reviews from nashif and marc-hb via 0521d54 April 11, 2023 21:06

dcpleung force-pushed the toolchain/cache_bfd_flag branch from e444f68 to 0521d54 Compare April 11, 2023 21:06

nashif approved these changes Apr 11, 2023

View reviewed changes

marc-hb approved these changes Apr 11, 2023

View reviewed changes

tejlmand approved these changes Apr 12, 2023

View reviewed changes

marc-hb added bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug labels Apr 12, 2023

nashif merged commit 2a76637 into zephyrproject-rtos:main Apr 12, 2023

nashif removed bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug labels Apr 12, 2023

dcpleung deleted the toolchain/cache_bfd_flag branch April 12, 2023 14:26

	set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Is linker ld.bfd compatible")
	set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Is linker ld.bfd compatible" FORCE)

	set(GNULD_LINKER_IS_BFD OFF CACHE BOOL "Is linker ld.bfd compatible")
	set(GNULD_LINKER_IS_BFD OFF CACHE BOOL "Is linker ld.bfd compatible" FORCE)

	set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Compiler says linker is ld.bfd compatible" FORCE)
	set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Linker BFD compatibility (compiler reported)" FORCE)

	set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Found ld.bfd compatible linker" FORCE)
	set(GNULD_LINKER_IS_BFD ON CACHE BOOL "Linker BFD compatibility (inferred from binary)" FORCE)

cmake: toolchain: cache property GNULD_LINKER_IS_BFD #56547

cmake: toolchain: cache property GNULD_LINKER_IS_BFD #56547

Uh oh!

Conversation

dcpleung commented Apr 4, 2023

Uh oh!

marc-hb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejlmand 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!

marc-hb Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

marc-hb commented Apr 5, 2023

Uh oh!

marc-hb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dcpleung commented Apr 5, 2023

Uh oh!

marc-hb commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tejlmand 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!

tejlmand commented Apr 11, 2023

Uh oh!

dcpleung commented Apr 11, 2023

Uh oh!

tejlmand commented Apr 12, 2023

Uh oh!

nashif commented Apr 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marc-hb left a comment •

edited

Loading

marc-hb Apr 6, 2023 •

edited

Loading

marc-hb left a comment •

edited

Loading

marc-hb commented Apr 7, 2023 •

edited

Loading