Skip to content
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

Merged
merged 20 commits into from
Jan 23, 2023
Merged

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Oct 28, 2022

20 commits:

  • allow POSIX headers to be on the default include path with CONFIG_POSIX_API=y
  • rename posix_sched.h to sched.h
  • treat pthread_key.h as more of an implementation detail than a standard include
  • enable getopt() by default with CONFIG_POSIX_API=y
  • enable eventfd() by default with CONFIG_POSIX_API=y
  • avoid conflicting aliases for open() with Newlib or PicoLibc
  • declare _exit() in <unistd.h>
  • avoid redefinition errors in POSIX headers with CONFIG_NET_SOCKETS_POSIX_NAMES=y
  • do not typedef struct mq_attr
  • declare an ioctl() function prototype
  • define in_port_t and in_addr_t
  • include <netinet/in.h> in <arpa/inet.h> to define in_port_t and in_addr_t
  • add EAI_SYSTEM and other <netdb.h> constants
  • define FIONREAD as some applications require it
  • define struct linger in sys/socket.h for spec compliance
  • provide a reasonable NI_MAXSERV for applications that want one
  • add SO_LINGER, SO_RCVLOWAT, SO_SNDLOWAT, and SOMAXCONN for spec compliance
  • add tests for standard POSIX headers, constants, structs, fields, and functions
  • net: sockets: conditionally include zephyr/posix/fcntl.h
  • use ifdef __cplusplus extern C in standard headers

@cfriedt
Copy link
Member Author

cfriedt commented Oct 28, 2022

Compliance check fails due to 4 warnings of long lines (comments). I would rather keep those lines as they are.

@cfriedt
Copy link
Member Author

cfriedt commented Oct 28, 2022

  • updated test name in testcase.yaml
  • commented-out pthread_mutexattr_setprotocol check

@keith-packard
Copy link
Collaborator

What's your migration plan? Would be nice if we could do one before the other instead of requiring synchronous updates?

@cfriedt
Copy link
Member Author

cfriedt commented Oct 29, 2022

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 <time.h> because that is owned by the libc. It's obviously not a new problem, but I think we want to use the libc's <time.h> and then ensure that all other primitives are defined by the POSIX subsystem.

The exact primitives that would need to be defined by <time.h> are here.

Additionally, a features header would need to be created and included (likely right after the config header).

@keith-packard
Copy link
Collaborator

I think really the only "odd duck" is <time.h> because that is owned by the libc. It's obviously not a new problem, but I think we want to use the libc's <time.h> and then ensure that all other primitives are defined by the POSIX subsystem.

libc "owns" all of the headers for syscalls -- fcntl.h, for open(2), unistd.h for read(2), write(2), close(2) and lseek(2), stdlib.h for _Exit(2), and of course sys/time.h for gettimeofday(2). The one I've had the most trouble with is actually stat(2) as that uses struct stat {} which is very os-specific, and yet the header (sys/stat.h) is provided by libc.

The exact primitives that would need to be defined by <time.h> are here.

* https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/time.h.html

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:

  1. Require applications wanting the static inline versions of the POSIX APIs to include a Zephyr-specific header before the POSIX header.
  2. Hack up the POSIX headers provided by libc to include a Zephyr header while building within the Zephyr environment.

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).

Additionally, a features header would need to be created and included (likely right after the config header).

* https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02

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.

@stephanosio
Copy link
Member

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.

Unfortunately, that approach is going to be problematic for various reasons:

  1. A lot of potential for conflicts between the POSIX primitives defined by the libc and the Zephyr POSIX subsystem -- once the Zephyr POSIX subsystem starts relying on the POSIX bits from libc, we will need a lot of libc-specific handling to make it work properly (every libc does this differently); with multiple libc support, this scheme will be analogous to opening a Pandora's box.
  2. ISO C does not require POSIX and there are C libraries that do not support the POSIX features (e.g. the libc included in third-party toolchains such as armclang). We do not want to make the Zephyr POSIX subsystem rely on the libc to provide any POSIX primitives because such C libraries will not provide them. In other words, we need the Zephyr POSIX subsystem to provide all POSIX-related definitions and implementations anyway.

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

