Page MenuHomePhabricator

libc++: include unistd.h if available to get some preprocessor macros
Needs RevisionPublic

Authored by LittleFox94 on May 13 2020, 10:34 AM.

Details

Reviewers
EricWF
Group Reviewers
Restricted Project
Summary

Including unistd.h allows the systems default headers to define some more preprocessor macros we might need for configuration.

This fixes a check for newlib, see https://bugs.llvm.org/show_bug.cgi?id=45862

Diff Detail

Event Timeline

LittleFox94 created this revision.May 13 2020, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2020, 10:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Since this include was deleted as "dead code" before, I added a comment why it is not dead

EricWF requested changes to this revision.May 13 2020, 10:57 AM
EricWF added a subscriber: EricWF.
EricWF added inline comments.
libcxx/include/__config
13

Please do this later in the file,

Specifically after we #define __has_include in the case where we don't have it.

This revision now requires changes to proceed.May 13 2020, 10:57 AM

I am slightly concerned that this also introduces a bunch of declarations in __config, which is the most included file of the library. That would also be the first time we include unistd.h in the libc++ headers (as opposed to the sources used to build the dylib). That might not be an issue, but it's something to keep in mind.

Is this necessary for libc++ to work with Newlib? I thought we already supported it as-is (we certainly have some code paths that are conditionals to it).

I am slightly concerned that this also introduces a bunch of declarations in __config, which is the most included file of the library. That would also be the first time we include unistd.h in the libc++ headers (as opposed to the sources used to build the dylib). That might not be an issue, but it's something to keep in mind.

Another way would be to include _newlib_version.h if it exists, would that be better? Would then be a newlib specific patch.

Is this necessary for libc++ to work with Newlib? I thought we already supported it as-is (we certainly have some code paths that are conditionals to it).

__config has a check for newlib which never works, due to the preprocessor macro not being set without an include (line 933 in the patched revision)

We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.

We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.

sounds good to me. If this is also good for @ldionne I'll prepare a new patch :)

We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.

sounds good to me. If this is also good for @ldionne I'll prepare a new patch :)

Yeah, I'd like to see that patch.

Is this review still relevant? If not, let's abandon it to clear up the review queue.

Is this review still relevant? If not, let's abandon it to clear up the review queue.

Ouch oops.. sorry, was really scatterbrained this year .. I'll look into it in the next 2 days, feel free to close afterwards if you don't read from me

Again, sorry