libc: minimal: add qsort to the minimal libc

[cfriedt]

This change implements qsort() for the minimal libc via Heapsort.

Heapsort time complexity is O(n log(n)) in the best, average, and worst cases. It is O(1) in space complexity (i.e. sorts in-place) and is iterative rather than recursive. Heapsort is not stable (i.e. does not preserve order of identical elements).

On cortex-m0, this implementation occupies ~240 bytes.

Fixes #28896

[cfriedt]

Since this is a standard C library function that is missing from the minimal C library, I would almost consider it a bug that it is not present. Would make a lot of sense, IMHO, to add this to LTS in the v2.7-branch, as it does not break any API (and actually improves API consistence), and fixes several issues where it has been mentioned as lacking.

I would be happy to present it to the TSC and / or CRB.

[cfriedt]

Now as for whether qsort() should be implemented by the minimal C library, it is debatable; but, if there is enough demand for it, I am not against adding it. Its implementation, however, should be minimal such that it is very simple, low footprint and easily maintainable.

From what I've gathered, there have been a few separate issues that have called for it

• undefined reference to `bsearch' #stdlib #15159
• add qsort as libc extension  #28896
• libc: minimal: add qsort routine #31107
• subsys: greybus: remove qsort implementation cfriedt/greybus-for-zephyr#63 - (a separate patch is made on top of Zephyr for this purpose)

After all, we are not trying to roll our own C library -- doing so would be immensely expensive and analogous to reinventing the wheel. The scope of the minimal libc should be limited to "keeping the boat afloat" and any advanced features should be provided by one of the supported "full C libraries" (e.g. newlib).

Agreed - it would be ideal if we used a single libc that would satisfy most requirements, but it's also nice to have some consistence. qsort() can be small enough that they do not add much of a footprint, and of course, if it is unused, it will be discarded by the linker.

Would it make more sense to just use Heapsort in this case? That's easily maintainable and should meet the requirements of most use cases (with the exception that it is not stable). I added the choice so that users could choose to use the bsd qsort that was under samples/ or an even smaller insertion sort if that served the purpose, but if the main purpose of "minimal" means "minimal maintenance" then Heapsort is probably a good enough solution.

As for the long-term objectives, we should look into adopting one of the existing lightweight open source C libraries to replace our current minimal C library (possibly picolibc, but we should keep searching for better alternatives).

Yes, agreed as well.

[stephanosio]

Some minor comments

[npitre]

You have:

/*
 * Normally parent is defined parent(k) = floor((k-1) / 2) but we can avoid a
 * divide by noticing that floor((k-1) / 2) = (k >> 1).
 */
#define parent(k) ((k) >> 1)

Here I can't ignore the fact that floor((k-1) / 2) is not equal to
(k >> 1). If used with 4, the former gives 1 while the later gives 2.

Then:

/*
 * Normally left is defined left(k) = (2 * k + 1) but we can avoid a
 * multiply by noticing that (2 * k + 1) = ((k << 1) + 1).
 */
#define left(k) (((k) << 1) + 1)

The +1 in the macro does match the comment, but it doesn't match its
parent() counterpart defined above. Incideltally, removing that +1 doesn't
appear to affect the algorithm, at least as far as the test suite is concerned.

Might be worth verifying the theory if that +1 could indeed be removed which
would 1) make the code coherent and 2) reduce the generated binary slightly.

And finally:

/*
 * Normally right is defined right(k) = (2 * (k) + 2) but we can avoid a
 * multiply by noticing that right(k) = left(k) + 1 = child + 1.
 */
#define right(root) (child + 1)

Here I really suggest you stick to left(k) + 1. It is very counter-intuitive
to have the macro argument being ignored in favor of some arbitrary variable.
And I bet the generated code will be the same anyway, or just as efficient.

[cfriedt]

/*
 * Normally parent is defined parent(k) = floor((k-1) / 2) but we can avoid a
 * divide by noticing that floor((k-1) / 2) = (k >> 1).
 */
#define parent(k) ((k) >> 1)

Here I can't ignore the fact that floor((k-1) / 2) is not equal to
(k >> 1). If used with 4, the former gives 1 while the later gives 2.

Good catch. Was editing slightly too hastily. It should be (k-1) >> 1.

And finally:

/*
 * Normally right is defined right(k) = (2 * (k) + 2) but we can avoid a
 * multiply by noticing that right(k) = left(k) + 1 = child + 1.
 */
#define right(root) (child + 1)

Here I really suggest you stick to left(k) + 1. It is very counter-intuitive
to have the macro argument being ignored in favor of some arbitrary variable.
And I bet the generated code will be the same anyway, or just as efficient.

Good suggestion - thanks 😊

[frantony]

I have faced with link error on experimental MIPS port after 2cf08cd merging, please see 2ef72de.