@keith-packard
Copy link
Collaborator

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).

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.

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.

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.

@cfriedt
Copy link
Member Author

cfriedt commented Oct 31, 2022

I think really the only "odd duck" is <time.h> because that is owned by the libc. It's obviously not a new problem,

libc "owns" all of the headers for syscalls -- fcntl.h, for open(2), unistd.h for read(2), write(2), close(2) and lseek(2), stdlib.h for _Exit(2), and of course sys/time.h for gettimeofday(2). The one I've had the most trouble with is actually stat(2) as that uses struct stat {} which is very os-specific, and yet the header (sys/stat.h) is provided by libc.

@keith-packard - what I meant by "owned" is that <time.h> is part of the ISO C specification, whereas <fcntl.h>, <unistd.h>, <sys/time.h>, <sys/stat.h> are not part of the ISO C specification (I may be wrong, but I don't believe that's the case).

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:

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 _POSIX_* macros are checked before declaring various structures, macros, symbols, etc.

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 #include_next hack (glibc does this internally btw, not externally with different bodies of software), it might be rational to do it in a very limited few cases.

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.

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.

We already have __ZEPHYR__. The POSIX spec requires various flags to be defined before any header is even included. So technically, it might be acceptible for that to look like -include <file.h> or it could be of the form -D_MACRO=foo ...

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

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:

  • make pthread_t a uint32_t
    • compatible with glibc / newlib / picolibc declarations
    • trivially portable
    • continue using CONFIG_MAX_POSIX_THREADS
  • make pthread_mutex_t a uint32_t
    • compabible with newlib / picolibc declarations
    • trivially portable
    • use a new CONFIG_MAX_POSIX_MUTEXES
  • make pthread_cond_t a uint32_t
    • compabible with newlib / picolibc declarations
    • trivially portable
    • use a new CONFIG_MAX_POSIX_CONDITIONS
  • see what can be done about pthread_attr_t
    • potentially use as-is and rework internal code accordingly
    • potentially alias the entire structure or even fields within it
    • note: pthread_attr_t does not require explicit management because it is quite short-lived

@stephanosio
Copy link
Member

FYI, some additional discussion happened on this matter on Discord:
https://discord.com/channels/720317445772017664/883450954878439445/1036504018412908574

Some excerpts:

