[PATCH] D54883: [Support/FileSystem] Use st_atimespec only when __APPLE__ is defined

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D54883#1308333 <https://reviews.llvm.org/D54883#1308333>, @krytarowski wrote:

> In D54883#1308313 <https://reviews.llvm.org/D54883#1308313>, @MaskRay wrote:
>
> > In D54883#1307857 <https://reviews.llvm.org/D54883#1307857>, @aaron.ballman wrote:
> >
> > > In D54883#1307853 <https://reviews.llvm.org/D54883#1307853>, @krytarowski wrote:
> > >
> > > > In D54883#1307787 <https://reviews.llvm.org/D54883#1307787>, @aaron.ballman wrote:
> > > >
> > > > > Do our minimum system requirements for building LLVM include requiring POSIX 2008 conformance (perhaps indirectly, through the minimum toolchain requirements)?
> > > >
> > > >
> > > > There are almost no OSes having conformance with UNIX/POSIX. UNIXv6 (or similar) is Solaris only.
> > >
> > >
> > > If our existing system requirements aren't sufficient to practically guarantee these fields are available, I'd prefer to keep the autodetect code and rely on that instead.
> >
> >
> > There are 2 pieces of autodetect code:
> >
> > 1. preprocessor level detection `#if defined(HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC)`
> > 2. cmake detection `CHECK_STRUCT_HAS_MEMBER`
> >
> >   After this patch, they get simplified and become:
> > 3. preprocessor level detection `#ifdef __APPLE__`
> >
> >   On https://github.com/apple/darwin-xnu/blob/master/bsd/sys/stat.h , I cannot find a better feature macro to test so I have to use `__APPLE__`. When they start to support `st_atim` (which is widely available on other platforms) in a newer version, we can make the `#ifdef __APPLE__` check more specific.
> >
> >   The patch directly tests for the necessary prerequisite and removes two cmake variables. This makes it substantially more build system agnostic (for people who use gn,bazel,... they don't need to re-invent the feature detection mechanism in their build systems) and probably (ironically) more portable.
>
>
> Do you mean that someone reinvented build scripts in bazel? If so, it sounds like a downstream problem not relevant to the LLVM codebase. I consider this new version as inferior as every __OS__ check shall be ideally replaced with a check against feature. For example mentioned Illumos - SmartOS has a lot of APIs from Linux in order to host Linux-KVM and this is different to other Illumos distributions (and different to Solaris).


Have you read the description? illumos `usr/src/uts/common/sys/stat.h` has had `st_atim` for probably 14 years. `typedef struct timespec timestruc_t;`

We are here discussing this specific feature of POSIX.1-2008, not overall POSIX.1-2008.

> For example mentioned Illumos - SmartOS has a lot of APIs from Linux in order to host Linux-KVM and this is different to other Illumos distributions (and different to Solaris).

Of course, but they are irrelevant here as we don't use these SmartOS/Linux specific features.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54883