Skip to content

Conversation

inikep
Copy link
Contributor

@inikep inikep commented Dec 12, 2016

No description provided.

@inikep
Copy link
Contributor Author

inikep commented Dec 12, 2016

The macros are set according to:
http://nadeausoftware.com/articles/2012/01/c_c_tip_how_use_compiler_predefined_macros_detect_operating_system

@inikep
Copy link
Contributor Author

inikep commented Dec 12, 2016

Another option is to turn on '-r' by default for all Unixes except Linux:
(!defined(__linux__) && (defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__))))

Advantages:

Disadvantages:

@Cyan4973
Copy link
Contributor

White-listing is already a hack, and the real intended test is supposed to be _POSIX_C_SOURCE >= 200112L.

So I suggest to make this test the main one. An unknown OS which properly set this macro and provide relevant library support should pass it, without the need to name it.

Keep the white-list for OS which are known to provide a compatible opendir() but without properly defining _POSIX_C_SOURCE, of which the main example is MAC OS-X.

Maybe later we may have to add some blacklist, if an OS lies by pretending to be _POSIX_C_SOURCE >= 200112L but failing to provide the implied library support. But such case should be very rare, if any.

@inikep
Copy link
Contributor Author

inikep commented Dec 12, 2016

real intended test is supposed to be _POSIX_C_SOURCE >= 200112L

  1. It's not so easy. If you look at FreeBSD manpage of opendir:
    https://www.freebsd.org/cgi/man.cgi?query=opendir&apropos=0&sektion=3&manpath=FreeBSD+4.2-RELEASE&format=html
    you can see the only requirement is "functions appeared in 4.2BSD".

It seems that clang (or less probably gcc) used by @DimitryAndric with FreeBSD doesn't declare _POSIX_C_SOURCE. This is also the case for MacOS and can be true for all Unixes.
The reason seems to be Linux is the only widely used system that has a strict environment by default, AFAIK. Every other system exposes all their interfaces (excepting conflicting ones) by default, from
https://lwn.net/Articles/592882/

  1. We know that on Linux gcc and clang with default options (that is -std=gnu89 and for gcc 5+: -std=gnu11) declare _POSIX_C_SOURCE with the value 200809L (200112L in glibc versions before 2.10; 199506L in glibc versions before 2.5; 199309L in glibc versions before 2.1).

According to https://linux.die.net/man/3/opendir opendir conforms POSIX.1-2001 therefore I assumed _POSIX_C_SOURCE >= 200112L. But today I checked glibc sources (https://github.com/lattera/glibc/blob/master/sysdeps/unix/opendir.c) and it seems that opendir() is available from the beginning of Linux. The issue here is that these functions are not exposed be default but gcc/clang fixes it with it's default -std=gnuXX. So we need only to check if flags were not changed to -std=cXX what can be done by checking #ifndef __STRICT_ANSI__ or even better by checking #ifdef _POSIX_C_SOURCE without any value.

@inikep
Copy link
Contributor Author

inikep commented Dec 12, 2016

A proof for my theory is that nanosleep requires _POSIX_C_SOURCE >= 199309L according to
https://linux.die.net/man/2/nanosleep but opendir https://linux.die.net/man/3/opendir has no requirements. It looks like opendir was added to Linux (glibc) in 1991 but standarized with POSIX.1-2001.

@Cyan4973
Copy link
Contributor

OK, so _POSIX_C_SOURCE is effectively for linux only.
What a misnomer.
Anyway, then let's just white list OS which are known to support opendir() correctly.
We'll open more if needed (i.e. requested)

@inikep
Copy link
Contributor Author

inikep commented Dec 12, 2016

I think that _POSIX_SOURCE and _XOPEN_SOURCE are designed to manually add/turn-on groups of functions but it's not useful for us. Therefore we use gcc/clang to automatically detect supported _POSIX_SOURCE using values provided by glibc what works well for us.

In the last commit I changed:

  • Linux requires only defined(_POSIX_C_SOURCE)
  • all OSes supporting (defined(_POSIX_C_SOURCE) && (_POSIX_C_SOURCE >= 200112L)) will have -r turned on

@Cyan4973 Cyan4973 merged commit 4c6ce5a into facebook:dev Dec 12, 2016
@DimitryAndric
Copy link
Contributor

@inikep, thanks, I have rebased my original pull request on top of your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants