lib: os: PoT utilities, hash function, hash map (hash table)

[cfriedt]

Note: Compliance failure is a false positive

• Add a few Power-of-Two macros
  • IS_POWER_OF_TWO()
  • LOG2()
  • LOG2CEIL() for ceil(log2(x))
  • NHPOT() for 2^ceil(log2(x))
  • compile-time constant and runtime efficient
• Add tests for Power-of-Two macros
• Add a flexible means of specifying a system-wide 32-bit hash function
  • Reference implementation 1: Daniel J. Bernstein's algorithm (djb2)
  • Reference implementation 2: Murmur3
  • Add tests to verify uniformity (Kolmogorov Smirnov)
• Add a flexible means of specifying a system-wide Hashmap implementation
  • Allocator / hash function / load factor may be specified flexibly
  • Allocator should follow realloc() semantics, with free()-like behaviour when size 0 is specified
  • Implement a Separate-Chaining Hashmap
  • Implement an Open-Addressing Hashmap with Linear Probe
  • Add a C-Wrapper around std::unordered_map for benchmarking purposes
  • Performance of sys_hashmap_oa_lp on par with std::unordered_map
• Add tests to verify all operations of the Hashmap implementations
• Add samples to demonstrate / benchmark different Hashmap implementations

[cfriedt]

Need to also make test permutations for each of the different libc's because the default allocator is realloc() and consistent behaviour is required

[cfriedt]

@JordanYates - would you ack?

[cfriedt]

@carlocaione - look reasonable to you now? If so, please ack :-)

[marc-hb]

As just discovered by @aborisovich , constexpr now requires C++14. Is that intentional and documented? I didn't find anything in the commit messages or in the doc/ folder

zephyr/sys/util.h

#ifdef __cplusplus
template <typename T> static inline constexpr int __z_log2_impl(T x)

[cfriedt]

As just discovered by @aborisovich , constexpr now requires C++14. Is that intentional and documented? I didn't find anything in the commit messages or in the doc/ folder

zephyr/sys/util.h

#ifdef __cplusplus
template <typename T> static inline constexpr int __z_log2_impl(T x)

@marc-hb - this is specifically a workaround for g++ choking badly when encountering C11 Generics (clang++ does not). It is documented here. Feel free to make a PR to document it elsewhere if you like, or a PR to fix g++, or some other PR that you feel is more appropriate.

[marc-hb]

@cfriedt I'm not asking about any specific workaround or other detail.

https://docs.zephyrproject.org/latest/develop/languages/c/index.html clearly states "In summary, it is recommended to use a compiler toolchain that supports at least the C11 standard for developing with Zephyr"

https://docs.zephyrproject.org/latest/develop/languages/cpp/index.html does not seem to mention any specific C++ version right now. If your recent change requires a C++14 toolchain for any C++ use then it is significantly different from the C11 requirement (3 years newer!) and it should be mentioned on that page.

Zephyr has a very complex toolchain abstraction layer to support a broad range of toolchains, so bumping the requirements and excluding a number of old toolchains is a pretty big deal. A doc update really looks like a minimum.

Feel free to make a PR to document it elsewhere if you like,

Sorry but I'm not a C++ expert, I don't know which C++ feature is available in each C++ version and I'm not the one who just bumped some C++ requirement. In fact I'm merely trusting @aborisovich here who noticed because his C++ workspace just started failing somehow.

To make sure there's no confusion: I have no opinion on which C++ hard dependencies the Zephyr C++ "core" should have versus not (others might). On the other hand, if C++ requires a newer toolchain than C then a brief mention on https://docs.zephyrproject.org/latest/develop/languages/cpp/index.html looks like a bare minimum. Next time some advance notice would not hurt either.

[cfriedt]

https://docs.zephyrproject.org/latest/develop/languages/c/index.html clearly states "In summary, it is recommended to use a compiler toolchain that supports at least the C11 standard for developing with Zephyr"

@marc-hb - If your C compiler is ISO compliant, then anything contained within #ifdef __cplusplus should be filtered out by the preprocessor. If that's not the case, you may want to look for a different C compiler.

https://docs.zephyrproject.org/latest/develop/languages/cpp/index.html does not seem to mention any specific C++ version right now. If your recent change requires a C++14 toolchain for any C++ use then it is significantly different from the C11 requirement (3 years newer!) and it should be mentioned on that page.

The ISO C standard (C11) and the ISO C++ standard (C++14) are not very closely related.

I would defer to the C++ maintainer for that. CC @stephanosio.

Zephyr has a very complex toolchain abstraction layer to support a broad range of toolchains, so bumping the requirements and excluding a number of old toolchains is a pretty big deal. A doc update really looks like a minimum.

Again, feel free to make a PR.

Sorry but I'm not a C++ expert, I don't know which C++ feature is available in each C++ version and I'm not the one who just bumped some C++ requirement. In fact I'm merely trusting @aborisovich here who noticed because his C++ workspace just started failing somehow.

I am also not a C++ expert. Feel free to file an issue, @aborisovich.

To make sure there's no confusion: I have no opinion on which C++ hard dependencies the Zephyr C++ "core" should have versus not (others might). On the other hand, if C++ requires a newer toolchain than C then a brief mention on https://docs.zephyrproject.org/latest/develop/languages/cpp/index.html looks like a bare minimum. Next time some advance notice would not hurt either.

I'm sorry for the inconvenience. Thank you for bringing it up.

Just for clarification, constexpr has been part of the ISO C++ standard since C++11, which is the default supported by Zephyr (see lib/cpp/Kconfig). It took me only a few seconds to Google that. Please do the same before pointing fingers in a public forum.

[andyross]

Not clear on the details here. Is the contention that Zephyr headers after this patch aren't going to build with existing C++11 projects? If so that does indeed seem like something we should fix; we're essentially breaking user apps. If it's just an issue of constexpr that's probably something we can do with some preprocessor magic, we don't generally expose C++ APIs per se, though I do see a few here.

But then as @cfriedt points out, constexpr itself isn't new. What's the problem that got introduced?

[cfriedt]

@andyross - the root cause was that g++ flakes out on C11 generics, which I think logging has already ran into. So I essentially just borrowed logging's workaround of using C++ templates when using a c++ compiler. Technically, this is nothing new, and it's all supported by C11 / C++11. C++11 is the default for C++ in Zephyr (has been since Oct 2018). I'm not quite sure how long our public docs have urged users to use a C11 compliant compiler, but it's certainly been over a year.

#53405 (comment)

[andyross]

OK... I'm coming up to speed. So the issue here (please correct what I'm getting wrong) is that this PR introduces a new generic API implemented both with C11 _Generic and as a C++ template, selected based on source language.

That does seem like it's likely to break things with oddball toolchains, yeah. Maybe we want to slow-walk this a little and move the new bits out into their own header, so that zephyr/sys/util.h (which is included promiscuously by basically everything) gets to stay compatible for longer?

(Edit: heh, crossed. But yeah, I think I got it mostly right if I understand the explanation.)

[marc-hb]

Just for clarification, constexpr has been part of the ISO C++ standard since C++11, which is the default supported by Zephyr (see lib/cpp/Kconfig). It took me only a few seconds to Google that.

This very page is littered with references to later versions of the C++ standard, it took me a few seconds to see that. C++14 is referenced 10 times, C++17 is referenced 4 times, C++20 is referenced 20 times, C++23 is referenced 9 times.

Please do the same before pointing fingers in a public forum.

I've been "pointing fingers" at technical and documentation issues only and only where they happened. Thanks for doing the same.

Feel free to file an issue, @aborisovich.

Yes please.

[aborisovich]

@cfriedt nobody is pointing fingers, this is feedback from us because if external clients of Zephyr project discover that you suddenly require newer standard than before by compilation errors/warnings, that means you are doing something wrong.
There should be a announcement for this and the code that suddenly requires new C++ standard should be probably wrapped in ifdefs that check if the compiler's standard is newer than required or conditionally compiled sources in CMake checking also compiler capabilities.
As @marc-hb always indicates continuous integration should be continuous. And if it can not be, a meaningful and loud announcement should be provided to Zephyr project clients.
Please do so next time if similar situation would take place.

[cfriedt]

@cfriedt nobody is pointing fingers, this is feedback from us because if external clients of Zephyr project discover that you suddenly require newer standard than before by compilation errors/warnings, that means you are doing something wrong.

@aborisovich - respectfully, please file a bug report and report details of your issue rather than continuing here. This PR was thoroughly approved and merged.

There should be a announcement for this and the code that suddenly requires new C++

As mentioned above, this is all in line with Zephyr's suggested ISO C and C++ standards (C11, C++11).

standard should be probably wrapped in ifdefs that check if the compiler's standard is newer than required or conditionally compiled sources in CMake checking also compiler capabilities.

Good, then please make a PR. I take it you're using C++98?

As @marc-hb always indicates continuous integration should be continuous. And if it can not be, a meaningful and loud announcement should be provided to Zephyr project clients.

Please report an issue with whatever detail you have. Zephyr CI is fairly extensive and this passed without issue, which is enough confidence to merge it.

Please do so next time if similar situation would take place.

Please tell me @aborisovich - how am I supposed to know that your client will have a problem with their CI system. I'm sorry, I don't have ESP.

This is the definition of finger pointing.

Please refer to the Code of Conduct. Thanks.

https://github.com/zephyrproject-rtos/zephyr/blob/main/CODE_OF_CONDUCT.md

[cfriedt]

@cfriedt nobody is pointing fingers, this is feedback from us because if external clients of Zephyr project discover that you suddenly require newer standard than before by compilation errors/warnings, that means you are doing something wrong.

@aborisovich - you seem to be very upset about this.

Let me help you:

Point number 1.

Always frame your critique around the code, not the author. The latter makes you come off as being rude & demanding. "you are doing something wrong" as an example. It's probably better to say, this line of code is causing an issue - better yet, just put that in a bug report.

There should be a announcement for this

I'm having a hard time understanding why you think the Zephyr Project should change the publicly facing documentation because you discovered a bug that was specific to your project. Why would we make public announcements when someone can just apply a Bugfix? I can only suspect that it's to be able to point a finger.

Point Number 2

Bugs are a fact of life here. If you are not basing your product from a tagged release, then it is an inevitability that your code will break. If you are concerned with stability, use a stable release.

the code that suddenly requires new C++ standard should be probably wrapped in ifdefs that check if the compiler's standard is newer than required or conditionally compiled sources in CMake checking also compiler capabilities.

Point Number 3

Good - you have solved your own problem. For the Nth time, please, file a PR. An issue, sure, if you like.

As @marc-hb always indicates continuous integration should be continuous. And if it cannot be, a meaningful and loud announcement should be provided to Zephyr project clients

Lol.. no, my friend.

Point number 4

Sorry, but you seem to believe that just because we have CI it's a guarantee that there will never be a single obscure bug due to some wild permutation of config variables.

That's not how this works. If you believe otherwise, I welcome you to read the Apache 2.0 License.

Please do so next time if similar situation would take place.

This is exactly my point. Stop making demands. Stop telling me what to do. I do not work for you, and I pray I never do. Demanding that someone else fix a bunch of things for you is the wrong approach here.

Had you asked politely in the first place instead of assuming some authoritative role here, I would probably have made the 2-line change for you, which you yourself had described above.

Given all of the above, it makes me infinitely less likely to ever solve a problem for you.

So please, file a PR.

CC @carlescufi

[aborisovich]

@cfriedt this is not about you. I've submitted the bug to draw attention to the matter so we start to track standard conformance better in Zephyr. Please stop being defensive.

[jwakely]

G++ doesn't "choke badly", you are trying to use a C feature in C++, that's totally invalid C++ code.

[jwakely]

constexpr now requires C++14

constexpr requires C++11, but there are restrictions on what you can do in a C++11 constexpr function. C++14 relaxes some of the restrictions, and every later standard relaxes them further. But in the simplest form, constexpr only requires C++11.

The biggest restriction in C++11 is that a constexpr function can only have a single return statement, so any conditional code must be done using ?: conditional expressions (or template metaprogramming).

[jwakely]

Is it just me, or does g++ kind of flake-out bigtime when presented with C11 Generics? In contrast, clang++ handles them without any complaints whatsoever. This could pose a bit of a problem, @JordanYates, @oyvindronningstad .

