This is an archive of the discontinued LLVM Phabricator instance.

Differential D69174

Refine check for `_LIBCPP_C_HAS_NO_GETS` on FreeBSD
ClosedPublic

Authored by dim on Oct 18 2019, 8:46 AM.

Details

Reviewers
EricWF
mclow.lists
emaste
ldionne
Commits
rGd5367db95c42: Refine check for `_LIBCPP_C_HAS_NO_GETS` on FreeBSD
rL375340: Refine check for `_LIBCPP_C_HAS_NO_GETS` on FreeBSD
rCXX375340: Refine check for `_LIBCPP_C_HAS_NO_GETS` on FreeBSD
Summary

In D67316 we added _LIBCPP_C_HAS_NO_GETS to signal that the C library
does not provide gets(), and added a test for FreeBSD 13 or higher,
using the compiler-defined __FreeBSD__ macro.

Unfortunately this did not work that well for FreeBSD's own CI process,
since the gcc compilers used for some architectures define __FreeBSD__
to match the build host, not the target.

Instead, we should use the __FreeBSD_version macro from the userland
header <osreldate.h>, which is more fine-grained. See also
https://reviews.freebsd.org/D22034.

Diff Detail

Event Timeline

dim created this revision.Oct 18 2019, 8:46 AM
Herald added subscribers: ldionne, christof, krytarowski. · View Herald TranscriptOct 18 2019, 8:46 AM
Harbormaster completed remote builds in B39789: Diff 225640.Oct 18 2019, 8:47 AM
emaste accepted this revision.Oct 18 2019, 11:17 AM

OK with me

This revision is now accepted and ready to land.Oct 18 2019, 11:17 AM
ldionne added inline comments.Oct 18 2019, 12:15 PM
include/__config
1169

Shouldn't this say defined(__FreeBSD_version) && __FreeBSD_version >= 1300043?

dim marked an inline comment as done.Oct 18 2019, 12:33 PM
dim added inline comments.
include/__config
1169

Good one; functionally it won't matter, since <osreldate.h>'s only function is to #define __FreeBSD_version, and it will always be defined. But I can see that it might be confusing to read.

bsdjhb added a subscriber: bsdjhb.Oct 18 2019, 12:36 PM
bsdjhb added inline comments.
include/__config
1169

Either way is fine I think. In the original patch in FreeBSD I kept the defined(FreeBSD) to minimize the diff and due to @dim's note, but whichever is most readable is probably best.

dim updated this revision to Diff 225680.Oct 18 2019, 12:41 PM

Rewrite the __FreeBSD_version condition to be more straightforward.

Harbormaster completed remote builds in B39804: Diff 225680.Oct 18 2019, 12:49 PM
emaste accepted this revision.Oct 18 2019, 12:54 PM
ldionne accepted this revision.Oct 18 2019, 2:05 PM
Herald added a subscriber: dexonsmith. · View Herald TranscriptOct 18 2019, 2:05 PM
Closed by commit rGd5367db95c42: Refine check for `_LIBCPP_C_HAS_NO_GETS` on FreeBSD (authored by dim). · Explain WhyOct 19 2019, 3:58 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2019, 3:58 AM

Revision Contents

PathSize
include/
4 lines

Diff 225680

include/__config

Show First 20 Lines • Show All 236 Lines • ▼ Show 20 Lines
# define _LIBCPP_LITTLE_ENDIAN # define _LIBCPP_LITTLE_ENDIAN
# elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ # elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
# define _LIBCPP_BIG_ENDIAN # define _LIBCPP_BIG_ENDIAN
# endif // __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ # endif // __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
#endif // __BYTE_ORDER__ #endif // __BYTE_ORDER__
#ifdef __FreeBSD__ #ifdef __FreeBSD__
# include <sys/endian.h> # include <sys/endian.h>
# include <osreldate.h>
# if _BYTE_ORDER == _LITTLE_ENDIAN # if _BYTE_ORDER == _LITTLE_ENDIAN
# define _LIBCPP_LITTLE_ENDIAN # define _LIBCPP_LITTLE_ENDIAN
# else // _BYTE_ORDER == _LITTLE_ENDIAN # else // _BYTE_ORDER == _LITTLE_ENDIAN
# define _LIBCPP_BIG_ENDIAN # define _LIBCPP_BIG_ENDIAN
# endif // _BYTE_ORDER == _LITTLE_ENDIAN # endif // _BYTE_ORDER == _LITTLE_ENDIAN
# ifndef __LONG_LONG_SUPPORTED # ifndef __LONG_LONG_SUPPORTED
# define _LIBCPP_HAS_NO_LONG_LONG # define _LIBCPP_HAS_NO_LONG_LONG
# endif // __LONG_LONG_SUPPORTED # endif // __LONG_LONG_SUPPORTED
▲ Show 20 Lines • Show All 906 Lines • ▼ Show 20 Lines
// have standard input and output channels. // have standard input and output channels.
#ifdef __CloudABI__ #ifdef __CloudABI__
#define _LIBCPP_HAS_NO_STDIN #define _LIBCPP_HAS_NO_STDIN
#define _LIBCPP_HAS_NO_STDOUT #define _LIBCPP_HAS_NO_STDOUT
#endif #endif
// Some systems do not provide gets() in their C library, for security reasons. // Some systems do not provide gets() in their C library, for security reasons.
#ifndef _LIBCPP_C_HAS_NO_GETS #ifndef _LIBCPP_C_HAS_NO_GETS
# if defined(_LIBCPP_MSVCRT) || (defined(__FreeBSD__) && __FreeBSD__ >= 13) # if defined(_LIBCPP_MSVCRT) || \
(defined(__FreeBSD_version) && __FreeBSD_version >= 1300043)
ldionneUnsubmitted
Not Done
ReplyInline Actions

Shouldn't this say defined(__FreeBSD_version) && __FreeBSD_version >= 1300043?

ldionne: Shouldn't this say `defined(__FreeBSD_version) && __FreeBSD_version >= 1300043`?
dimAuthorUnsubmitted
Done
ReplyInline Actions

Good one; functionally it won't matter, since <osreldate.h>'s only function is to #define __FreeBSD_version, and it will always be defined. But I can see that it might be confusing to read.

dim: Good one; functionally it won't matter, since <osreldate.h>'s only function is to `#define`…
bsdjhbUnsubmitted
Not Done
ReplyInline Actions

Either way is fine I think. In the original patch in FreeBSD I kept the defined(FreeBSD) to minimize the diff and due to @dim's note, but whichever is most readable is probably best.

bsdjhb: Either way is fine I think. In the original patch in FreeBSD I kept the defined(__FreeBSD__)…
# define _LIBCPP_C_HAS_NO_GETS # define _LIBCPP_C_HAS_NO_GETS
# endif # endif
#endif #endif
#if defined(__BIONIC__) || defined(__CloudABI__) || \ #if defined(__BIONIC__) || defined(__CloudABI__) || \
defined(__Fuchsia__) || defined(__wasi__) || defined(_LIBCPP_HAS_MUSL_LIBC) defined(__Fuchsia__) || defined(__wasi__) || defined(_LIBCPP_HAS_MUSL_LIBC)
#define _LIBCPP_PROVIDES_DEFAULT_RUNE_TABLE #define _LIBCPP_PROVIDES_DEFAULT_RUNE_TABLE
#endif #endif
▲ Show 20 Lines • Show All 292 Lines • Show Last 20 Lines