toolchain: cleanup toolchain Kconfigs

[galak]

No description provided.

[keith-packard]

Any idea if this supports TLS? It's not marked as such here, wondering what the upstream status is.

[galak]

Not sure, its something we need to dig into and test out.

[keith-packard]

Should this be here? Or should this just continue to use the bits provided in the SDK itself?

[galak]

The intent is to be able to put things here that either don't depend on the SDK version, or we don't want to have to update and re-spin the SDK for.

So for example when you introduce TOOLCHAIN_SUPPORTS_NEWLIB, that can just go in here w/o having to update the SDK Kconfig.

[keith-packard]

all good, as long as the sdk team is aligned.

[tejlmand]

This seems to have a built-in risk of getting back to the mess we used to have, but was cleaned up here: e747fe7

The intent is to be able to put things here that either don't depend on the SDK version

How do you know ?
If there is a setting here describing a feature of the SDK, why can't that feature be removed in a future version ?
Or in case the Zephyr-SDK is split into multiple packages, that the feature is only available if a specific sub-package is installed.

or we don't want to have to update and re-spin the SDK for.

has this been a problem so far ?

Now you introduce Kconfig files in two places that describes the toolchain.
In Zephyr tree, and and in the SDK.
At some point someone will end-up placing a setting in the wrong file (most likely in the Zephyr repo, when it should have been in the Sdk repo), and that PR just gets approved because it looks ok, and things are working with the current SDK.

Not to mention, there are now two env settings for kconfig that controls Kconfig sourcing:

• ZEPHYR_TOOLCHAIN_KCONFIG_DIR=${ZEPHYR_TOOLCHAIN_KCONFIG_DIR}
• TOOLCHAIN_KCONFIG_DIR=${TOOLCHAIN_KCONFIG_DIR}

bound to result in confusion.

I'm not convinced, and I fear there's a high risk of ending up in a situation we had with Zephyr SDK and versions years back.

[galak]

I think we should have something like this since we shouldn't require a user to have to download a new toolchain just because of a Kconfig update. For example when we introduce TOOLCHAIN_HAS_PICOLIBC.

I get that there might be some confusion on where to put what Kconfig's but, I'd argue that we don't do that very often, we have people to review those changes to tell a developer if they are placing it in the wrong place.

[tejlmand]

For example when we introduce TOOLCHAIN_HAS_PICOLIBC.

when we introduce such a setting then that should go into the toolchain itself.
Cause there could be support for multiple Zephyr SDK version, with or without Picolibc, at the same time.

Users can already now be using 0.15, but they may also use 0.16-rc2 for tests, and when 0.16 is released, then users of older Zephyr release (v3.3, v3.2, etc) can decide to use newer Zephyr SDK, but in such cases the old Zephyr tag/release cannot have a TOOLCHAIN_HAS_PICOLIBC in it's Kconfig, cause the Zephyr SDK 0.15 might still be used by some.

I get that there might be some confusion on where to put what Kconfig's but, I'd argue that we don't do that very often

So let's not add it and avoid any potential confusion.
If there is a real use-case / need for adding this later, then let's do it at that time.

[galak]

How can we introduce a setting like TOOLCHAIN_HAS_PICOLIBC into an already released toolchain? My point is we need Kconfig in both Zephyr and the SDK to handle cases in which the Zephyr code base wants to describe a new Kconfig symbol for something that exists in an already released toolchain, and spinning a new toolchain version just to update the Kconfig doesn't make sense to me.

[tejlmand]

How can we introduce a setting like TOOLCHAIN_HAS_PICOLIBC into an already released toolchain?

will such a feature be introduced into Zephyr SDK without it being actively used ?

My point is we need Kconfig in both Zephyr and the SDK to handle cases in which the Zephyr code base wants to describe a new Kconfig symbol for something that exists in an already released toolchain

and that's why there should be a common toolchain file which simply defines the settings and type of toolchain features, and then dedicated toolchains can describe the default without specifying the type, see other comment.

