[PATCH] D55296: [Support] Fix GNU/kFreeBSD build

[James Clarke via Phabricator via llvm-commits]

jrtc27 marked 2 inline comments as done.
jrtc27 added inline comments.


================
Comment at: lib/Support/Unix/Path.inc:59
 #if !defined(__APPLE__) && !defined(__OpenBSD__) && !defined(__FreeBSD__) &&   \
-    !defined(__linux__)
+    !defined(__linux__) && !defined(__FreeBSD_kernel__)
 #include <sys/statvfs.h>
----------------
krytarowski wrote:
> jrtc27 wrote:
> > krytarowski wrote:
> > > jrtc27 wrote:
> > > > krytarowski wrote:
> > > > > jrtc27 wrote:
> > > > > > krytarowski wrote:
> > > > > > > efriedma wrote:
> > > > > > > > Does this make the `!defined(__FreeBSD__)` check redundant?
> > > > > > > Technically yes.
> > > > > > 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.
> > > > > FreeBSD defines it in `sys/param.h`.
> > > > > 
> > > > > https://github.com/freebsd/freebsd/blob/467e55b8abbf80cd144961ee51833b8ff4209b17/sys/sys/param.h#L65
> > > > 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__`.
> > > 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.
> > 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?
> 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.
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55296/new/

https://reviews.llvm.org/D55296