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

pico-sdk: Avoid using 'typeof()' operator #9

Open
wants to merge 1 commit into
base: zephyr
Choose a base branch
from

Conversation

soburi
Copy link
Member

@soburi soburi commented Jan 25, 2025

C99 doesn't support the typeof() operator used in Pico-SDK. So we need to rewrite using hw_xor_alias code with hw_xor_alias_untyped().

C99 doesn't support the `typeof()` operator used in Pico-SDK.
So we need to rewrite using `hw_xor_alias` code with
`hw_xor_alias_untyped()`.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
@ajf58
Copy link
Collaborator

ajf58 commented Jan 25, 2025

This feels like going in the wrong direction. typeof has been a compiler extension for decades, and is now in C23. I'd like to minimise the changes made to the Pico SDK to support Zephyr.

https://github.com/pdgendt @pdgendt might be able to offer some guidance. There seems to be support for enabling GNU extensions as a per-module, and that would be a good fit for this, I think.

@pdgendt
Copy link

pdgendt commented Jan 25, 2025

@pdgendt might be able to offer some guidance. There seems to be support for enabling GNU extensions as a per-module, and that would be a good fit for this, I think.

Not sure what you need here, modules can require a minimum C standard version with select REQUIRES_STD_CXX, there isn't an option for requiring GNU extensions, but you could add that 🤷

@ajf58
Copy link
Collaborator

ajf58 commented Jan 25, 2025

@pdgendt might be able to offer some guidance. There seems to be support for enabling GNU extensions as a per-module, and that would be a good fit for this, I think.

Not sure what you need here, modules can require a minimum C standard version with select REQUIRES_STD_CXX, there isn't an option for requiring GNU extensions, but you could add that 🤷

I think this is simpler than I thought.

I want to avoid changing the source code, and change how Zephyr builds hal_rpi_pico instead. It looks like this is simply done in modules/hal_rpi_pico/CMakeLists.txt. E.g. I think we should do this:

# SPDX-License-Identifier: Apache-2.0

include(ExternalProject)

if(CONFIG_HAS_RPI_PICO)
  zephyr_library()
  zephyr_library_compile_options(-std=gnu11)           <---- Add this here, then building the Pico SDK won't require these changes.

  set(rp2_common_dir ${ZEPHYR_HAL_RPI_PICO_MODULE_DIR}/src/rp2_common)
...

-std=gnu11 is supported by the version of gcc in 0.16.x SDK, and onwards.

@soburi
Copy link
Member Author

soburi commented Jan 25, 2025

We use BSP to reduce development costs, so I think it needs to be reasonable from that perspective.
(We are not required to retain commonality with Pico-SDK. The cost is the only reason we do so.)

In the scope of this case, the change is only to avoid compilation errors for code that does not actually run, so I think there is not enough benefit to introducing new constraints for retain the original code.

I don't necessarily think that we should adhere to C99, but raising the level of the C standard would still strengthen the constraints.
If you grep, you can see that the cases in which STD_C restrictions are used in Zephyr are extremely limited. (nanopb and native_posix are only cases.) Overall, Zephyr maintains compatibility with C99. I believe that considerable rationalization is necessary for their implementation.
This is an option that needs to be dealt with conservatively.

To put it directly, I believe maintaining compatibility with C99 is more valuable than preserving code commonality with the Pico-SDK.

@ajf58
Copy link
Collaborator

ajf58 commented Jan 26, 2025

raising the level of the C standard would still strengthen the constraints.

I'm sorry, I don't really understand this sentence. The Zephyr Project builds its own build toolchain, and ships it as part of Zephyr-SDK. The version of gcc built into the current LTS SDK is:

arm-zephyr-eabi-gcc.exe (Zephyr SDK 0.16.5-1) 12.2.0

It supports -std=gnu11. If the project doesn't want to support a standard, why is it building it into its toolchain?

If you grep, you can see that the cases in which STD_C restrictions are used in Zephyr are extremely limited. (nanopb and native_posix are only cases.)

That wasn't what I was suggesting in the example I gave. Please re-read the snippet in #9 (comment) . There are several examples

Overall, Zephyr maintains compatibility with C99. I believe that considerable rationalization is necessary for their implementation.
This is an option that needs to be dealt with conservatively.

-std=gnu11 is a requirement for the Linux kernel. typeof is supported by the Zephyr toolchain, and has been in every compiler for... 20+(?) years. It's even in C23.

Changing the Pico SDK in a piecemeal way like this is going to be a way to introduce maintenance burden: unless someone changes everything, it's going to result in multiple pairs of PRs (one updating the HAL, then another that changes the west manifest). I really think we should just use -std=gnu11, like the Pico SDK authors intended

@soburi
Copy link
Member Author