k: (I guess I don't quite understand the desire to avoid providing a POSIX interface for Zephyr; it's super convenient when porting existing code, but it's not my OS)
s: the reason is that Zephyr's native API is not POSIX, and having POSIX support enabled results in footprint increase. we want Zephyr to be as small as possible, and we're talking about kilobytes here.

k: ok, so I think we have some agreement here -- Zephyr will provide a POSIX interface for portability. It should be optional. The Zephyr toolchain should not depend on the POSIX interface for any ISO C APIs.
s: yes, that is the gist of it. basically, we want to decouple libc from the POSIX and make the Zephyr POSIX library/subsystem self-contained, such that all POSIX primitives and implementations are provided by (and only by) the Zephyr POSIX library/subsystem.

s: the problem is that newlib (and picolibc) includes POSIX support and makes use of the POSIXisms internally. the other C library support here really means proprietary toochain libc support (e.g. Arm Compiler, IAR, ...), and they tend to not include POSIX support in their libc.
k: how do they support stdio?
s: a bunch of (non-POSIX) hooks

k: so there are two jobs here -- the first is to remove dependency on POSIX APIs from picolibc as used by Zephyr. The second is to make picolibc not express a POSIX API to applications.
k: That first part is actually pretty easy; we just need to replace the POSIX-dependent functions, such as fopen/fdopen
k: have those implemented in libc-hooks.c instead and base them on Zephyr interfaces
k: Picolibc provides an abstract 'buffered I/O' layer which the POSIX code is built atop, that can be re-used for this work making the actual implementation quite simple.
s: IIRC, as far as newlib is concerned, it uses POSIX-looking but not really POSIX interface functions internally anyway. e.g. _open_r (the _ prefix)
s: we can just keep treating them like libc hooks. after all, these are not in the same scope as the POSIX functions themselves.
k: yup
s: I suppose picolibc is doing something similar (if not the same)?
k: no, picolibc doesn't actually depend on any named functions.
k: fopen and fdopen are layers on top of stdio that are only available if you have an underlying POSIX API. so Zephyr can re-implement those in libc-hooks.c using Zephyr interfaces and the rest of the library won't care. a total of about maybe 50 lines of code
s: ok, so just overriding the fopen and fdopen in the Zephyr-side libc-hooks.c should do then.
k: yup

k: what we do need to do is more serious surgery to picolibc to prevent it from installing POSIX headers
k: that will catch applications using POSIX headers when Zephyr doesn't provide them
k: again, not strictly necessary as any Zephyr apps should find the Zephyr headers instead of the Picolibc ones, but it's a good error prevention measure
s: yup. we do not want picolibc (or any other libc for that matter) to provide any POSIX primitives.
k: oh, it doesn't provide them, it just declares them in the defined header files. So we need to stop installing those.
s: are these POSIX decls clearly separated from the rest?
k: I don't know -- I hope the POSIX declarations are all in separate files, but I haven't done any serious review
k: if not, there will be lots of untangling to be done
s: hmm, so that needs some work. I will look into it later as well. for now, the task at hand is to make the picolibc the default libc for Zephyr. making Zephyr POSIX subsys work with picolibc can be considered a future enhancement, especially since it will likely need some time.

@cfriedt
Copy link
Member Author

cfriedt commented Oct 31, 2022

FYI, some additional discussion happened on this matter on Discord: https://discord.com/channels/720317445772017664/883450954878439445/1036504018412908574

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.

cfriedt added a commit to zephyrproject-rtos/gsoc-2022-thrift that referenced this pull request Jan 15, 2023
Temporarily build this PR against zephyrproject-rtos/zephyr#51771
until that pull has been accepted to main.

Signed-off-by: Chris Friedt <[email protected]>
cfriedt added a commit to zephyrproject-rtos/gsoc-2022-thrift that referenced this pull request Jan 15, 2023
Temporarily build this PR against zephyrproject-rtos/zephyr#51771
until that pull has been accepted to main.

Signed-off-by: Chris Friedt <[email protected]>
cfriedt added a commit to zephyrproject-rtos/gsoc-2022-thrift that referenced this pull request Jan 15, 2023
Temporarily build this PR against zephyrproject-rtos/zephyr#51771
until that pull has been accepted to main.

Signed-off-by: Chris Friedt <[email protected]>
cfriedt added a commit to zephyrproject-rtos/gsoc-2022-thrift that referenced this pull request Jan 15, 2023
Temporarily build this PR against zephyrproject-rtos/zephyr#51771
until that pull has been accepted to main.

Signed-off-by: Chris Friedt <[email protected]>
cfriedt added a commit to zephyrproject-rtos/gsoc-2022-thrift that referenced this pull request Jan 15, 2023
Temporarily build this PR against zephyrproject-rtos/zephyr#51771
until that pull has been accepted to main.

Signed-off-by: Chris Friedt <[email protected]>
cfriedt added a commit to zephyrproject-rtos/gsoc-2022-thrift that referenced this pull request Jan 15, 2023
Temporarily build this PR against zephyrproject-rtos/zephyr#51771
until that pull has been accepted to main.

Signed-off-by: Chris Friedt <[email protected]>
cfriedt added a commit to zephyrproject-rtos/gsoc-2022-thrift that referenced this pull request Jan 16, 2023
Temporarily build this PR against zephyrproject-rtos/zephyr#51771
until that pull has been accepted to main.

Signed-off-by: Chris Friedt <[email protected]>
rlubos
rlubos previously approved these changes Jan 16, 2023
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]>
@cfriedt
Copy link
Member Author

cfriedt commented Jan 16, 2023

@rlubos - I had to add a no-op to process SOL_SOCKET / SO_LINGER so that the Thrift tests would pass. I hope that's ok. At some point, it would be good to add proper support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking area: POSIX POSIX API Library area: Sockets Networking sockets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants