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 subsys: transition to #include_next for header consistency #18892

Closed
pabigot opened this issue Sep 4, 2019 · 7 comments
Closed

POSIX subsys: transition to #include_next for header consistency #18892

pabigot opened this issue Sep 4, 2019 · 7 comments
Assignees
Labels
area: POSIX POSIX API Library Enhancement Changes/Updates/Additions to existing features priority: low Low impact/importance bug

Comments

@pabigot
Copy link
Collaborator

pabigot commented Sep 4, 2019

The POSIX subsystem injects a large number of standard headers into the include path to provide declarations required for POSIX when they may be missing in minimal libc. It currently uses detailed knowledge of underlying newlib and minimal libc private headers to re-use as much as possible.

A similar problem has been solved for C++ by having Zephyr's minimal libc stdint.h use #include_next <stdint.h> to obtain material from the toolchain version while still enhancing the header contents for Zephyr-specific needs.

This should be considered for the POSIX subsystem to reduce maintenance complexity.

Relates to #13767 #17706

@pabigot pabigot added the Enhancement Changes/Updates/Additions to existing features label Sep 4, 2019
@aescolar
Copy link
Member

aescolar commented Sep 4, 2019

CC @pfalcon

@aescolar aescolar added the area: POSIX POSIX API Library label Sep 4, 2019
@pfalcon
Copy link
Contributor

pfalcon commented Sep 4, 2019

@pabigot, I'm not sure what problem is reported here. Did you try to port some existing POSIX software and faced issues with that? If so, please report in it in much more detail in a separate ticket.

Otherwise, that's the criteria for whether POSIX subsys goes in the right direction or not. And it's not currently in the state where we can discuss whether it's better with adornments of include_next or just include. It's just going from the state of "totally broken" to "somewhat less broken".

I can't even assess your proposal here, because well, I'm busy with putting POSIX to real work (build existing real-world software with it, again). There won't be premature optimizations to it before it's proven to be really working. There won't be even premature cleanups, for those may be as harmful as premature optimizations, at least by them taking valuable time and attention from real tasks. The RFC for development/maintenance of POSIX subsystem is up: #17706, and I'm busy following it. Since the time it was posted, I didn't have any other revolutionary ideas how to approach it, sorry.

injects a large number of standard headers into the include path to provide declarations required for POSIX when they may be missing in minimal libc.

That's very peephole view of what POSIX subsystem does. Again, it strives to build existing POSIX software. All other tasks are subjugate to that top-level goal, and real-world constraints. For example, "minimal libc" and POSIX has very little in common, POSIX will never work reasonably with minimal libc. But I was told by the project maintainers that I should do something about minimal libc. So I crawl thru this monkey job (as usual, in the reasonably best possible way). And if you've seen a way to improve doing this monkey job, I'm really impressed, but kindly ask to treat it with the corresponding priority.

@pabigot
Copy link
Collaborator Author

pabigot commented Sep 4, 2019

The primary problem is that the POSIX subsystem is forcing changes on minimal libc that are inappropriate because the POSIX subsystem approach is to replace the toolchain headers, rather than augment them. See #18780 (comment).

If #include_next <time.h> was used in the POSIX subsystem <time.h> then (a) minimal libc would not need to be told about things like interval time data structures, and (b) if it did need them it wouldn't have to hide them in an obscure BSD-compatible header in order to be consistent with how newlib provides them.

@nashif has also expressed concern about this sort of modification of minimal libc solely to support the POSIX subsystem. Now that the issue has been recorded I think this is a path that would allow us to be a bit more consistent about criteria for adding things to minimal libc, even if they're just header-only, and I would take that into account when reviewing proposed changes.

@nashif
Copy link
Member

nashif commented Sep 5, 2019

while include_next was used for posix subsys already, I think we should avoid it. It is a gcc-specific language extension that has its share of issues.

random reference: https://rules.sonarsource.com/c/tag/lock-in/RSPEC-3730

@pabigot
Copy link
Collaborator Author

pabigot commented Sep 6, 2019

@nashif It's true that #include_next is a GNU extension. It's also true it was designed specifically to satisfy this problem: augmenting the content of a standard-defined header without impacting user or framework code, nor application control of include-path priority. It's supported by clang, and in theory could be supported by any other toolchain. In fact, google provides evidence it's supported by IAR, Keil, and Intel compilers.

I understand the market value of following coding style guides that customers feel provide a signal of quality, though the extensibility of those guides from applications targeting a single toolchain to frameworks that are toolchain-agnostic is unclear.

Our current situation is that Zephyr requires a solution to include file augmentation to work with a C++ compiler (GNU or other), and #include_next has been used as that solution. No toolchain currently supported by Zephyr has been identified that does not handle #include_next. If one arises, then the build infrastructure could (and would have to) be augmented to determine the absolute path to the next instance of a specific header so this can be injected into the source, probably by defining a macro.

So we have a problem, we have a clean solution that works today, we have prior art in-project demonstrating that use, and we have a concept plan to move away from the technology if we have to.

I don't think your objection should prevent progress on this issue using #include_next unless/until (a) there is clear project guidance that conformance to external coding standards takes priority over functionality, and (b) a replacement solution has been identified and implemented.

@pabigot
Copy link
Collaborator Author

pabigot commented Apr 28, 2020

A demonstration of a working technique to incorporate a toolchain header into a POSIX include file is in this commit archived at https://github.com/pabigot/zephyr/commits/nordic/pr/24746.

@cfriedt
Copy link
Member

cfriedt commented Nov 17, 2022

The suggested approach (after much discussion with c, cpp maintainers and collaborators) was to use the Newlib (PicoLibc) POSIX headers internally, as they are quite mature and already provide much of the necessary POSIX feature checking.

With that, using #include_next is unnecessary.

For details, please see the POSIX Roadmap for LTSv3 (#51211).

@cfriedt cfriedt closed this as not planned Won't fix, can't repro, duplicate, stale Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: POSIX POSIX API Library Enhancement Changes/Updates/Additions to existing features priority: low Low impact/importance bug
Projects
None yet
Development

No branches or pull requests

5 participants