soburi commented Jan 27, 2025

Simply put, we need to follow Zephyr's customs.

Since very few things in Zephyr specify anything other than STD_C99, you will need to clearly explain why exceptional correspondence is needed.
In other words, it requires a strong reason, such as the absence of a workaround or the impossibility of practical maintenance.

-std=gnu11 is a requirement for the Linux kernel. typeof is supported by the Zephyr toolchain, and has been in every compiler for... 20+(?) years. It's even in C23.

Rather than whether it can be done, I think it is necessary to justify specifying STD_C when other things do not.

Changing the Pico SDK in a piecemeal way like this is going to be a way to introduce maintenance burden: unless someone changes everything, it's going to result in multiple pairs of PRs (one updating the HAL, then another that changes the west manifest). I really think we should just use -std=gnu11, like the Pico SDK authors intended

The need to do so has already arisen, and as you can see from the maintenance of other HALs, such actions are commonplace. Also, it is within the realm of realistic costs at this point.

Zephyr is not Linux kernel, and also Zephyr is not Pico-SDK.
We should consider how to support it based on the fact that "Zephyr generally supports up to C99."
There are examples of NATIVE_POSIX up to C11, so there seems to be room for compromise, but C23 is too radical.

@carlescufi

I have one question. I think we should maintain C99 compatibility as much as possible, but do you have any policy on this point?

@soburi
Copy link
Member Author

soburi commented Jan 27, 2025

Ah, I remember now.
The reason it's based on C99 is because Zephyr's coding rules are based on MISRA-C 2012, which is written based on C99.
If the reason is it, we can't use standards newer than C99, and we can't introduce GNU extensions.

@pdgendt
Copy link

pdgendt commented Jan 27, 2025

Zephyr does not only care about it's own SDK, you can use a different toolchain, see zephyrproject-rtos/zephyr#84119 where support for GCC 4.1.2 is kept possible.

@carlescufi
Copy link
Member

carlescufi commented Jan 31, 2025

@carlescufi

I have one question. I think we should maintain C99 compatibility as much as possible, but do you have any policy on this point?

@soburi any toolchain building Zephyr must support C99 as of today, but C11 or later cannot be required in general, because as @pdgendt states, the codebase is being built by older toolchains. However, the doc clearly states that, for some modules or functionality, it is allowed to require C11. So, in this particular case, since this only applies to Raspberry Pi Pico, I would tend to agree that requiring C11 is not an issue. Copying @kartben and @nashif in case they have other opinions.

@carlescufi
Copy link
Member

It supports -std=gnu11. If the project doesn't want to support a standard, why is it building it into its toolchain?

@ajf58 as @soburi pointed out, Zephyr supports multiple toolchains, not only the ones bundled in the SDK. Here's a good example of this:
zephyrproject-rtos/zephyr#84119

@kartben
Copy link

kartben commented Jan 31, 2025

would it make sense to also contribute the patch upstream too? I am sure this hw_xor_alias macro has a good reason to exist, but on the other hand this seems to be the only place it's used in the HAL

@ajf58
Copy link
Collaborator

ajf58 commented Jan 31, 2025

It supports -std=gnu11. If the project doesn't want to support a standard, why is it building it into its toolchain?

@ajf58 as @soburi pointed out, Zephyr supports multiple toolchains, not only the ones bundled in the SDK. Here's a good example of this: zephyrproject-rtos/zephyr#84119

Right, but I don't think that answer my question. If Zephyr doesn't want to support gnu11, why does Zephyr's custom-built gcc in Zephyr-SDK offer it as an option?

I share @andyross 's bemusement about this. Is someone picking up a part released in 2021, trying to build software written in 2022, with a compiler that doesn't support a standard from 2011?

Isn't it reasonable for a vendor to stipulate "If you want to run Zephyr on our products, you need a toolchain that was release in the past... 14... years."? C23 added typeof, so it's not like we're going against the grain here. If I have to wait 25 years to use a C standard feature then I'll have retired!

Edit: corrected my typos

@carlescufi
Copy link
Member

Right, but I don't think that answer my question. If Zephyr doesn't want to support gnu11, why does Zephyr's custom-built gcc in Zephyr-SDK offer it as an option?

@stephanosio would be better placed to answer this, but I believe this is because it makes little sense to disable features in GCC regardless of what the RTOS can use by default. Also, as stated above and in the doc, some modules and features do require C11 today, so the toolchain must support that as well.

I share @andyross 's bemusement about this. Is someone picking up a part released in 2021, trying to build software written in 2022, with a compiler that doesn't support a standard from 2011?

I agree, but things are more complicated than that. Some vendors (Xtensa cough cough) or old architectures (see my link above to the Nvidia PR) simply do not have modern compilers available, but the silicon or IP is still being used widely.

@soburi
Copy link
Member Author

soburi commented Jan 31, 2025

@carlescufi
I have one question. I think we should maintain C99 compatibility as much as possible, but do you have any policy on this point?

@soburi any toolchain building Zephyr must support C99 as of today, but C11 or later cannot be required in general, because as @pdgendt states, the codebase is being built by older toolchains. However, the doc clearly states that, for some modules or functionality, it is allowed to require C11. So, in this particular case, since this only applies to Raspberry Pi Pico, I would tend to agree that requiring C11 is not an issue. Copying @kartben and @nashif in case they have other opinions.

Thank you for your explanation.
I am also able to agree to introduce C11 std.
IMO, even if we are considering functional safety certification, we can go up to C11, and if conditions permit, we can go up to C18 because these are supported by MISRA-C 2023.
Conversely, GNU-specific extensions are an unpreferable option from this perspective.

But pico-sdk requires C23 or GNU extensions...

@carlescufi
Copy link
Member

But pico-sdk requires C23 or GNU extensions...

Oh wow I see. @stephanosio any preferences here?

@ajf58
Copy link
Collaborator

ajf58 commented Feb 1, 2025

But pico-sdk requires C23 or GNU extensions...

Oh wow I see. @stephanosio any preferences here?

I don't think this is a dramatic thing.
What I'm saying is "Everyone for 25+ years has had typeof. Now, finally, it's part of the C standard. Instead of pretending we must support MSVC pre-2023, let's use the tools we have to avoid some of the footguns of C. Yes it was a GNU extension. No, that's not a problem."

What I really want to avoid is fettling with Pico-SDK beyond the bare minimum. I think it's wasted effort: Raspberry Pi built the SDK using -std=gnu11, Zephyr-SDK toolchain supports it, therefore we should use it. This is consistent with #9 (comment) "

@carlescufi
Copy link
Member

carlescufi commented Feb 1, 2025

What I really want to avoid is fettling with Pico-SDK beyond the bare minimum. I think it's wasted effort: Raspberry Pi built the SDK using -std=gnu11, Zephyr-SDK toolchain supports it, therefore we should use it. This is consistent with #9 (comment) "

No objections from me to require -std=gnu11 for the Pico SDK.

@ThreeEights
Copy link

I have to agree with @carlescufi and @ajf58: Avoid Zephyr-specific changes to the Pico SDK, and require the newer language standard when building for the Pico boards. This seems to me to be the most consistent with the C Language Support :

In summary, it is recommended to use a compiler toolchain that supports at least the C11 standard for developing with Zephyr. It is, however, important to note that some optional Zephyr components and external modules may make use of the C language features that have been introduced in more recent versions of the standards, in which case it will be necessary to use a more up-to-date compiler toolchain that supports such standards.

@soburi
Copy link
Member Author

soburi commented Feb 4, 2025

From the perspective of a "developer" of rpi_pico, I think it makes sense to make an exception for rpi_pico's hal and specify the specific "--std=gnu11",
but I'm concerned about the sufficiency of the MISRA-C and the impact of obtaining functional safety certification, which is what Zephyr is aiming for.
I hope there are no problems with this.

@carlescufi

Does this seem like an issue from this perspective? Or do you know of any experts who would be good to consult?

@kartben
Copy link

kartben commented Feb 5, 2025

From the perspective of a "developer" of rpi_pico, I think it makes sense to make an exception for rpi_pico's hal and specify the specific "--std=gnu11",
but I'm concerned about the sufficiency of the MISRA-C and the impact of obtaining functional safety certification, which is what Zephyr is aiming for.
I hope there are no problems with this.

@carlescufi

Does this seem like an issue from this perspective? Or do you know of any experts who would be good to consult?

@soburi HALs are very much out of scope of the safety certification efforts as far as I know :)
Safety certification only concerns "core" Zephyr features

1738715664000.png

@soburi
Copy link
Member Author

soburi commented Feb 5, 2025

@kartben

From the perspective of a "developer" of rpi_pico, I think it makes sense to make an exception for rpi_pico's hal and specify the specific "--std=gnu11",
but I'm concerned about the sufficiency of the MISRA-C and the impact of obtaining functional safety certification, which is what Zephyr is aiming for.
I hope there are no problems with this.
@carlescufi
Does this seem like an issue from this perspective? Or do you know of any experts who would be good to consult?

@soburi HALs are very much out of scope of the safety certification efforts as far as I know :) Safety certification only concerns "core" Zephyr features

1738715664000.png

Thank you for explaining.
It doesn't seem to be any problem for now, so I changed zephyrproject-rtos/zephyr#84974 to add -std=gnu11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants