libc: add strerror() to minimal libc #16527

PiotrZierhoffer · 2019-05-31T12:26:35Z

Based on musl implementation.

From IEEE Std 1003.1-2017:
The strerror() function shall map the error number in errnum to a
locale-dependent error message string and shall return a pointer to it.

This implementation does not take locales information into account, as
this concept is not present in Zephyr.

Signed-off-by: Piotr Zierhoffer [email protected]

zephyrbot · 2019-05-31T12:28:36Z

Found the following issues, please fix and resubmit:

License issues

In most cases you do not need to do anything here, especially if the files
reported below are going into ext/ and if license was approved for inclusion
into ext/ already. Fix any missing license/copyright issues. The license
exception if a JFYI for the maintainers and can be overriden when merging the
pull request.

lib/libc/minimal/source/string/__strerror.h is not apache-2.0 licensed: mit
lib/libc/minimal/source/string/strerror.c is not apache-2.0 licensed: mit

nashif · 2019-05-31T12:37:02Z

can you provide some context on why you are adding all those libc functions?

Based on musl implementation. From IEEE Std 1003.1-2017: The strerror() function shall map the error number in errnum to a locale-dependent error message string and shall return a pointer to it. This implementation does not take locales information into account, as this concept is not present in Zephyr. Signed-off-by: Piotr Zierhoffer <[email protected]>

PiotrZierhoffer · 2019-05-31T13:11:54Z

can you provide some context on why you are adding all those libc functions?

This is required for integration with civetweb.

We will, most likely, submit some time related functions, float parsing and sscanf as well.

I used code based on musl, as it is already present in Zephyr.

pfalcon · 2019-05-31T14:23:04Z

can you provide some context on why you are adding all those libc functions?

+1

This is required for integration with civetweb.

Unfortunately, the idea that you can build POSIX applications (like civetweb) with non-existent libc (like minlibc) is flawed. You can't, because (full) C library is unalienable part of POSIX. Please just use newlib. Whoever won't follow that advice, will effectively build a copy of newlib. Likely, in "frankenstein style"copy, pulling parts from different libc's around. All that someone will then have to maintain. And so far, we can't maintain well even integration with more or less integral libc like newlib.

I used code based on musl, as it is already present in Zephyr.

Good to know, because pulling in non-Apache2 licenced code into Zephyr usually means going thru Zephyr TSC discussion (even if it's MIT/BSD license which everyone knows compatible with Apache2).

PiotrZierhoffer · 2019-06-03T11:33:34Z

We tried to use Newlib, but it doesn't work with the POSIX APIs in Zephyr - it doesn't compile due to a large number of issues like symbol redefinitions etc.

Since Zephyr's documentation (https://github.com/zephyrproject-rtos/zephyr/blob/master/doc/guides/c_library.rst) states that you can either use Newlib or extend minimal-libc, we went for the latter.

pfalcon · 2019-06-03T12:02:42Z

We tried to use Newlib, but it doesn't work with the POSIX APIs in Zephyr

As explained above, the opposite is also true - POSIX API doesn't (can't) work without newlib (or it's not POSIX API, but very limited and holey thing).

it doesn't compile due to a large number of issues like symbol redefinitions etc.

Well, https://github.com/zephyrproject-rtos/zephyr/tree/master/tests/posix are 2 examples where "POSIX API" works well with newlib.

However, if you formulate the situation as "if you step aside from whatever POSIX API samples are included in Zephyr, all the hell breaks loose", that would pinpoint the situation well. That happens because people who initially implemented parts of POSIX API didn't think how it all would fit together into consistent a POSIX system. Well, that will need to be thought out sooner or later.

One possible "alternative" would be:

Ignore the situation.
Make another box within Zephyr and keep adding adhoc things based on particular and peculiar needs to it.

But p.2 is exactly how "POSIX API" was developed so far, so it would be too optimistic to think that any different results will come out from p.2.

Since Zephyr's documentation (https://github.com/zephyrproject-rtos/zephyr/blob/master/doc/guides/c_library.rst) states that you can either use Newlib or extend minimal-libc, we went for the latter.

IMHO, it's a bug in the docs, it doesn't take into consideration that POSIX subsystem has special needs and requirements.

pfalcon · 2019-06-04T15:58:48Z

Summing up discussion at the networking forum (from my PoV):

Between choices of improving minimal libc vs adding adhoc workarounds to "civitweb sample", improving minimal libc is definitively the preferred choice.
But then "minimal libc" should be, well, minimal, and this implementation adds a bunch of strings to ROM. Having stuff like E(EDOM, "EDOM") would be a good choice for minimal libc IMHO.
In the context of civetweb porting, this is still more like a workaround, instead of using (and improving) newlib.

pfalcon · 2019-06-04T15:59:56Z

instead of using (and improving) newlib.

It was said that there're issues with, like symbol conflicts trying to use POSIX_API and newlib, but I don't believe I saw any bugreport. Is there any?

pfalcon · 2019-06-04T16:00:44Z

libc: add strerror() to minimal libc

I'd say, better title would be "libc: minimal: add strerror()".

rakons · 2019-06-05T06:57:11Z

@pfalcon
I cannot agree only about the 2nd point of your list. This would add strings into ROM only if you use strerror function. And it should be optimized out even if you do disable garbage collection in linker - as all that strerror requires is placed in a separate file. So as far as I know - it would be removed if nothing references to any element inside this file.

pfalcon · 2019-06-05T07:02:23Z

I cannot agree only about the 2nd point of your list. This would add strings into ROM only if you use strerror function.

Yeah, and if you use it, it adds a bunch of bloat, whereas a function from "minimal libc" could take care to be minimal, as suggested above.

rakons

I sometimes really missing strerror functionality. The problem is that Zephyr is complex enough that it takes time to judge witch errno.h file is really used, especially if trying to quickly debug someones other code.

What I really would love to see here is a configuration between:

Return descriptive error message (like here)
Return error mnemonic - so EINVAL would return "EINVAL"
Disable - return an empty string.

rakons · 2019-06-05T06:54:55Z

lib/libc/minimal/source/string/strerror.c

+
+#include <errno.h>
+
+#define E(a, b) ((unsigned char)a),


Do not cast to unsigned char here - you are removing a compiler warning if any code does not fit 8 bit (I get that it should be not an issue, but it seems not to make any sense to remove this warning if it happens).

rakons · 2019-06-05T06:58:31Z

lib/libc/minimal/source/string/__strerror.h

+E(EALREADY,     "Operation already in progress")
+E(EINPROGRESS,  "Operation in progress")
+
+E(0,            "No error information")


0 - operation finished successfully?
Moreover - the current implementation of strerror function requires this one to be the last one. It has to be commented or the implementation should be changed.

rakons · 2019-06-05T07:02:38Z

lib/libc/minimal/source/string/strerror.c

+{
+	int i;
+
+	for (i = 0; errid[i] && errid[i] != e; i++) {


why not:

for (i=0; i<ARRAY_SIZE(errid); i++) { if (errid[i] == e) { return errmsg[i]; } } return "No error description found";

This way no special mark inside the errid has to be implemented.

rakons · 2019-06-05T07:04:08Z

lib/libc/minimal/source/string/strerror.c

+	for (i = 0; errid[i] && errid[i] != e; i++) {
+		/* intentionally left blank */
+	}
+	return (char *)errmsg[i];


I cannot agree about removing "const" modifier like here. It is very risky. I would prefer to have a non-standard strerror implementation that returns const char* instead of such a dirty trick.

rakons · 2019-06-05T07:22:09Z

@pfalcon

"minimal libc" could take care to be minimal, as suggested above.

I agree. This is why I have added a proposal of the configurable strerror in my review above.

pfalcon · 2019-06-05T08:20:11Z

This is why I have added a proposal of the configurable strerror in my review above.

Sorry, but even I would consider that scope creep (a typical problem with Zephyr reviews). This patch adds strerror() implementation to minimal libc, whereas there was none before. It should be judged based on that, not on what else it might have been. That's why I personally don't add -1 here, even though I think that the approach taken by this patch is wrong on several accounts. Even if that's true (and it's just my opinion), in the end this patch does add proper and useful functionality which wasn't available before.

Someone needing additional things from strerror() mostly likely will need to follow up with further patches on top of this one.

rakons · 2019-06-05T08:58:59Z

Why creep? Just change:

#define E(a, b) b,

To:

#if STRERROR_FULL
#define E(a, b) b,
#else
#define E(a, b) #a,
#endif

PiotrZierhoffer · 2019-08-22T11:56:39Z

The discussion went far away from the original problem, but as this PR is not about to be accepted, I'll close it

PiotrZierhoffer requested review from andrewboie and nashif as code owners May 31, 2019 12:26

PiotrZierhoffer force-pushed the strerror branch from dcadded to 98c4461 Compare May 31, 2019 13:07

aescolar added the area: C Library C Standard Library label Jun 3, 2019

rakons suggested changes Jun 5, 2019

View reviewed changes

PiotrZierhoffer mentioned this pull request Jun 7, 2019

[RFC] Missing parts of libc required for CivetWeb #16683

Closed

PiotrZierhoffer closed this Aug 22, 2019

libc: add strerror() to minimal libc #16527

libc: add strerror() to minimal libc #16527

Uh oh!

Conversation

PiotrZierhoffer commented May 31, 2019

Uh oh!

zephyrbot commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License issues

Uh oh!

nashif commented May 31, 2019

Uh oh!

PiotrZierhoffer commented May 31, 2019

Uh oh!

pfalcon commented May 31, 2019

Uh oh!

PiotrZierhoffer commented Jun 3, 2019

Uh oh!

pfalcon commented Jun 3, 2019

Uh oh!

pfalcon commented Jun 4, 2019

Uh oh!

pfalcon commented Jun 4, 2019

Uh oh!

pfalcon commented Jun 4, 2019

Uh oh!

rakons commented Jun 5, 2019

Uh oh!

pfalcon commented Jun 5, 2019

Uh oh!

rakons left a comment

Choose a reason for hiding this comment

Uh oh!

rakons Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

rakons Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

rakons Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

rakons Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

rakons commented Jun 5, 2019

Uh oh!

pfalcon commented Jun 5, 2019

Uh oh!

rakons commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiotrZierhoffer commented Aug 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zephyrbot commented May 31, 2019 •

edited

Loading

rakons commented Jun 5, 2019 •

edited

Loading