st_atim is required by POSIX 2008 and has very good support across POSIX platforms (glibc 2.12, FreeBSD 9, OpenBSD 4.5, NetBSD (since 2013-04-07), illumos (likely from the beginning), ...). We can simplify the code by fallbacking to st_atimespec only when APPLE is defined.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 25335 Build 25334: arc lint + arc unit
Event Timeline
Comment Actions
Well, as long as there is need for a special code path under __APPLE__ I prefer to stick to autodetection.
Comment Actions
Do our minimum system requirements for building LLVM include requiring POSIX 2008 conformance (perhaps indirectly, through the minimum toolchain requirements)?
Comment Actions
There are almost no OSes having conformance with UNIX/POSIX. UNIXv6 (or similar) is Solaris only.
Comment Actions
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.
Comment Actions
There are 2 pieces of autodetect code:
- preprocessor level detection #if defined(HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC)
- cmake detection CHECK_STRUCT_HAS_MEMBER
After this patch, they get simplified and become:
- 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.
Comment Actions
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).
Comment Actions
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.
Comment Actions
This is an example of a mentioned OS in description when feature checks are plain wrong by design (even if this particular example might work for all illumos distributions). Another example, Minix keeps importing a lot of code from NetBSD, I don't intend to hardcode minix here or there and keep iterating over 3rd party projects for every new feature in the OS.
Comment Actions
Sorry if I didn't specify the NetBSD date when the st_atim support is added in the first revision, but I think i've added that before you made your first comment. Do you think a minix that is modern enough and the rest of the community should care about doesn't support st_atim? If it doesn't, I can extend the #ifdef __APPLE__ check to include it. I have a feeling that some of our discussion may have diverged from the original purpose of this revision. It tries to simplify the various cmake feature detection and preprocessor detection. If the minix community cares if someone else may inadvertently break their builds, they should contribute a buildbot, otherwise, it is up to luck that nobody would break theirs. We should also not emphasize more on hypothetical platforms that don't have good POSIX.1-2008 support without a clear proof. At the least, for the platforms that are not widely used, it is not rare that people inadvertently break them but usually they can be fixed soon.
Comment Actions
I recognize that, but I don't think it's all that much more simple (we still have to test for __APPLE__) and worry that it is also more fragile because it removes the dynamic cmake detection.
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.
Given that we already use feature detection rather heavily, I don't think deviating here buys us all that much in practice.
Comment Actions
I'm expressing that swapping feature detection to OS checks is inferior and makes this code worse, I gave two illustrations when it makes a lot of harm. I don't agree with this patch regardless of what OS in what version when adopted one or the another form (or none of them).
If you have problems replacing cmake with something else, it's not a problem in LLVM.
Comment Actions
I agree with Aaron. The CMake feature is working correctly for all supported use cases, and buys us much more portability than the replacement. I'm yet to hear any actual advantage from not using CMake check.
Comment Actions
Given that we already use feature detection rather heavily, I don't think deviating here buys us all that much in practice.
We do have lots of feature detection macros in use but each brings some maintenance cost and we should be cautious to introduce new ones. We definitely should take each case by case. For this particular case, we know that st_atim has good support for systems other than macOS.
If I have to write defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__), I'll start considering whether a HAVE_* macro can do a better job. But here, a simple check like #ifdef __APPLE__ suffices.
The CMake feature is working correctly for all supported use cases, and buys us much more portability than the replacement. I'm yet to hear any actual advantage from not using CMake check.
cmake variables and HAVE_* macros are easy to introduce but hard to remove. For this st_atim case, __APPLE__ is lacking behind but we can envision it will catch up later, and we can eventually remove the check. The check is also intentionally dichotomous instead of trichotomous as atime_nsec = mtime_nsec = 0; shouldn't happen. Readers will not stop here and wonder what happens if neither macros is defined.
Portability is considered with the universal set in mind. It does not buy us anything to extend our support unlimited to some unknown hypothetical platforms. If such platform does exist and this patch breaks it, I hope its users will stop for 1 second and think why their POSIX platform still hasn't be prepared for a standard released 10 years ago. On the other hand, the code is evolving. Though for portability it might not happen, if __APPLE__ removed st_atimespec or the hypothetical platform's users want to fix the code, I believe the case is simple enough for contributors to fix.
Comment Actions
- Wrong. Specific checks are easier to find and remove because they can be easily found. Looking for __APPLE__ is going to yield much more results than anticipated.
- Expecting people to fix old releases because your fragile logic stopped working and they need the old version for some project is not nice.
Portability is about writing code that's portable right now, i.e. can work on many platforms, including platforms you didn't specifically anticipate. It's not only about specifically accounting for the platforms you care about, and deferring fixing the rest on somebody else. Or claiming that somebody's platform doesn't have to be supported because he doesn't have resources to contribute a buildbot.
Comment Actions
Sure, but very little maintenance cost for us overall.
We definitely should take each case by case.
I think it's great to ask "is this the right way to do this" for new code. However, once we pick a way to do something, I prefer to use it consistently unless there's a strong reason to deviate.
For this particular case, we know that st_atim has good support for systems other than macOS.
If I have to write defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__), I'll start considering whether a HAVE_* macro can do a better job. But here, a simple check like #ifdef __APPLE__ suffices.
Except that the code being guarded has nothing to do with __APPLE__ or the behavior on that platform and everything to do with whether struct stat has the expected fields. The current conditional inclusion expression more closely relates to what's being guarded, which is one of the reasons why it's an improvement over the proposed change.
The CMake feature is working correctly for all supported use cases, and buys us much more portability than the replacement. I'm yet to hear any actual advantage from not using CMake check.
cmake variables and HAVE_* macros are easy to introduce but hard to remove.
We have made passes over this file in the past and removed macros that were no longer required. The last time we did this was back in April.
For this st_atim case, __APPLE__ is lacking behind but we can envision it will catch up later, and we can eventually remove the check.
The same is true for the feature testing macros. For instance, once we updated our minimum system requirements such that stdint.h was provided everywhere, we removed the corresponding feature test macro.
The check is also intentionally dichotomous instead of trichotomous as atime_nsec = mtime_nsec = 0; shouldn't happen. Readers will not stop here and wonder what happens if neither macros is defined.
Portability is considered with the universal set in mind. It does not buy us anything to extend our support unlimited to some unknown hypothetical platforms. If such platform does exist and this patch breaks it, I hope its users will stop for 1 second and think why their POSIX platform still hasn't be prepared for a standard released 10 years ago. On the other hand, the code is evolving. Though for portability it might not happen, if __APPLE__ removed st_atimespec or the hypothetical platform's users want to fix the code, I believe the case is simple enough for contributors to fix.
"It does not buy us anything" -- yes it does; it buys us exactly what we were sold: does the target architecture support this feature we're relying on? "There's good support for X everywhere *except for*" is exactly why I prefer the feature testing macro -- it means we don't have to guess where the feature is supported and it means that we can trivially identify WHY something is conditionally included rather than wondering why the platform is suddenly important. In this same file, check out getMainExecutable() for an example of what this kind of "look at the platform, not the functionality" turns into. I believe:
#elif defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) || \
defined(__minix) || defined(__DragonFly__) || \
defined(__FreeBSD_kernel__) || defined(_AIX)should be spelled #elif defined(HAVE_GETPROGPATH); saving the platform identification checks for times when the behavior of the same API differs between platforms.
Comment Actions
I don't really have an opinion on this patch, but I'd like to mention a few things.
- The feature test macros generally do have two downsides:
- They can have false positives. We had one bug where the feature detection for mmap() was accidentally true on Windows which lead to pretty bad behavior.
- They slow down cmake. Every single check probably not a ton, but running cmake on my system takes minutes, much of this time doing feature checks that return the same every time.
- maskray somewhere says that this helps the gn build, but a) the gn build explicitly shouldn't be an argument for making changes in the cmake build, see llvm/utils/gn/README.rst, so please don't do that b) this was very easy to merge to the gn build (see r347530).
I personally think doing something like
if (APPLE) (set HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC 1) (set HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC 0) else() (set HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC 1) (set HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC 0) endif()
if that's what the check computes on every system makes a lot of sense, but it's in the end a philosophical decision on if you want your systems to be fast, predictable, and inflexible, or slow, unpredictable, and flexible.
Comment Actions
This is a sign that feature detection was done wrongly, not that it's inferior to hardcoding.
There are narrow cases when it's difficult to detect some features with feature detection rules, but it's not this case.
- They slow down cmake. Every single check probably not a ton, but running cmake on my system takes minutes, much of this time doing feature checks that return the same every time.
It's a marginal cost of compatibility.
Comment Actions
Machine time is cheap, human time isn't. Saving time on CMake checks in no way justifies wasting somebody else's time when the hardcoded values mismatch at least once.
Comment Actions
In order to push this forward I recommend to:
- report to Apple developers to fix this compliance nit with POSIX 2008,
- enhance gn/bazel/any to handle detection of members in structures (it's elementary feature for a build system that intends to be portable).
Comment Actions
(As said,the gn build shouldn't inform what LLVM does, but this is an explicit non-goal for gn, which chooses fast, predictable, and inflexible. utils/gn/README.rst's "Philosophy" section has some words on this.)