Page MenuHomePhabricator

Do not #error if no OS is #defined
Needs RevisionPublic

Authored by davezarzycki on Sep 26 2019, 12:58 AM.

Details

Summary

Currently all standard C++ headers #error with "No thread API" if the OS is "none" in the target triple. This is unfortunate and unnecessary. In particular, this breaks two clang tests if clang is configured to use libcxx as the default C++ standard library.

Diff Detail

Event Timeline

davezarzycki created this revision.Sep 26 2019, 12:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2019, 12:58 AM

Part of me is tempted to say that this standard library should be built with -DLIBCXX_ENABLE_THREADS=OFF -- any reason why this isn't the case?

Hi @ldionne – That's a good question and I don't know the answer. If it's okay with you, I'd like to keep this change request narrowly focused on getting libcxx to work at all with freestanding environments.

Hi @ldionne – That's a good question and I don't know the answer. If it's okay with you, I'd like to keep this change request narrowly focused on getting libcxx to work at all with freestanding environments.

My point is that it already does, when configured properly. If you specify -DLIBCXX_ENABLE_THREADS=OFF, you don't get the error you're seeing.

I think the larger point I'm trying to make is that we have a dual personality problem in libc++, where we try one the one hand to accommodate all systems from a single configuration of libc++, but we also have configuration options that would make libc++ work on that system without any code change. I'd like us (the libc++ maintainers) to figure this out.

I think trying to work out of the box with zero tuning in common scenarios and minimal tuning with uncommon scenarios is the right approach. If it were up to me, I’d flip the test: If windows then set the Windows define, otherwise pthreads.h exists, assume pthreads. If somebody is compiling “freestanding” like myself, then a link error about missing pthreads is expected behavior.

Hi @ldionne – That's a good question and I don't know the answer. If it's okay with you, I'd like to keep this change request narrowly focused on getting libcxx to work at all with freestanding environments.

"Narrowly focused" changes is how we end up with a twisty little maze of #ifdefs and code.
I would much prefer if at the end of the day we had a better code base. (that includes the features that you want).

This is unfortunate and unnecessary. In particular, this breaks two clang tests if clang is configured to use libcxx as the default C++ standard library.

Which two clang tests? (and why do they supply "none" as the target triple)

The two tests:

clang/test/Headers/arm-fp16-header.c
clang/test/Headers/arm-neon-header.c

As to why they specify "none" as the OS, I can only speculate. For example: ARM CPUs are popular with embedded/freestanding targets. In any case, it should work and it does work with the GNU C++ standard library.

If you want a simpler __config file then perhaps what follows is the way to go. The only downside to this change is that old compilers on POSIX platforms might need to pass -D_LIBCPP_HAS_THREAD_API_PTHREAD because __has_include doesn't work.

--- __config.orig	2019-09-27 06:51:51.598527020 +0300
+++ __config	2019-09-27 07:00:10.082234165 +0300
@@ -1069,21 +1069,12 @@
     !defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && \
     !defined(_LIBCPP_HAS_THREAD_API_WIN32) && \
     !defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
-#  if defined(__FreeBSD__) || \
-      defined(__Fuchsia__) || \
-      defined(__wasi__) || \
-      defined(__NetBSD__) || \
-      defined(__linux__) || \
-      defined(__GNU__) || \
-      defined(__APPLE__) || \
-      defined(__CloudABI__) || \
-      defined(__sun__) || \
-      (defined(__MINGW32__) && __has_include(<pthread.h>))
-#    define _LIBCPP_HAS_THREAD_API_PTHREAD
-#  elif defined(_LIBCPP_WIN32API)
+#  if defined(_LIBCPP_WIN32API)
 #    define _LIBCPP_HAS_THREAD_API_WIN32
+#  elif __has_include(<pthread.h>)
+#    define _LIBCPP_HAS_THREAD_API_PTHREAD
 #  else
-#    error "No thread API"
+#    define _LIBCPP_HAS_NO_THREADS
 #  endif // _LIBCPP_HAS_THREAD_API
 #endif // _LIBCPP_HAS_NO_THREADS

Hi @mclow.lists – Ping. Any thoughts on the simplification you requested?

I kinda like the suggestion. I'm pretty confident we don't support compilers that don't implement __has_include anyway (maybe we technically do, but I'm sure nothing good happens in that case).

I kinda like the suggestion. I'm pretty confident we don't support compilers that don't implement __has_include anyway (maybe we technically do, but I'm sure nothing good happens in that case).

However, even your suggestion doesn't answer the question of what's the purpose of setting -DLIBCXX_ENABLE_THREADS=OFF if we detect it automatically in the headers anyway. If we go down your route, I'd drop -DLIBCXX_ENABLE_THREADS completely from CMake (but that suggests a broader direction from the project where the headers should be usable without any configuration option).

I kinda like the suggestion. I'm pretty confident we don't support compilers that don't implement __has_include anyway (maybe we technically do, but I'm sure nothing good happens in that case).

However, even your suggestion doesn't answer the question of what's the purpose of setting -DLIBCXX_ENABLE_THREADS=OFF if we detect it automatically in the headers anyway. If we go down your route, I'd drop -DLIBCXX_ENABLE_THREADS completely from CMake (but that suggests a broader direction from the project where the headers should be usable without any configuration option).

An old boss of mine liked to say that "simple things should be simple and hard things should be possible". Working out of the box for most configurations most of the time is the simple case. If somebody wants to explicitly disable threading, they should be able to do that, but we shouldn't force most people to enable threading if it can be automatically detected.

threading support is required to implement a conforming standard library. if we don't have it we're non-conforming.

additionally this option changes the symbols we export from the dylib. a library built without threads is not compatible with usages that have threads and vice versa

when we build the library either we configure with threading disabled, or threads are enabled and it should be a hard error if that configuration is broken.

The test failures you're running into are correctly failing and should not be made to pass. making them password hide a bug. The compeler's test suite is not a normal consumer of the standard library, and we shouldn't bend to it as such.

EricWF requested changes to this revision.Oct 4 2019, 10:05 PM
This revision now requires changes to proceed.Oct 4 2019, 10:05 PM