I might need to revert to the XXX64() macros that I had before.

Update: used C++ templates with #ifdef __cplusplus

If the issue is simply wanting type-generic <math.h> functions, that has worked out of the box in C++ since the first 1998 standard, simply by using overloading. Every math function such as cbrt is overloaded by the C++ standard library's version of <math.h>:

#ifdef __cplusplus
extern "C" {
#endif
float cbrtf(float);
double cbrt(double);
long double (cbrtl(long double);
#ifdef __cplusplus
// Additional overloads for float and long double
inline float cbrt(float f) { return cbrtf(f); }
inline long double cbrt(long double f) { return cbrtl(f); }
} // extern "C"
#endif 

You don't need to do anything with C11 generics, and you don't need constexpr.

[cfriedt]

If the issue is simply wanting type-generic <math.h> functions, that has worked out of the box in C++ since the first 1998 standard, simply by using overloading. Every math function such as cbrt is overloaded by the C++ standard library's version of <math.h>:

@jwakely - That is not the issue encountered here but the simplest possible test case to show that g++ fails to parse _Generic while LLVM / clang does it with ease.

The actual code in question was for compile-time constant power-of-two macros.
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/sys/util.h#L595

You don't need to do anything with C11 generics, and you don't need constexpr.

Actually I do, and I do (but mainly as a workaround for g++ not supporting _Generic)

Bug was filed (and quickly marked invalid / closed) here
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108965

[jwakely]

That is not the issue encountered here but the simplest possible test case to show that g++ fails to parse _Generic while LLVM / clang does it with ease.

That just means clang has a non-standard extension. It's not a GCC bug that it doesn't support it. It's not in C++.

Bug was files (and quickly marked invalid / closed) here

Yes, I know - I closed it.

Looking at your __z_log2_impl implementation for C++, it should not be static (that leads to ODR violations if called from other inline functions or templates, i.e. undefined behaviour). And constexpr implies inline so that's redundant.

A valid C++11 implementation would be:

template <typename T> constexpr int __z_log2_impl(T x)
{
	/* No declaration of __ASSERT is available here */
	static_assert(sizeof(x) <= sizeof(unsigned long long), "unsupported type for LOG2()");

	return sizeof(x) < sizeof(unsigned int) ?
		((x) < 1) * (-1) +
		       ((x) >= 1) * (NUM_BITS(unsigned int) - __builtin_clz(x) - 1)
         : sizeof(x) <= sizeof(unsigned long long) ?
		((x) < 1) * (-1) +
		       ((x) >= 1) * (NUM_BITS(unsigned long long) - __builtin_clzll(x) - 1)
         : -1;
}

[jwakely]

Is omitting signed char intentional?

[cfriedt]

@jwakely - no... because .. it wasn't omitted?

[jwakely]

Yes it was. char and signed char are distinct types.

[cfriedt]

Thanks for clarifying - patches welcome!

[cfriedt]

That is not the issue encountered here but the simplest possible test case to show that g++ fails to parse _Generic while LLVM / clang does it with ease.

That just means clang has a non-standard extension. It's not a GCC bug that it doesn't support it. It's not in C++.

@jwakely Oh, ok. Then should I file an issue as a feature request? I didn't see that option in bugzilla. Would it receive any more attention than what it was given today?

Looking at your __z_log2_impl implementation for C++, it should not be static (that leads to ODR violations if called from other inline functions or templates, i.e. undefined behaviour). And constexpr implies inline so that's redundant.

A valid C++11 implementation would be:

Thanks for the tips!

[mbolivar-nordic]

Is there any chance that this discussion could continue in a new issue so that people who were subscribed to the PR do not continue to get updates that may not be relevant to them?

[marc-hb]

Is there any chance that this discussion could continue in a new issue so that people who were subscribed to the PR do not continue to get updates that may not be relevant to them?

@aborisovich filed #55204 two days ago and Github automatically added a link above. Its description seems very detailed and some discussion has already started there.

Anyone who feels the need to file yet another, separate issue should of course do so.

@andyross just submitted PR

• Unbreak builds with old C++ compilers #55290