We don't need an extra setting and Kconfig file for the Zephyr SDK for this.
If for some reason there really is a high demand for a temp solution until a new SDK spin is ready, or backport reasons where older SDK may also support a feature we start using, then we may have a default y if TOOLCHAIN_ZEPHYR_0_16 or similar, and then remove this special condition handling at earliest possibility.

I still don't see a justification for having an extra Zephyr SDK Kconfig tree and setting which must be passed to konfiglib invocation, considering the risk of misuse / mistakes that may occur.

[keith-packard]

Is this trying to encompass all possible toolchains? Or is there still a mechanism for people to provide their own with suitable configuration bits?

A couple of these toolchains appear to just be relabled versions of the Zephyr SDK? Is there some reason (aside from branding) to have additional names?

[galak]

Is this trying to encompass all possible toolchains? Or is there still a mechanism for people to provide their own with suitable configuration bits?

It was for the one's we supported a possible values of ZEPHYR_TOOLCHAIN_VARIANT. Nothing in here precludes some other out of tree toolchain being supported.

A couple of these toolchains appear to just be relabled versions of the Zephyr SDK? Is there some reason (aside from branding) to have additional names?

Some of it may have been historic, some of it has to due with subtle uniqueness between various toolchain builds.

[carlescufi]

@stephanosio @tejlmand can you please review?

[stephanosio]

Looks reasonable to me, but it should wait for @tejlmand 's review in case he has a better idea.

[tejlmand]

thanks for this cleanup.

Some minor comments, and a single major.
I'm not convinced we should introduce one more Toolchain specific Kconfig, imo it has a too high risk of being misused and/or users not understanding what settings should be placed where.

[tejlmand]

This seems to have a built-in risk of getting back to the mess we used to have, but was cleaned up here: e747fe7

The intent is to be able to put things here that either don't depend on the SDK version

How do you know ?
If there is a setting here describing a feature of the SDK, why can't that feature be removed in a future version ?
Or in case the Zephyr-SDK is split into multiple packages, that the feature is only available if a specific sub-package is installed.

or we don't want to have to update and re-spin the SDK for.

has this been a problem so far ?

Now you introduce Kconfig files in two places that describes the toolchain.
In Zephyr tree, and and in the SDK.
At some point someone will end-up placing a setting in the wrong file (most likely in the Zephyr repo, when it should have been in the Sdk repo), and that PR just gets approved because it looks ok, and things are working with the current SDK.

Not to mention, there are now two env settings for kconfig that controls Kconfig sourcing:

• ZEPHYR_TOOLCHAIN_KCONFIG_DIR=${ZEPHYR_TOOLCHAIN_KCONFIG_DIR}
• TOOLCHAIN_KCONFIG_DIR=${TOOLCHAIN_KCONFIG_DIR}

bound to result in confusion.

I'm not convinced, and I fear there's a high risk of ending up in a situation we had with Zephyr SDK and versions years back.

[tejlmand]

a bit unfortunate that #45656 introduced ZEPHYR_TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE as environment variable, but now that's it's there we should do a proper deprecation and tell users how to define that the toolchain is supporting this.

I assume @keith-packard was using the cross-compile toolchain setting when testing with Debian arm-none-eabi, correct @keith-packard ?

The "$(ZEPHYR_TOOLCHAIN_VARIANT)" = "zephyr" is something that should have been placed in the Zephyr SDK Kconfig file, not sure why I didn't noticed that back then: #29591
We should move this to Zephyr-SDK for the upcomming 0.16 release, and then make a comment here, that this code is kept for backwards compatibility until minimum required Zephyr SDK is 0.16.
fyi: @stephanosio

[stephanosio]

It already is part of Zephyr SDK.

https://github.com/zephyrproject-rtos/sdk-ng/blob/0bc76f4b82e1f2d5d9d22f8a2c4fe9b1551cd08d/cmake/zephyr/Kconfig#L7

I believe this was a leftover from before the config was added on the SDK side.

[tejlmand]

So only question remains is the ZEPHYR_TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE in environment.

[galak]

I put back ZEPHYR_TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE support. We can clean that up in a separate PR based on what @keith-packard says. (if we keep it we should document it since its a completely hidden setting).

[keith-packard]

a bit unfortunate that #45656 introduced ZEPHYR_TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE as environment variable, but now that's it's there we should do a proper deprecation and tell users how to define that the toolchain is supporting this.

Agreed -- I think having any environment variables adjusting configuration at this level is fraught with potential conflict.

I assume @keith-packard was using the cross-compile toolchain setting when testing with Debian arm-none-eabi, correct @keith-packard ?

Yes, I regularly test Zephyr builds with the Debian toolchain as I am the most active maintainer for that, and Zephyr offers another great source of test cases, in addition to the glibc and picolibc test cases already in use.

[galak]

Agreed -- I think having any environment variables adjusting configuration at this level is fraught with potential conflict.

@keith-packard just to be clear, you are good w/removing the env var option?

[keith-packard]

As long as there's some way to configure this option, yes, removing the environment variable would be fine. I'm using Zephyr as part of the Debian toolchain validation process; changing the scripts there to adapt to whatever Zephyr needs is easy for me, it's not like that's part of a large number of product build systems.

[tejlmand]

As long as there's some way to configure this option, yes

@keith-packard if you're using ZEPHYR_TOOLCHAIN_VARIANT=cross-compile, then setting -DCONFIG_TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE=y on CMake invocation should do the same.
If you want to avoid typing this with CMake, then you could adjust the default Kconfig setting in a dedicated Kconfig file.

[tejlmand]

Still not convinced regarding the (re-)introduction of Zephyr SDK Kconfig file directly in the Zephyr repo.

Besides that, some additional comments / observation when having a new look at code.

[tejlmand]

For example when we introduce TOOLCHAIN_HAS_PICOLIBC.

when we introduce such a setting then that should go into the toolchain itself.
Cause there could be support for multiple Zephyr SDK version, with or without Picolibc, at the same time.

Users can already now be using 0.15, but they may also use 0.16-rc2 for tests, and when 0.16 is released, then users of older Zephyr release (v3.3, v3.2, etc) can decide to use newer Zephyr SDK, but in such cases the old Zephyr tag/release cannot have a TOOLCHAIN_HAS_PICOLIBC in it's Kconfig, cause the Zephyr SDK 0.15 might still be used by some.

I get that there might be some confusion on where to put what Kconfig's but, I'd argue that we don't do that very often

So let's not add it and avoid any potential confusion.
If there is a real use-case / need for adding this later, then let's do it at that time.

[tejlmand]

Why is this needed ?

We already have TOOLCHAIN_KCONFIG_DIR as setting for sourcing of toolchain Kconfig.

[galak]

As we need a means to distinguish the toolchain Kconfig in the zephyr code base vs the one in the SDK.

[tejlmand]

but you are introducing a purely Zephyr SDK specific setting, all other settings are common among all Kconfigs and toolchains and their values determine the behavior.

The ZEPHYR_TOOLCHAIN_KCONFIG_DIR is only for the Zephyr SDK, and therefore I don't think it belongs here.

As mentioned in other comment, then I don't believe the extra Kconfig is needed, but if it goes in, then there should still be only one Kconfig entry point for the toolchain Kconfig, as defined by: TOOLCHAIN_KCONFIG_DIR and then that entry point can decide if other relevant Kconfig files exists which should be sourced.

[tejlmand]

As long as there's some way to configure this option, yes

@keith-packard if you're using ZEPHYR_TOOLCHAIN_VARIANT=cross-compile, then setting -DCONFIG_TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE=y on CMake invocation should do the same.
If you want to avoid typing this with CMake, then you could adjust the default Kconfig setting in a dedicated Kconfig file.

[tejlmand]

this is actually a lot of boilerplate being copied and new files added just for this.

We should create a cmake/toolchain/Kconfig file with content:

config TOOLCHAIN_$(ZEPHYR_TOOLCHAIN_VARIANT_UPPER)
	def_bool y

and then source it here:

--- a/Kconfig.zephyr
+++ b/Kconfig.zephyr
@@ -52,6 +52,7 @@ source "drivers/Kconfig"
 source "lib/Kconfig"
 source "subsys/Kconfig"
 
+source "cmake/toolchain/Kconfig"
 osource "$(TOOLCHAIN_KCONFIG_DIR)/Kconfig"
 
 menu "Build and Link Features"

remember to create an upper version and export to Kconfig similar to this:

+  ZEPHYR_TOOLCHAIN_VARIANT_UPPER=${ZEPHYR_TOOLCHAIN_VARIANT_UPPER}

zephyr/cmake/modules/kconfig.cmake

Line 129 in 8e18c5c

ZEPHYR_TOOLCHAIN_VARIANT=${ZEPHYR_TOOLCHAIN_VARIANT}

later we can then replace all uses of if ZEPHYR_TOOLCHAIN_VARIANT = "<name>" with just if TOOLCHAIN_<NAME> then deprecate and remove the old export.

[galak]

The issue with having:

config TOOLCHAIN_$(ZEPHYR_TOOLCHAIN_VARIANT_UPPER)
	def_bool y

Is that it doesn't allow a means to 'select' other Kconfig options based on what a toolchain supports or not.

For example, how would you do the select TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE than?

[tejlmand]

in such cases you can still add the following code to the toolchain specific Kconfig.

For example

config TOOLCHAIN_ARCMWDT
    select TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE

without a type on the TOOLCHAIN_ARCMWDT.

That has the advantage that if a toolchain is renamed (or a spelling mistake), then because there is no type on this setting, then Kconfig will complain.
This way it's ensured that the toolchain name used in CMake and Kconfig are always in sync.

Altenatively, because TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE is promptless (except in the cross-compile case) and <toolchain>/Kconfig is only sourced when that specific toolchain is in use, then the following is also possible:

config TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE
      default y

[galak]

I can introduce:

config TOOLCHAIN_$(ZEPHYR_TOOLCHAIN_VARIANT_UPPER)
	def_bool y

to handle the checking you suggested, I think we will still end up with a Kconfig. anyways.

I don't want to default TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE to y. I think in general toolchain featuers should be default n and have the toolchains explicitly select them.

[keith-packard]

yeah, the fact that disabling TLS support in gcc got cargo-culted into so many toolchains is really frustrating. It has zero impact on code not using TLS (at least on ARM). There shouldn't even be a GCC option to do this.

[tejlmand]

I think we will still end up with a Kconfig. anyways.

We might, but the main advantage is that we avoid settings getting out of sync on setting names created based on variables values in CMake, like the TOOLCHAIN_<NAME>.

I think in general toolchain featuers should be default n and have the toolchains explicitly select them.

If you noticed, it would be changing the default in a toolchain specific Kconfig: <toolchain>/Kconfig
and because it's promptless, then it's effectively behaves the same as a select.
The main difference between a select and a default in this particular case is that you can only select not de-select something.

Whereas, doing a conditional default n or default y on a promptless symbol is possible, offering the possibility to decide direction.

Also, changing the default can also allow toolchains to source / inherit a common setup as more toolchain features starts to appear, for example with toolchain adjusted Kconfig defaults we can do:

# toolchain/Kconfig.gnu_common
#
# gcc default supports those features:

config TOOLCHAIN_SUPPORTS_FEATURE_A
      default y

config TOOLCHAIN_SUPPORTS_FEATURE_B
      default n
...
config TOOLCHAIN_SUPPORTS_FEATURE_N
      default y

and then in a specific toolchain:

# <toolchain-specific>/Kconfig

# We don't support feature H
config TOOLCHAIN_SUPPORTS_FEATURE_H
      default n

# Rest of features are common with gcc
rsource "toolchain/Kconfig.gnu_common"

This is why the feature settings generally should be promptless, except for the cross-compile use-case.
That way it will be possible to have some common Kconfig which can be inherited, for example one for gnu another for llvm based toolchains, and then there can be made local adjustments in a toolchain specifc Kconfig if there is a need.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.