This is an archive of the discontinued LLVM Phabricator instance.

Refine check for `_LIBCPP_C_HAS_NO_GETS` on FreeBSD
ClosedPublic

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

Details

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
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 ↗(On Diff #225640)

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 ↗(On Diff #225640)

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 ↗(On Diff #225640)

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.

emaste accepted this revision.Oct 18 2019, 12:54 PM
ldionne accepted this revision.Oct 18 2019, 2:05 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2019, 3:58 AM