Page MenuHomePhabricator

[Support] Fix GNU/kFreeBSD build
ClosedPublic

Authored by jrtc27 on Dec 4 2018, 1:49 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jrtc27 created this revision.Dec 4 2018, 1:49 PM
efriedma added inline comments.Dec 4 2018, 2:07 PM
lib/Support/Unix/Path.inc
59 ↗(On Diff #176705)

Does this make the !defined(__FreeBSD__) check redundant?

krytarowski added inline comments.Dec 4 2018, 2:11 PM
lib/Support/Unix/Path.inc
59 ↗(On Diff #176705)

Technically yes.

jrtc27 marked 2 inline comments as done.Dec 4 2018, 2:11 PM
jrtc27 added inline comments.
lib/Support/Unix/Path.inc
59 ↗(On Diff #176705)

Unfortunately not. Plain FreeBSD just defines __FreeBSD__, and because that indicates both kernel and userland, __FreeBSD_kernel__ was introduced just for GNU/kFreeBSD. In theory it should also be defined for pure FreeBSD, but it isn't. Maybe someone will propose that and we can drop this in 10 years' time, but today is not that day.

krytarowski added inline comments.Dec 4 2018, 2:15 PM
lib/Support/Unix/Path.inc
59 ↗(On Diff #176705)
jrtc27 marked 3 inline comments as done.Dec 4 2018, 2:16 PM
jrtc27 added inline comments.
lib/Support/Unix/Path.inc
59 ↗(On Diff #176705)

Huh, TIL. I wonder why this isn't put in the compiler(s) though... Anyway, like it says, we should still really be checking for __FreeBSD__.

krytarowski added inline comments.Dec 4 2018, 2:20 PM
lib/Support/Unix/Path.inc
59 ↗(On Diff #176705)

GNU/kFreeBSD should be detected imho with defined(__FreeBSD_kernel__) && defined(__GLIBC__).. but since GNU/kFreeBSD is an ongoing experiment I recommend to keep using standalone check for __FreeBSD__ and another for __FreeBSD_kernel__ with assumption that this is GNU/kFreeBSD.

efriedma accepted this revision.Dec 4 2018, 2:20 PM

Okay, then LGTM

This revision is now accepted and ready to land.Dec 4 2018, 2:20 PM
jrtc27 marked 3 inline comments as done.Dec 4 2018, 2:25 PM
jrtc27 added inline comments.
lib/Support/Unix/Path.inc
59 ↗(On Diff #176705)

Well, this looks like a kernel-based check, not a userland check, as it's pulling in stuff from sys/? Everything here is checking kernel macros (__CYGWIN__ is an odd-ball that's sort of both), including __GNU__ which by your suggestion should be __GNU__ && __GLIBC__; ditto for __linux__. So whilst I would agree with your suggestion on a technical level, I don't think it actually applies here?

krytarowski added inline comments.Dec 4 2018, 2:34 PM
lib/Support/Unix/Path.inc
59 ↗(On Diff #176705)

kFreeBSD could be in theory used with a different userland than GNU or BSD, but it's a question whether it would solve any new problems.

__GNU__ && __GLIBC__ would be more purist and there is __gnu_hurd__ for this exact purpose, similar to __gnu_linux__.

Actually checking for __FreeBSD_kernel__ && __GLIBC__ is even recommended by https://sourceforge.net/p/predef/wiki/OperatingSystems/

It's a similar debate whether Debian is Linux or GNU/Linux.

For practical purposes the proposed patch is the right thing to do.

jrtc27 marked 2 inline comments as done.Dec 4 2018, 2:51 PM
jrtc27 added inline comments.
lib/Support/Unix/Path.inc
59 ↗(On Diff #176705)

Yes, but that's only a valid thing to be checking if you care about the userland; my point is that this is very much a kernel check (or at least, that's what it's checking on every other platform here; e.g. __linux__ is there on its own without a __GLIBC__ check) and so we explicitly *don't* want to be checking whether it's GNU/kFreeBSD, musl/kFreeBSD or something stupid like Android/kFreeBSD.

jrtc27 added a comment.Dec 4 2018, 2:52 PM

Okay, then LGTM

I don't have commit rights, so could somebody please commit on my behalf?

krytarowski added inline comments.Dec 4 2018, 2:55 PM
lib/Support/Unix/Path.inc
59 ↗(On Diff #176705)

This is not userland check but OS check. GNU/kFreeBSD composes an OS (kernel + userland). NetBSD/FreeBSD/Linux/Apple copose a different OS.

Mixing userlands and kernels does not buy much in real world so kFreeBSD is the sole experiment out there (ignoring now some alternative libc userlands for small embedded Linux world).

Android check is a check for Android kernel and Android userland.

Okay, then LGTM

I don't have commit rights, so could somebody please commit on my behalf?

Ping?

Sorry, lost track of it; I'll merge it today.

This revision was automatically updated to reflect the committed changes.