libc: newlib/picolibc: define prototypes for non-ansi C functions #57619

galak · 2023-05-05T17:42:10Z

No description provided.

keith-packard

If you want GNU functions in you application, then define _GNU_SOURCE in your application. That's a well defined mechanism for getting libc extensions into your namespace.

stephanosio · 2023-05-05T17:52:29Z

If you want GNU functions in you application, then define _GNU_SOURCE in your application. That's a well defined mechanism for getting libc extensions into your namespace.

Same issue as the _POSIX_C_SOURCE; _GNU_SOURCE will end up pulling a lot of things we do not want (and not allowed by our coding guidelines once #57598 is in).

I only see this as a temporary workaround until we add the support for properly filtering the extensions allowed by Zephyr using something like __ZEPHYR_VISIBLE.

keith-packard · 2023-05-05T18:01:06Z

If you want GNU functions in you application, then define _GNU_SOURCE in your application. That's a well defined mechanism for getting libc extensions into your namespace.

Same issue as the _POSIX_C_SOURCE; _GNU_SOURCE will end up pulling a lot of things we do not want (and not allowed by our coding guidelines once #57598 is in).

I'm definitely not suggesting setting _GNU_SOURCE in any global header, just in specific source code (not core Zephyr) which want to use GNU extensions. That will effectively label exceptions to the Zephyr policy.

I only see this as a temporary workaround until we add the support for properly filtering the extensions allowed by Zephyr using something like __ZEPHYR_VISIBLE.

I'd suggest that things happen in the other order -- we map _ZEPHYR_SOURCE to _GNU_SOURCE for C libraries which don't have native _ZEPHYR_SOURCE support, then fix relevant libraries (including minimal C library) to support _ZEPHYR_SOURCE so that we can detect applications using names out of bounds for "standard Zephyr apps". Of course, external applications can still use _GNU_SOURCE to get whatever they want out of the C library.

stephanosio · 2023-05-05T18:08:35Z

I'm definitely not suggesting setting _GNU_SOURCE in any global header, just in specific source code (not core Zephyr) which want to use GNU extensions. That will effectively label exceptions to the Zephyr policy.

#57610 (comment)

galak · 2023-05-05T18:16:01Z

Same issue as the _POSIX_C_SOURCE; _GNU_SOURCE will end up pulling a lot of things we do not want (and not allowed by our coding guidelines once #57598 is in).

For example setting _GNU_SOURCE for qsort_r ends causing strerror_r to get defined as _GNU_SOURCE vs how we've defined as _POSIX_C_SOURCE.

I think this is the cleanest approach at this point.

stephanosio · 2023-05-05T18:26:06Z

For example setting _GNU_SOURCE for qsort_r ends causing strerror_r to get defined as _GNU_SOURCE vs how we've defined as _POSIX_C_SOURCE.

https://github.com/zephyrproject-rtos/newlib-cygwin/blob/4e150303bcc1e44f4d90f3489a4417433980d5ff/newlib/libc/include/stdlib.h#L310-L317

It looks like __BSD_VISIBLE is being defined too somehow? If neither __BSD_VISIBLE nor __GNU_VISIBLE is defined, qsort_r should be free for us to define.

stephanosio · 2023-05-05T18:36:16Z

It looks like __BSD_VISIBLE is being defined too somehow? If neither __BSD_VISIBLE nor __GNU_VISIBLE is defined, qsort_r should be free for us to define.

@galak Guess what ...

zephyr/tests/lib/mem_alloc/src/main.c

Line 27 in b004224

#define _BSD_SOURCE

p.s. Another reason to avoid requiring people to define their own _SOURCE.

keith-packard · 2023-05-05T18:49:06Z

p.s. Another reason to avoid requiring people to define their own _SOURCE.

I agree that apps shouldn't have to define various _SOURCE macros to access the defined Zephyr API, but applications accessing C library extensions that aren't part of the Zephyr API should use these mechanisms as that makes it clear what standards they refer to. Define _GNU_SOURCE and I know what documentation to go review to understand the semantics desired by the application. Don't define any _SOURCE stuff and I know that the application should only be using the Zephyr C library API.

Being able to test that the application respects the API it has requested is a separable issue, and one which can only be achieved by getting _ZEPHYR_SOURCE support into each C library. The current approach of defining _POSIX_C_SOURCE already expands the available C library API far beyond what Zephyr will use.

stephanosio · 2023-05-05T19:29:57Z

applications accessing C library extensions that aren't part of the Zephyr API should use these mechanisms as that makes it clear what standards they refer to.

As per the proposed coding guidelines in #57598, the idea is to not allow the upstream codebase to use the C library extensions that are not explicitly allowed by the coding guidelines; and for the allowed extensions, a specific (de-facto) standard will be designated (e.g. POSIX for strerror_r, GNU for qsort_r) so that there would be no confusion as to which "flavour" of the extension should be used.

Of course, downstream users and applications are not obligated to follow this and may choose to define _GNU_SOURCE and other feature test macros to pull in additional extensions, which can lead to the prototype mismatches as you suggest (e.g. Zephyr local libc header defining qsort_r for GNU, while application defines _BSD_SOURCE leading to a mismatch).

One way we could prevent that is by adding the feature test macro ifdefs to replicate exactly what each libc does (just copy and paste the block of code defining the function from the original libc header) -- while it is not a pretty solution, it will keep the Zephyr codebase more clean overall by not requiring each source file to define the feature test macros like _GNU_SOURCE for the officially allowed C library extensions, which really should be available by default (and they will be available by default when using the minimal libc or other libcs pulling the common libc implementation).

stephanosio · 2023-05-05T19:32:30Z

lib/libc/newlib/include/stdint.h

+void qsort_r(void *base, size_t nmemb, size_t size,
+	     int (*compar)(const void *, const void *, void *), void *arg);


As per #57619 (comment), in order to support downstream applications that may choose to pull in a different flavour (e.g. BSD) of this function, it may be desirable to pull in the full definition here:

https://github.com/zephyrproject-rtos/newlib-cygwin/blob/4e150303bcc1e44f4d90f3489a4417433980d5ff/newlib/libc/include/stdlib.h#L308-L318

I think we should cross that bridge if/when someone wants to do that.

keith-packard · 2023-05-05T20:18:39Z

Ok, this is getting messy -- It sounds like you want the system, when not directed with some _SOURCE definition, to declare some functions that differ between POSIX and GNU to use POSIX semantics and still offer some functions that are only available in GNU systems?

Maybe I just need to understand the desired end point. I think an explicit goal should be that code which defines _POSIX_C_SOURCE, _BSD_SOURCE or _GNU_SOURCE should get the API as specified by those relevant standards. Code not defining any of these should get what we can call _ZEPHYR_SOURCE behavior, which would offer a mixture of POSIX, BSD, C and GNU APIs.

As to the process, I'd much rather the underlying root problems get fixed than layer kludges on top. Maybe newlib would be just fine taking _ZEPHYR_SOURCE patches? Obviously getting those into non-SDK environments would take time, but is there a huge rush to fix a few API corner cases in Zephyr?

stephanosio · 2023-05-05T21:09:18Z

It sounds like you want the system, when not directed with some _SOURCE definition, to declare some functions that differ between POSIX and GNU to use POSIX semantics and still offer some functions that are only available in GNU systems?

Yes; at least, based on the status quo of the non-standard functions available in the minimal libc and have been used throughout the Zephyr codebase, which will likely end up in the list of "allowed" non-ISO functions.

Maybe I just need to understand the desired end point. I think an explicit goal should be that code which defines _POSIX_C_SOURCE, _BSD_SOURCE or _GNU_SOURCE should get the API as specified by those relevant standards. Code not defining any of these should get what we can call _ZEPHYR_SOURCE behavior, which would offer a mixture of POSIX, BSD, C and GNU APIs.

Yes, that sounds like a fairly accurate description of what we want eventually.

As to the process, I'd much rather the underlying root problems get fixed than layer kludges on top. Maybe newlib would be just fine taking _ZEPHYR_SOURCE patches? Obviously getting those into non-SDK environments would take time, but is there a huge rush to fix a few API corner cases in Zephyr?

For Newlib, there is not much of a rush and it will certainly be preferable if we upstream the _ZEPHYR_SOURCE support.

Nevertheless, there remains the problem of maintaining compatibility with older versions of toolchains that do not include this patch (not just Zephyr SDK, but third-party toolchains such as GNU Arm Embedded).

lib/libc/newlib/include/stdint.h

keith-packard · 2023-05-05T22:19:54Z

Here's an equivalent series for picolibc. Note that this currently requires that Zephyr explicitly define _ZEPHYR_SOURCE, which is probably not what we want (but is closest to how _GNU_SOURCE works today).

https://github.com/zephyrproject-rtos/picolibc/tree/zephyr-source

galak · 2023-05-05T23:21:39Z

Here's an equivalent series for picolibc. Note that this currently requires that Zephyr explicitly define _ZEPHYR_SOURCE, which is probably not what we want (but is closest to how _GNU_SOURCE works today).

https://github.com/zephyrproject-rtos/picolibc/tree/zephyr-source

Would be good to default to the POSIX version of strerror_r when _ZEPHYR_SOURCE is defined.

keith-packard · 2023-05-06T03:16:38Z

Here's an equivalent series for picolibc. Note that this currently requires that Zephyr explicitly define _ZEPHYR_SOURCE, which is probably not what we want (but is closest to how _GNU_SOURCE works today).
https://github.com/zephyrproject-rtos/picolibc/tree/zephyr-source

Would be good to default to the POSIX version of strerror_r when _ZEPHYR_SOURCE is defined.

I was hoping @cfriedt would figure out if you wanted the POSIX or GNU version of this. I'll stick the POSIX version in this series and we can review.

keith-packard · 2023-05-06T03:38:50Z

Note that this currently requires that Zephyr explicitly define _ZEPHYR_SOURCE, which is probably not what we want (but is closest to how _GNU_SOURCE works today).

I've fixed this -- if you have __ZEPHYR__ defined, then _ZEPHYR_SOURCE is selected unless you've explicitly defined one of the other API selectors.

lib/libc/newlib/include/string.h

lib/libc/picolibc/include/string.h

keith-packard · 2023-05-08T17:59:59Z

I've added commits to pr #57340 which pulls in the picolibc Zephyr API changes from https://github.com/zephyrproject-rtos/picolibc/tree/zephyr-source, fix a couple of unneeded _*_SOURCE uses and then removes the _POSIX_C_SOURCE definition when building with picolibc.

cfriedt

A couple of copy-paste typos by the looks of it

lib/libc/picolibc/include/stdlib.h

lib/libc/newlib/include/stdlib.h

keith-packard · 2023-05-09T17:08:01Z

I think we should pull the picolibc changes out of this series and see if we can land the upstream fixes instead. We should be able to get those fixes back-ported to current picolibc bits used in the SDK and module to avoid introducing other picolibc changes into the environment.

galak · 2023-05-09T17:25:41Z

I think we should pull the picolibc changes out of this series and see if we can land the upstream fixes instead. We should be able to get those fixes back-ported to current picolibc bits used in the SDK and module to avoid introducing other picolibc changes into the environment.

The issue is we need the picolibc changes for older SDKs that wouldn't have the new picolibc changes.

Define prototypes for qsort_r, strnlen, strtok_r, and strerror_r as these are not part of the standard ANSI defined C-lib. As newlib & picolibc provided both the GNU and POSIX style of strerror_r we have to redefine strerror_r to __xpg_strerror_r to get the POSIX version. Signed-off-by: Kumar Gala <[email protected]>

This should now be defined in strings.h for newlib and picolibc so don't need to define it here. Signed-off-by: Kumar Gala <[email protected]>

keith-packard · 2023-05-17T16:24:39Z

lib/libc/newlib/include/string.h

+#endif
+
+size_t strnlen(const char *, size_t);
+char  *strtok_r(char *, const char *, char **);


Does the compiler complain when you add identifier names here and the application has defined _POSIX_C_SOURCE so that newlib also declares these functions? If not, adding them to make the compliance checks pass would be nice (the names serve as a limited form of documentation if chosen well).

I would like to add feature test macros soon on the POSIX side too, so it's good to be aware that it's in the pipes

keith-packard

Please test with _POSIX_C_SOURCE (and _GNU_SOURCE?) defined in various ways to make sure these don't conflict with the newlib definitions. I'd actually encourage you to create per-function tests using those so we can catch mis-matching prototypes. I was concerned about that aspect of this patch, but with tests like that, we'll be safe.

github-actions · 2023-07-17T00:35:02Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

cfriedt · 2023-08-03T15:21:40Z

dev-review: @galak - will you be moving forward with this one? If not, it might make sense to close

MaureenHelm · 2023-09-21T15:14:06Z

dev-review: @galak - will you be moving forward with this one? If not, it might make sense to close

@galak ping

keith-packard · 2023-09-21T16:21:09Z

In particular, the picolibc changes to expose the Zephyr API when __ZEPHYR__ is defined during the build have been available for some time now.

github-actions · 2023-11-21T00:29:57Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

galak requested review from nashif, dcpleung and andyross as code owners May 5, 2023 17:42

galak requested review from keith-packard and stephanosio May 5, 2023 17:42

zephyrbot added area: C Library C Standard Library area: Formatting Output labels May 5, 2023

zephyrbot requested review from cfriedt and nordic-krch May 5, 2023 17:42

zephyrbot assigned stephanosio May 5, 2023

galak mentioned this pull request May 5, 2023

libc: newlib: switch from ANSI_SOURCE to POSIX_C_SOURCE config #57610

Closed

keith-packard reviewed May 5, 2023

View reviewed changes

stephanosio reviewed May 5, 2023

View reviewed changes

stephanosio mentioned this pull request May 5, 2023

Make picolibc the default C library for Zephyr #49922

Closed

60 tasks

keith-packard reviewed May 5, 2023

View reviewed changes

lib/libc/newlib/include/stdint.h Outdated Show resolved Hide resolved

cfriedt reviewed May 6, 2023

View reviewed changes

lib/libc/newlib/include/string.h Outdated Show resolved Hide resolved

lib/libc/picolibc/include/string.h Outdated Show resolved Hide resolved

galak force-pushed the libc-hdr-cleanup branch from 8d179ab to e7b0bfa Compare May 8, 2023 14:47

galak requested review from keith-packard, cfriedt and stephanosio May 8, 2023 14:48

cfriedt reviewed May 8, 2023

View reviewed changes

lib/libc/picolibc/include/stdlib.h Outdated Show resolved Hide resolved

lib/libc/newlib/include/stdlib.h Outdated Show resolved Hide resolved

galak added 2 commits May 9, 2023 12:26

lib: os: cbprintf: Remove prototype for strnlen

762d0bb

This should now be defined in strings.h for newlib and picolibc so don't need to define it here. Signed-off-by: Kumar Gala <[email protected]>

galak force-pushed the libc-hdr-cleanup branch from e7b0bfa to 762d0bb Compare May 9, 2023 17:27

galak requested a review from cfriedt May 9, 2023 17:27

galak mentioned this pull request May 16, 2023

Add coding guidelines on C standard library function usage #57598

Merged

keith-packard reviewed May 17, 2023

View reviewed changes

github-actions bot added Stale and removed Stale labels Jul 17, 2023

github-actions bot added the Stale label Nov 21, 2023

github-actions bot closed this Dec 5, 2023

aescolar mentioned this pull request Jan 31, 2024

[RFC] On the way C libraries (don't) expose all functions defined in coding guideline rules A.4 and A.5 #68278

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libc: newlib/picolibc: define prototypes for non-ansi C functions #57619

libc: newlib/picolibc: define prototypes for non-ansi C functions #57619

galak commented May 5, 2023

keith-packard left a comment

stephanosio commented May 5, 2023

keith-packard commented May 5, 2023

stephanosio commented May 5, 2023

galak commented May 5, 2023

stephanosio commented May 5, 2023

stephanosio commented May 5, 2023 •

edited

Loading

keith-packard commented May 5, 2023

stephanosio commented May 5, 2023

stephanosio May 5, 2023

galak May 5, 2023

keith-packard commented May 5, 2023

stephanosio commented May 5, 2023

keith-packard commented May 5, 2023

galak commented May 5, 2023

keith-packard commented May 6, 2023

keith-packard commented May 6, 2023 •

edited

Loading

keith-packard commented May 8, 2023

cfriedt left a comment

keith-packard commented May 9, 2023

galak commented May 9, 2023

keith-packard May 17, 2023

cfriedt Jul 17, 2023

keith-packard left a comment

github-actions bot commented Jul 17, 2023

cfriedt commented Aug 3, 2023

MaureenHelm commented Sep 21, 2023

keith-packard commented Sep 21, 2023

github-actions bot commented Nov 21, 2023

		void qsort_r(void *base, size_t nmemb, size_t size,
		int (compar)(const void , const void , void ), void *arg);

libc: newlib/picolibc: define prototypes for non-ansi C functions #57619

libc: newlib/picolibc: define prototypes for non-ansi C functions #57619

Conversation

galak commented May 5, 2023

keith-packard left a comment

Choose a reason for hiding this comment

stephanosio commented May 5, 2023

keith-packard commented May 5, 2023

stephanosio commented May 5, 2023

galak commented May 5, 2023

stephanosio commented May 5, 2023

stephanosio commented May 5, 2023 • edited Loading

keith-packard commented May 5, 2023

stephanosio commented May 5, 2023

stephanosio May 5, 2023

Choose a reason for hiding this comment

galak May 5, 2023

Choose a reason for hiding this comment

keith-packard commented May 5, 2023

stephanosio commented May 5, 2023

keith-packard commented May 5, 2023

galak commented May 5, 2023

keith-packard commented May 6, 2023

keith-packard commented May 6, 2023 • edited Loading

keith-packard commented May 8, 2023

cfriedt left a comment

Choose a reason for hiding this comment

keith-packard commented May 9, 2023

galak commented May 9, 2023

keith-packard May 17, 2023

Choose a reason for hiding this comment

cfriedt Jul 17, 2023

Choose a reason for hiding this comment

keith-packard left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 17, 2023

cfriedt commented Aug 3, 2023

MaureenHelm commented Sep 21, 2023

keith-packard commented Sep 21, 2023

github-actions bot commented Nov 21, 2023

stephanosio commented May 5, 2023 •

edited

Loading

keith-packard commented May 6, 2023 •

edited

Loading