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.

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

cmake: xcc: fix compilation issues #79731

Merged
merged 3 commits into from
Oct 15, 2024
Merged

Conversation

dcpleung
Copy link
Member

@dcpleung dcpleung commented Oct 11, 2024

  • cmake: xcc: use xt-ld as linker
  • cmake: xcc: do not use -fno-printf-return-value
  • cmake: xcc: remove TOOLCHAIN_LIBS

Fixes some issues when compiling with good old XCC.

nashif
nashif previously approved these changes Oct 11, 2024
@dcpleung dcpleung requested a review from marc-hb October 11, 2024 17:26
@marc-hb
Copy link
Collaborator

marc-hb commented Oct 11, 2024

This is great if it is enough to fix XCC compilation that was just broken but note this does not seem to answer the confusing ${TOOLCHAIN_LIBS} situation and inconsistency that caused the regression in the first place:

cc: @tejlmand

@dcpleung
Copy link
Member Author

Twister failures are on samples/subsys/dap/sample.dap.bulk with Freedom and NRF boards, which is unrelated to this change.

@dcpleung

This comment was marked as outdated.

XCC does not recognize -fno-printf-return-value as compiler
flag. So skip it.

Signed-off-by: Daniel Leung <[email protected]>
@dcpleung
Copy link
Member Author

Turns out it is not too difficult to remove TOOLCHAIN_LIBS.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Turns out it is not too difficult to remove TOOLCHAIN_LIBS.

@tejlmand can you please explain why this removal was incomplete in #78320? Just fear of "ripping the bandaid", oversight, accident, some more fundamental reason, other?

@@ -6,6 +6,7 @@ set(COMPILER xcc)
set(OPTIMIZE_FOR_DEBUG_FLAG "-O0")
set(CC xcc)
set(C++ xc++)
set(LINKER xt-ld)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need to use an obsolete linker in addition to using an obsolete compiler? The existing solution worked fine until the big CMake shuffle. Moreover, you seemed to have cracked the TOOLCHAIN_LIBS nut in v2 so is this switch really still necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is better to stay within the confine of the Xtensa toolchain instead of mixing toolchain components that may not be compatible with each other.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is debatable. Compilation and linkage are two steps fairly independent from each other with good compatibility and zephyr/cmake seems to assume they are indeed. xt-xcc has been a pre-C11 pain but it features proprietary customizations found no one else, so it's a necessary pain. What value does xt-ld bring to the table besides old linker bugs that no-one wants to debug and a lack of recent features? Isn't the same than ld, just older? Also: it has apparently NOT been used in production. Probably by accident but still.

But back to the initial issue: can't we have ONLY a fix for the CMake regression FIRST? And then only later a linker change once SOF CI is back on its feet?

@marc-hb marc-hb requested review from andyross, lyakh and kv2019i October 14, 2024 21:07
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Also: xt-ld has apparently NOT been used in production. Probably by accident but still.

Correction.

So I just tested multiple Zephyr versions with -DEXTRA_LDFLAGS=-lfail_and_tell to see what linker is actually in use. As usual: the best way to test something is to break it. For a build system it's probably the ONLY testing method available... I digress

It turns out that linkage is and was performed by NOT invoking the linker directly but by invoking the the compiler front-end XtensaTools/bin/xt-xcc. As it should be. Also, the error message is always XtensaTools/bin/xt-ld: cannot find -lfail_and_tell.

So, xt-ld has always been used?

The error message comes from XtensaTools/bin/xt-ld before and after the big #78320 shuffle (which I clearly don't have a clue about, just in case that wasn't obvious yet). This is also true before and after this PR. Funny enough this is true before AND after commit cmake: xcc: use xt-ld as linker. So at the very least this commit title must be fixed because this does not seem to actually switch the linker.

-lhal is missing without that commit so that commit does "something" but it apparently does not switch the linker.

Note: I tested SOF compilation with the SOF configuration script sof/scripts/xtensa-build-zephyr.py tgl. I really, really hope that does not make any linker difference; I'd be amazed!

Shuffling of ld/lld on C library linking cmake code causes
issue with XCC as the HAL library is not being included in
linking. So make XCC to use xt-ld linker cmake code such
that the HAL library is included.

Signed-off-by: Daniel Leung <[email protected]>
Following the footstep of GCC/Clang cmake code to remove
TOOLCHAIN_LIBS, xcc also has it removed and utilizes
something similar to c_library to link the HAL library.

Signed-off-by: Daniel Leung <[email protected]>
@dcpleung
Copy link
Member Author

The commit message actually refers to the "xt-ld linker cmake code" so the message itself is fine. I just changed the title to reflect that.

@marc-hb
Copy link
Collaborator

marc-hb commented Oct 14, 2024

I just changed the title to reflect that.

Thanks, the old one really threw me off.

@tejlmand
Copy link
Collaborator

tejlmand commented Oct 15, 2024

@tejlmand can you please explain why this removal was incomplete in #78320? Just fear of "ripping the bandaid", oversight, accident, some more fundamental reason, other?

placed a comment here: #79731 (review)

EDIT: here #78320 (comment)

@nashif nashif merged commit ac98d0e into zephyrproject-rtos:main Oct 15, 2024
23 checks passed
@dcpleung dcpleung deleted the xcc/fixes branch October 15, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arch: xtensa: Undefined reference to _xtos_pso_savearea and _xtos_core_restore_nw
7 participants