Skip to content

Conversation

@dcpleung
Copy link
Member

This renames the toolchain profile xcc-clang to xt-clang to better reflect the actual compiler being used.

This also adds a linker profile xt-ld so we can tailor support for Xtensa's linker. For now, it is simply to not pass -no-pie.

andyross
andyross previously approved these changes Feb 14, 2023
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

No complaints with this patch, but I still think we need to rethink the way we handle "gcc-like" toolchains to reduce all the cut/paste in the cmake tree.

ceolin
ceolin previously approved these changes Feb 15, 2023
tmleman
tmleman previously approved these changes Feb 15, 2023
@dcpleung
Copy link
Member Author

No complaints with this patch, but I still think we need to rethink the way we handle "gcc-like" toolchains to reduce all the cut/paste in the cmake tree.

Xtensa tools tend to be way behind (like xt-ld being based on ld 2.34). The LD profile on main tracks latest binutils rather closely (as with the Zephyr SDK). So this is simply to avoid the main LD profile having incompatible changes to xt-ld.

@marc-hb
Copy link
Contributor

marc-hb commented Feb 15, 2023

Xtensa tools tend to be way behind (like xt-ld being based on ld 2.34). The LD profile on main tracks latest binutils rather closely (as with the Zephyr SDK). So this is simply to avoid the main LD profile having incompatible changes to xt-ld.

This is worth being stated at the top of the new file IMHO. Otherwise people will keep wondering about the duplication.

PS: thanks for the rename that was requested in #54445 and thesofproject/sof#7027, much appreciated.

Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

No problem with the change but this seems to just instantly break anyone using xcc-clang with no deprecation notice to them or indication of how they cane update their workflow to remain compatible in the future

Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

please deprecate so that users are made aware about this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a deprecation warning here:

--- a/cmake/modules/FindDeprecated.cmake
+++ b/cmake/modules/FindDeprecated.cmake
@@ -50,6 +50,11 @@ if("XCC_USE_CLANG" IN_LIST Deprecated_FIND_COMPONENTS)
     set(ZEPHYR_TOOLCHAIN_VARIANT xt-clang CACHE STRING "Zephyr toolchain variant" FORCE)
     message(DEPRECATION "XCC_USE_CLANG is deprecated. Please set ZEPHYR_TOOLCHAIN_VARIANT to 'xt-clang'")
   endif()
+
+  if("${ZEPHYR_TOOLCHAIN_VARIANT}" STREQUAL "xcc-clang")
+    set(ZEPHYR_TOOLCHAIN_VARIANT xt-clang CACHE STRING "Zephyr toolchain variant" FORCE)
+    message(DEPRECATION "ZEPHYR_TOOLCHAIN_VARIANT 'xcc-clang' is deprecated. Please set ZEPHYR_TOOLCHAIN_VARIANT to 'xt-clang'")
+  endif()
 endif()
 
 if("CROSS_COMPILE" IN_LIST Deprecated_FIND_COMPONENTS)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

@marc-hb
Copy link
Contributor

marc-hb commented Feb 17, 2023

this seems to just instantly break anyone using xcc-clang with no deprecation notice to them or indication of how they cane update their workflow to remain compatible in the future

This is exactly why it's being done now while very few people and machines are just getting started with xt-clang; before the backwards incompatibility situation you describe happens.

@nashif
Copy link
Member

nashif commented Feb 18, 2023

This is exactly why it's being done now while very few people and machines are just getting started with xt-clang; before the backwards incompatibility situation you describe happens.

we have been using xt-clang for quite a while, deprecation is still needed./

@nordicjm
Copy link
Contributor

This is exactly why it's being done now while very few people and machines are just getting started with xt-clang; before the backwards incompatibility situation you describe happens.

It doesn't matter if it's 0, 1 or half of europe using it, you can't just silently break a whole setup by changing the name without having some level of warning to users for a period of time until it is removed.

@dcpleung dcpleung dismissed stale reviews from tmleman, ceolin, and andyross via f4e445e February 21, 2023 18:49
This reflects the actual compiler executable name of the Xtensa
LLVM/Clang compiler.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
Xtensa toolchain has its own linker, xt-ld, which is based on
binutils' ld. There are, of course, Xtensa specific options.
But mostly it is based on old version of ld. It would be
better to detach it from the ld profile to avoid any
incompatible changes there.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
@marc-hb
Copy link
Contributor

marc-hb commented Feb 21, 2023

It doesn't matter if it's 0, 1 or half of europe using it, you can't just silently break a whole setup by changing the name without having some level of warning to users for a period of time until it is removed.

Some level of warning is required and it does matter for how many people it breaks.

Copy link
Contributor

@galak galak left a comment

Choose a reason for hiding this comment

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

so probably better to just copy the files over to xt-clang and leave the xcc-clang files

@dcpleung
Copy link
Member Author

The snippet posted by @tejlmand above should provide the necessary deprecation warning.

@nashif nashif merged commit 4683c8d into zephyrproject-rtos:main Feb 22, 2023
@dcpleung dcpleung deleted the toolchain/xt-clang branch February 22, 2023 15:11
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.

10 participants