-
Notifications
You must be signed in to change notification settings - Fork 6.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
posix: header fixups #51771
posix: header fixups #51771
Conversation
Compliance check fails due to 4 warnings of long lines (comments). I would rather keep those lines as they are. |
125ac99
to
c0be0a8
Compare
|
What's your migration plan? Would be nice if we could do one before the other instead of requiring synchronous updates? |
I think really the only "odd duck" is The exact primitives that would need to be defined by Additionally, a features header would need to be created and included (likely right after the config header). |
libc "owns" all of the headers for syscalls --
Yup, newlib and picolibc both provide a posix-conforming header with all of those definitions, and those are used while building the C library itself, so we should try to use them while building application code as well to avoid potential type mis-matches. One trick is that the application code will want to use the static inline versions of these functions. If that application code included a Zephyr-specific header defining the inlines before the associated libc header, that would actually work. For that, it seems like there are two choices:
Option 2 would allow using unmodified POSIX code in a Zephyr environment without any performance penalty, and would mean applications wouldn't accidentally end up with non-inline versions of these functions (which need to exist for internal libc uses).
Yeah, I think this is a reasonable plan -- have applications define _ZEPHYR_SOURCE (or something) and have that cause libc headers to include a Zephyr-specific header that can define the POSIX wrappers around Zephyr API and also set the correct feature-test macros so that libc will expose a Zephyr API to applications. As a transition plan, I think Zephyr should expose a pure POSIX ABI surface so that applications including unmodified libc headers will work correctly, then a Zephyr header which affects libc behavior can be created, and even tested by having the test code explicitly include it. Finally, a modified libc which includes the Zephyr header at the right place can be introduced into the Zephyr toolchain, along with patches delivered upstream so that external toolchains will also include the right stuff. |
Unfortunately, that approach is going to be problematic for various reasons:
IMHO, we should disable all POSIX aspects of the libc so that they only provide the ISO C features (i.e. no POSIX primitives). All POSIX definitions and implementations should be provided SOLELY by the Zephyr POSIX subsystem, without mixing up the POSIX bits from here and there -- that has always been the problem causing various issues and will continue to be a problem unless we make a clear separation between the ISO C library and the POSIX support/library (i.e. Zephyr POSIX subsystem). Any POSIX-isms inside the libc (picolibc and newlib) should be abstracted away such that they can be replaced with direct Zephyr API calls. This is especially important because the POSIX support is an optional feature for Zephyr and we need the libc to be fully usable without enabling the Zephyr POSIX subsystem. Related discussion on Discord: https://discord.com/channels/720317445772017664/883450954878439445/1036035108416794714 |
That's not really a viable choice -- libc uses OS interfaces to provide many of the standard C APIs (e.g. stdio). Picolibc uses POSIX interfaces for this, and so it must be built using a standard POSIX header that defines those APIs. As Zephyr currently allows/encourages the C library to be included in the toolchain, and that toolchain may be built without regard to Zephyr OS interfaces, the only workable solution is the one we use today where libc provides the POSIX interface declarations for its own compilation, and then we expect Zephyr to provide matching definitions.
You can't have that without providing a replacement API that libc can use to provide standard C functionality. POSIX happens to be convenient because it's well specified and matches the underlying requirements for libc reasonably well. Picolibc documents which POSIX APIs are required for various parts of the library; if you don't need file I/O or time interfaces, then you don't actually need any POSIX APIs in the underlying implementation. But, that's not often the case with other libcs -- Newlib has no such provisions and requires significant POSIX APIs for any use of stdio, portions of the locale code and other areas in the library. I don't see any realistic alternatives to the current situation unless you add Zephyr-specific functionality to the C library itself so that it can use Zephyr APIs instead of POSIX APIs. I'd be happy to do that in Picolibc, but I think that'd be a harder sell in some other toolchains. |
@keith-packard - what I meant by "owned" is that
I'd prefer not getting into implementation details / optimizations yet (i.e. inlines vs non-inlines). This is really just to pay off technical debt and organize the POSIX subsystem in a more sustainable way so that it's future proof. Personally, I think that the Newlib (and subsequently PicoLibc) way of doing it is quite good (mature, portable, and standards compliant). So posix features are pulled in, and then all of the It's also a lot of work to do create those headers, declarations, etc, from scratch, so I would like to avoid that if possible. From what I understand, newlib is MIT / BSD-like licensed, and therefore permissive, so actually copying the relevant POSIX headers (and sections) into Zephyr's POSIX subsystem is not a bad solution. At least in that sense, we would have POSIX headers in Zephyr that are done properly, that are also consistent with Newlib and PicoLibc. Additionally, I think we can use that to eventually meet in the middle to satisfy @stephanosio's preference that the C library and POSIX subsystem have mostly mutual-exclusive headers. While I kind of loathe the Again, my focus here is sustainability. It makes zero sense from my perspective to do work that will simply perpetuate more work (i.e. technical debt). I would like to pay my future self and the future selves of all of the maintainers.
We already have
I actually agree with this entirely, but I don't think @stephanosio does. However, I think I can satisfy both by making some incremental modifications to the Zephyr POSIX subsystem implementation. I hope both you (both @keith-packard and @stephanosio) can bear with me through this, because it may be a bit involved, but the payoff will likely be very worth it. My thoughts:
|
FYI, some additional discussion happened on this matter on Discord: Some excerpts:
|
I've seen the chat - how is that relevant at the moment? Actually, let's chat on Discord - it's not really helpful to copy-paste messages from Discord to GH. |
c0be0a8
to
9090db5
Compare
03fc0ec
to
1c91bb5
Compare
Temporarily build this PR against zephyrproject-rtos/zephyr#51771 until that pull has been accepted to main. Signed-off-by: Chris Friedt <[email protected]>
Temporarily build this PR against zephyrproject-rtos/zephyr#51771 until that pull has been accepted to main. Signed-off-by: Chris Friedt <[email protected]>
Temporarily build this PR against zephyrproject-rtos/zephyr#51771 until that pull has been accepted to main. Signed-off-by: Chris Friedt <[email protected]>
Temporarily build this PR against zephyrproject-rtos/zephyr#51771 until that pull has been accepted to main. Signed-off-by: Chris Friedt <[email protected]>
Temporarily build this PR against zephyrproject-rtos/zephyr#51771 until that pull has been accepted to main. Signed-off-by: Chris Friedt <[email protected]>
Temporarily build this PR against zephyrproject-rtos/zephyr#51771 until that pull has been accepted to main. Signed-off-by: Chris Friedt <[email protected]>
Temporarily build this PR against zephyrproject-rtos/zephyr#51771 until that pull has been accepted to main. Signed-off-by: Chris Friedt <[email protected]>
The POSIX spec requires that `SO_LINGER`, `SO_RCVLOWAT`, and `SO_SNDLOWAT`, and `SOMAXCONN` are defined in `<sys/socket.h>`. However, most of the existing socket options and related constants are defined in `<zephyr/net/socket.h>`. For now, we'll co-locate them. It would be good to properly namespace things. Additionally, a no-op for setsockopt for `SO_LINGER` to make things Just Work (TM) for now. Signed-off-by: Chris Friedt <[email protected]>
Add a trivial suite that simply ensures headers exist and that they supply standard symbols and constants. These tests are intended to be ordered exactly as the respective feature appears in the respective specification at https://pubs.opengroup.org/onlinepubs/9699919799 Over time, as POSIX support improves, we can enable additional checks. If `CONFIG_POSIX_API=n`, then we simply ensure that the header can be included, that constants and structures exist, including the existence of required fields in each structure. We check that a constant exist, by comparing its value against an arbitrary number. If the constant does not exist, it would of course be a compile error. ``` zassert_not_equal(-1, POLLIN); ``` We check that a structure contains required fields by comparing the field offset with an arbitrary number. If the field does not exist, of course, there would be a compile error. ``` zassert_not_equal(-1, offsetof(struct pollfd, fd)); ``` For non-scalar constants, we simply attempt to assign a value to the specific type: ``` struct in6_addr any6 = IN6ADDR_ANY_INIT; ``` If `CONFIG_POSIX_API=y`, then we additionally check that required functions are non-NULL (limited to what is currently supported in Zephyr). ``` zassert_not_null(pthread_create); ``` Note: functional verification tests should be done outside of this test suite. Signed-off-by: Chris Friedt <[email protected]>
Only include `<fcntl.h>` for `CONFIG_ARCH_POSIX`. Otherwise, include `<zephyr/posix/fcntl.h>`. Signed-off-by: Chris Friedt <[email protected]>
`<fcntl.h>`, `<net/if.h>`, and `<netinet/tcp.h>` were missing `extern "C" { .. }"` which is required to avoid C++ name mangling. Signed-off-by: Chris Friedt <[email protected]>
e4320a1
to
759dc8d
Compare
@rlubos - I had to add a no-op to process |
20 commits:
CONFIG_POSIX_API=y
posix_sched.h
tosched.h
pthread_key.h
as more of an implementation detail than a standard includegetopt()
by default withCONFIG_POSIX_API=y
eventfd()
by default withCONFIG_POSIX_API=y
open()
with Newlib or PicoLibc_exit()
in<unistd.h>
CONFIG_NET_SOCKETS_POSIX_NAMES=y
struct mq_attr
ioctl()
function prototypein_port_t
andin_addr_t
<netinet/in.h>
in<arpa/inet.h>
to definein_port_t
andin_addr_t
EAI_SYSTEM
and other<netdb.h>
constantsFIONREAD
as some applications require itstruct linger
insys/socket.h
for spec complianceNI_MAXSERV
for applications that want oneSO_LINGER
,SO_RCVLOWAT
,SO_SNDLOWAT
, andSOMAXCONN
for spec compliance