libc/common: Add memalign

[keith-packard]

Memalign is another name for the posix aligned_alloc function, although it has weaker restrictions on the relationship between the alignment and size.

memalign appears to be used internally by the G++, so we need to provide an implementation of this when using C++, even though it's not part of the Zephyr C library API.

Closes: #57899

[galak]

tweak ifdef to include __GNUC__

[cfriedt]

Just realized that memalign is nonstandard. I won't block, but likely @stephanosio will have some input.

In any case, I appreciate the effort to improve C++ support :-)

[keith-packard]

Just realized that memalign is nonstandard. I won't block, but likely @stephanosio will have some input.

Yup, that's why it's not defined in any header file and is only made available with using G++. It's provided as part of the G++ ABI requirements for the C library, but it's not part of the Zephyr C library API.

In any case, I appreciate the effort to improve C++ support :-)

I'm constantly surprised what people will do with C++.

[stephanosio]

memalign appears to be used internally by the G++, so we need to provide an implementation of this when using C++, even though it's not part of the Zephyr C library API.

I do not think the right solution is to add the non-standard memalign support just for the purpose of appeasing GNU libstdc++, especially when we already support the standard aligned_alloc

Moreover, libstdc++ should be using aligned_alloc when it is available:

namespace __gnu_cxx {
#if _GLIBCXX_HAVE_ALIGNED_ALLOC
using ::aligned_alloc;
[...]
#elif _GLIBCXX_HAVE_MEMALIGN
static inline void*
aligned_alloc (std::size_t al, std::size_t sz)
{
  // Solaris requires al >= sizeof a word and QNX requires >= sizeof(void*)
  // but they both provide posix_memalign, so will use the definition above.
  return memalign (al, sz);
}
[...]
  while ((p = __gnu_cxx::aligned_alloc (align, sz)) == nullptr)

Something is causing _GLIBCXX_HAVE_ALIGNED_ALLOC to be not defined and the libstdc++ is falling back to using memalign, which is a configuration issue and needs to be root-caused and fixed there.

[stephanosio]

Something is causing _GLIBCXX_HAVE_ALIGNED_ALLOC to be not defined and the libstdc++ is falling back to using memalign, which is a configuration issue and needs to be root-caused and fixed there.

I have created zephyrproject-rtos/sdk-ng#675 to track this.

[keith-packard]

We can fix the SDK, but we cannot fix other gcc-based toolchains. How can we tell when a fixed version of the SDK is being used?

[stephanosio]

We can fix the SDK, but we cannot fix other gcc-based toolchains.

Correct.

First of all, we probably should consider upstreaming zephyrproject-rtos/gcc#22, once it is verified working, so that it propagates to all third-party GNU toolchain distributions.

After that, we can go about this in one of two ways: a) require using the version of Zephyr SDK or a third-party GNU toolchain that includes the patch, or b) provide the solution proposed in this PR as a workaround for the third-party GNU toolchains that do not have the patch.

IMHO, given that this will not be the last time we encounter issues like this, my preference is towards the option 'a'.

How can we tell when a fixed version of the SDK is being used?

The only way to do that would be to check the Zephyr SDK version.

[keith-packard]

First of all, we probably should consider upstreaming zephyrproject-rtos/gcc#22, once it is verified working, so that it propagates to all third-party GNU toolchain distributions.

Yup, I'm already planning on doing that. Given the usual gcc merge process, I expect this to take between a week and infinite weeks.

After that, we can go about this in one of two ways: a) require using the version of Zephyr SDK or a third-party GNU toolchain that includes the patch, or b) provide the solution proposed in this PR as a workaround for the third-party GNU toolchains that do not have the patch.

IMHO, given that this will not be the last time we encounter issues like this, my preference is towards the option 'a'.

I don't think we can responsibly adopt that solution at this point -- that would rule out using any existing GCC-based c++ toolchain with Zephyr. We can (and should) plan on removing this kludge when the upstream patch is upstream and included in a GCC release.

How can we tell when a fixed version of the SDK is being used?

The only way to do that would be to check the Zephyr SDK version.

Sounds like we need a Kconfig variable driving the inclusion of memalign() in the common C library that will be set when using a toolchain which might require it? That seems pretty simple.

[stephanosio]

that would rule out using any existing GCC-based c++ toolchain with Zephyr. We can (and should) plan on removing this kludge when the upstream patch is upstream and included in a GCC release.

I am afraid that is an inevitable future if we want to properly support libstdc++ in the future (e.g. synchronisation, threading support), unless the third-party GNU toolchain is specifically built for the *-zephyr target.

Sounds like we need a Kconfig variable driving the inclusion of memalign() in the common C library that will be set when using a toolchain which might require it? That seems pretty simple.

Actually, I have another solution in mind. Since we are only adding memalign() for the purpose of appeasing GNU libstdc++ (and not for the purpose of supporting it as part of the "Zephyr C API"), we should just add as part of the "GNU libstdc++ integration layer" in lib/cpp/glibcxx/libcpp-hook.c or something.

That will allow the memalign() to be only defined when GLIBCXX_LIBCPP is selected (i.e. when libstdc++ is used), which is what we want.

[stephanosio]

Yup, I'm already planning on doing that. Given the usual gcc merge process, I expect this to take between a week and infinite weeks.

Haha.

[keith-packard]

That will allow the memalign() to be only defined when GLIBCXX_LIBCPP is selected (i.e. when libstdc++ is used), which is what we want.

Any reason to not stick that test in the common C library malloc.c file instead of the current one?

[stephanosio]

Any reason to not stick that test in the common C library malloc.c file instead of the current one?

Because memalign() is not supposed to be a libc function that Zephyr supports or is used anywhere in Zephyr. It is analogous to libc-hooks.c defining functions like __retarget_lock_init_recursive that is used by the Newlib (or Picolibc for that matter).

[keith-packard]

Because memalign() is not supposed to be a libc function that Zephyr supports or is used anywhere in Zephyr. It is analogous to libc-hooks.c defining functions like __retarget_lock_init_recursive that is used by the Newlib (or Picolibc for that matter).

I've started implementing this idea and it doesn't seem very pretty -- it depends on configurations that use both the common C library malloc and libstdc++, so you have a cross-subsystem dependency with either implementation.

The other issue is that implementing memalign() in terms of aligned_alloc() is not generally valid as aligned_alloc() imposes additional restrictions on the parameters: the size requested must be a multiple of the alignment. Writing memalign() in terms of a a pure ISO/IEC C interface will cause significant additional memory usage for applications as allocations will need to be padded to a larger size.

Given these two issues, I'd like you to reconsider this suggestion and place memalign() in the common C library malloc implementation making its presence in the resulting binary conditional upon using libstdc++ (via a check for CONFIG_GLIBCXX_LIBCPP).

[stephanosio]

I've started implementing this idea and it doesn't seem very pretty -- it depends on configurations that use both the common C library malloc and libstdc++, so you have a cross-subsystem dependency with either implementation.

Yes, we will have cross-subsystem dependency either way; under lib/libc or lib/cpp.

The other issue is that implementing memalign() in terms of aligned_alloc() is not generally valid as aligned_alloc() imposes additional restrictions on the parameters: the size requested must be a multiple of the alignment. Writing memalign() in terms of a a pure ISO/IEC C interface will cause significant additional memory usage for applications as allocations will need to be padded to a larger size.

I thought we were only talking about the common libc aligned_alloc(), which is not affected by this, since we need this workaround only when using the common libc. Am I missing something here?

The reason that I say this belongs in lib/cpp/glibcxx/libcpp-hook.c (or something equivalent) is that this is strictly for the purpose of supporting the libstdc++ and nothing else (i.e. it is not meant to be a libc function, but a libstdc++ support function).

memalign in this case is nothing more than an "internal interface" between the libstdc++ and the operating system (as __retarget_lock_init_recursive is an internal interface between Newlib and the operating system). The only thing about memalign here that makes it as though it should belong in the libc is its name.

That is why I say it would be more logical to put it in the libstdc++ support layer than in a libc.

[keith-packard]

I thought we were only talking about the common libc aligned_alloc(), which is not affected by this, since we need this workaround only when using the common libc. Am I missing something here?

No, you're correct, but this makes it different from other 'support' code in lib/cpp as it is a linked dependency between the common C library and libstdc++; you need the common C library to provide this interface whenever it is being used by libstdc++. As you cannot use this implementation with any other C library, it feels disingenuous to have it exist outside of the common C library implementation -- it's only within the context of the common C library that this implementation is valid, so I really think it needs to be placed in that code and not someplace where it could conceivably get adopted for use with some other C library where it cannot work.

The reason that I say this belongs in lib/cpp/glibcxx/libcpp-hook.c (or something equivalent) is that this is strictly for the purpose of supporting the libstdc++ and nothing else (i.e. it is not meant to be a libc function, but a libstdc++ support function).

But it's a support function that that depends upon internal details about the implementation of aligned_alloc() as provided by the common C library, not a support function that exists upon standard C or Zephyr interfaces, which makes it very different from code found in the libc support code (like libc-hooks.c).

memalign in this case is nothing more than an "internal interface" between the libstdc++ and the operating system (as __retarget_lock_init_recursive is an internal interface between Newlib and the operating system). The only thing about memalign here that makes it as though it should belong in the libc is its name.

Yup, memalign will not be visible in header files to Zephyr applications, so they should not have access to it. If it could be implemented purely in terms of standard functions like a ISO/IEC C aligned_alloc(), or standard Zephyr functions, then I'd be all for having it placed in lib/cpp/glibcxx.

[stephanosio]

But it's a support function that that depends upon internal details about the implementation of aligned_alloc() as provided by the common C library, not a support function that exists upon standard C or Zephyr interfaces, which makes it very different from code found in the libc support code (like libc-hooks.c).

As long as we add #ifdef CONFIG_COMMON_LIBC_MALLOC (and that should be fine since we only need this when the common libc is used), I think it equally makes sense to place this on the libstdc++ integration layer side as a libstdc++ support function -- for instance, we do not have aligned_alloc inside sys_heap because it depends on the internal details about how sys_heap_aligned_alloc is implemented.

But yeah, it would seem both approaches are almost equally as valid depending on how you look at them, and this is a matter of opinion.

Yup, memalign will not be visible in header files to Zephyr applications, so they should not have access to it. If it could be implemented purely in terms of standard functions like a ISO/IEC C aligned_alloc(), or standard Zephyr functions, then I'd be all for having it placed in lib/cpp/glibcxx.

Sure, I am fine with placing memalign in the common libc as long as it is not visible in the header files, and the comments make it clear that it is only there for supporting the GNU libstdc++ and is not meant to be used by the application code -- we do not want people submitting PRs saying "malloc.h was missing memalign function declaration."

[keith-packard]

Sure, I am fine with placing memalign in the common libc as long as it is not visible in the header files, and the comments make it clear that it is only there for supporting the GNU libstdc++ and is not meant to be used by the application code -- we do not want people submitting PRs saying "malloc.h was missing memalign function declaration."

Sorry, I hadn't pushed the update I'd made to the comments in the file yesterday which (I hope) address this concern. Thanks for your review; it's always a challenge to know where to stick kludges like this which don't fit the desired abstraction layering.

[stephanosio]

Looks good. Just a minor comment about a typo.

[cfriedt]

No object from me. @galak any further thoughts here?

please revisit