Page MenuHomePhabricator

Implementation of C++20's P1135R6 for libcxx
ClosedPublic

Authored by __simt__ on Oct 4 2019, 1:11 PM.

Details

Summary

This is the first review of this code so there's a lot to look at, and I'm fully expecting a lot of changes will be requested.

This patch contains...

A) Changes to __threading_support that introduce:

  1. Low-level semaphores on POSIX and Apple GCD, and futex on Linux.
  2. Declarations for a sharded table of contention state in the dylib, used by atomic::wait.
  3. Declarations for a thread_local variable in the dylib, used by barriers.

B) The <atomic> changes from P1135:

  1. High QoI: multi-layered back-off, using either/both the state from 1b and futexes from 1a.
  2. Low QoI: exponential time back-off, using chrono only.

C) Barrier:

  1. High QoI: a tree barrier, using the acceleration state in 1c to amortize the extra round.
  2. Low QoI: a central barrier, with a specialization for the empty completion function.

D) Semaphore:

  1. All QoI: a general template semaphore for very large ptrdiff_t values.
  2. High QoI: a specialization for “reasonable” ptrdiff_t values, using semaphores in 1a and acceleration atomics.
  3. Low QoI: a specialization for unit count.

E) Latch:

  1. All QoI (low): a central latch. (If there's a desire to see it, I could borrow the same QoI knobs from barrier, but sizeof() would grow a lot.)

F) The first basic tests for each facility from P1135.

G) Miscellaneous tweaks I needed to get this to build as libcu++ (the CUDA variant). We can drop these or you can take them as improvements. One of them is a legit macro bug.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
zoecarver added inline comments.Jan 9 2020, 11:29 AM
libcxx/src/atomic.cpp
33

What's the point of this macro, ATOMIC_VAR_INIT (I realize you didn't add it, but I'm still curious)?

47

This is different if _LIBCPP_HAS_PLATFORM_WAIT_STATE is false, right?

130

Will this ever be true?

__simt__ marked 24 inline comments as done.Jan 9 2020, 1:55 PM

No problem. I just want to know what I need to do in the next patch in order to move forward.

libcxx/src/atomic.cpp
29

It's false on CUDA. It would be false on platforms that can't rely on OS support for efficient waiting and have to fall back to polling with backoff.

33

Prior to P0883 merging into C++20, atomics are constructed in an uninitialized state. You're supposed to use this macro to give it a static initialization. This macro is deprecated after C++20.

47

If you have a platform wait state, then it's used by this facility.

If you don't have a platform wait state, then a condvar is used instead.

130

Oh yes. Every time that there is a contending waiter concurrent with this notifier. This condition is true whenever the facility's use is not trivial.

134

Answered below.

140

Undefined behavior. Usually a segfault.

149

This is the uglier part of the patch. We have this situation where one platform (Apple) is about to go from not having platform wait states, to having them. This API is trying to be able to take either kind of efficient wait structure in the application OR in the dylib, and provide efficient waiting either way.

I think it would not be unreasonable if you guys asked me to cut this part out in order to get a first commit of the facility, and then work in the background with Louis on recovering the capability in some way, with or without a public patch.

163

Yes.

libcxx/src/barrier.cpp
45

This needs to be inside the loop unfortunately. During the first round, we need to record our effective start (leaf) location in the tree. I could express it differently - record it only at the end of the first round - but it would remain in the loop nest.

What we could potentially do is dumb down this favorite barrier index concept, or remove it entirely. It's worth a relatively small amount of performance by comparison to using the tree barrier in the first place.

I would not be offended if you asked me to give you an ordered list of things I could delete and tell you what it costs you to delete them, approximately. Then we could stop where we are more comfortable.

libcxx/src/semaphore.cpp
27 ↗(On Diff #226990)

Because of how GCD semaphores work, unfortunately.

We could delete this by sending all Apple semaphores to the generic template based on atomics.

Last I spoke with Louis, we thought that would be acceptable.

123 ↗(On Diff #226990)

Yeah. Aren't there enough macros though? I think I might come back later with a different patch to propose a set of macros to configure back-offs.

152 ↗(On Diff #226990)

It's not. This is masking the lower 32-bits of a 64-bit value, and then comparing that with 0.

zoecarver added inline comments.Jan 10 2020, 2:54 PM
libcxx/src/atomic.cpp
33

Heh. I didn't realize that was part of the standard (I was wondering why it wasn't mangled). Good to know.

47

I see. I got confused for a second while trying to follow the #ifs.

140

Isn't that a problem if _LIBCPP_HAS_NO_PLATFORM_WAIT_TABLE isn't defined?

149

I'll defer to others on this but, (assuming it wouldn't be much more work for you) it might be better to remove that from this patch and add it as a follow-up patch.

In general, the less code in this patch, the faster it can get committed.

libcxx/src/barrier.cpp
45

What we could potentially do is dumb down this favorite barrier index concept, or remove it entirely. It's worth a relatively small amount of performance by comparison to using the tree barrier in the first place.

I don't know enough about this piece of code or potential methods of implementation to comment on what we should do. I'll defer to you/others on this.

libcxx/src/semaphore.cpp
123 ↗(On Diff #226990)

Yes, there are an unfortunate number of macros in this bit of code.

I don't feel strongly about adding a macro now or later, maybe add a comment, though (that the number isn't magic or referenced elsewhere).

152 ↗(On Diff #226990)

I'm pretty sure they are the same. Look at this example. The first and last functions generate the same optimized assembly.

__simt__ updated this revision to Diff 240389.Jan 25 2020, 10:34 AM
__simt__ marked 11 inline comments as done.

A lot of changes in this patch, mostly simplifications based on the areas that got feedback about complexity.

For the most part, I have deleted code:

  1. I deleted the condvar-based implementation of atomic_wait because no current platform would make use of it. That also allowed me to delete much of the complicated ABI tricks added in the last revision. That removed several configuration macros.
  2. After discussing it with Louis, I switched the Apple config to use atomics-based semaphores. Not only did that delete a lot of code on its own, but it made some other optimizations in semaphores no longer needed, so I deleted them too. That removed several more configuration macros as well.
  3. I deleted code dealing with the thread_local optimization in barrier. Using this_thread::get::id to inject a thread-stable search seed for the algorithm is about just as good, and everything for that already exists.
  4. Latches, barriers and semaphores make more direct use of the wait/notify functionality now. Where they had "exceptions" with customized waiting, now there's a unified interface there, which deletes code.

In addition to this, the internal abstractions are much better now. I particularly like how natively-supported Futex types pass-through the intermediate layer now, it just works.

I hope you find this version is close enough that we might soon switch to delta patches on top, in trunk.

Cheers,

Olivier

jfb added a subscriber: MadCoder.Jan 27 2020, 10:42 AM

Not finished looking through this but, here are some comments.

Overall, I really like this change. It looks fantastic. It seems like a much cleaner/simpler implementation. Thanks for all the time you've spent on this.

libcxx/include/__threading_support
29

Are all the platforms we support guaranteed to have this header (they very well might, I just don't know)?

31

Is this macro still needed?

33

This could be an else block, no?

74

Are there any platforms where SEM_VALUE_MAX doesn't exist? Maybe some BSD platform? Could you check that it is defined (or maybe that we're on a certain platform)?

libcxx/include/atomic
2445

I might just be missing it but, is this in the standard? Otherwise, could you mangle it?

2446

nit: space between the equals.

libcxx/include/barrier
275

Will __barrier_base be defined if _LIBCPP_HAS_NO_TREE_BARRIER isn't?

libcxx/include/latch
54

Can we make it so that these headers just don't exist before C++11 via cmake? That might be a nicer way to fail.

__simt__ marked 10 inline comments as done.Jan 27 2020, 1:02 PM

Thanks Zoe!

Some responses ->

libcxx/include/__threading_support
29

This header is part of Pthreads and this #include is in the Pthreads section of the header, so I think the answer is yes. The Apple case is special in that they have deprecated this header.

31

Yes. We need a way to disable the platform native semaphores. Both Apple (in the current design) and CUDA need it.

33

It would have to be an #elif !defined(...) so that CUDA could trigger it too.

74

Like the header, it's mandated by Pthreads. It's conceivable that some BSD is non-conforming and we still want to work on it, but I don't have a BSD system handy to try.

libcxx/include/atomic
2445

It is, in C++20.

2446

OK.

libcxx/include/barrier
275

Yes. The macro ends up selecting between two different base classes -- one is the scalable tree barrier, the other is the simpler central barrier.

libcxx/include/latch
54

I don't have this skill. :^/

But I do think that we want to support them in C++11/14/17. This author's production team is going to present it to users in these dialects, for instance.

This is looking pretty good, only a few nitpicks. Once the nitpicks are addressed we can commit this and then go incrementally.

libcxx/include/__threading_support
34

Any reason to introduce this macro instead of just use !defined(_LIBCPP_NO_NATIVE_SEMAPHORES) when you need it?

510

static constexpr?

libcxx/include/atomic
550

Just wondering: will this in any way make it harder to support <atomic> in freestanding?

1465

I'm not seeing this used anywhere -- am I missing something?

libcxx/include/semaphore
62

Olivier and I spoke offline, and it's reasonable to request C++14 at least here. Please do this in all the headers you're adding.

The idea is to avoid having headers that are supported in older dialects than they need to be (within reason), which is often a source of technical debt.

libcxx/src/atomic.cpp
48

In apple_availability.h, add:

#if defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__)
#if __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 101500
#define _LIBCPP_USE_ULOCK
#endif
#elif defined(__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__)
#if __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__ >= 130000
#define _LIBCPP_USE_ULOCK
#endif
#elif defined(__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__)
#if __ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__ >= 130000
#define _LIBCPP_USE_ULOCK
#endif
#elif defined(__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__)
#if __ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__ >= 60000
#define _LIBCPP_USE_ULOCK
#endif
#endif // __ENVIRONMENT_.*_VERSION_MIN_REQUIRED__

That should do it for all platforms aligned to Mac OS 10.15. Feel free to use whatever name for _LIBCPP_USE_ULOCK -- _LIBCPP_USE_APPLE_ULOCK probably makes the most sense since it's an Apple-specific API.

Then, your #elif becomes #elif defined(__APPLE__) && defined(_LIBCPP_USE_APPLE_ULOCK).

__simt__ marked 11 inline comments as done.Feb 14 2020, 8:35 AM

Replies. Also noting that I will include an update to the implementation status HTML page.

libcxx/include/__threading_support
34

I was going to say it’s more convenient for me, but I’m not even convinced.

I can streamline this.

510

Yes.

libcxx/include/atomic
550

A bit, yeah. There are a few ways to proceed.

1465

I intend to use that to implement the normative encouragement that atomic_signed/unsigned[...]_t should be the efficient ones for waiting. I added those types below, but they don’t follow that encouragement.

libcxx/include/semaphore
62

Yep

libcxx/src/atomic.cpp
48

Thanks!

__simt__ updated this revision to Diff 245055.Feb 17 2020, 4:21 PM

This revision addresses the outstanding comments, and incorporates Louis' recommended macros for ulock detection.

I also added missing max() members to barrier/latch that were added by NB comment resolution, and I implemented the normative encouragement for the lock-free types to match the ideal contention type, if there is one. I added tests for these things.

Finally, I further improved the ABI resilience of the barrier type so it's almost a full pimpl now.

ldionne accepted this revision.Feb 18 2020, 7:11 AM

Thanks a lot for all the work and patience, Olivier. I think we're good to go now. Do you have commit access?

Also, can you please confirm that you meant to introduce all the following new symbols in the dylib, and nothing else:

std::__1::__libcpp_atomic_wait(std::__1::__cxx_atomic_impl<long long, std::__1::__cxx_atomic_base_impl<long long> > const volatile*, long long)
std::__1::__libcpp_atomic_wait(void const volatile*, long long)
std::__1::__cxx_atomic_notify_all(std::__1::__cxx_atomic_impl<long long, std::__1::__cxx_atomic_base_impl<long long> > const volatile*)
std::__1::__cxx_atomic_notify_all(void const volatile*)
std::__1::__cxx_atomic_notify_one(std::__1::__cxx_atomic_impl<long long, std::__1::__cxx_atomic_base_impl<long long> > const volatile*)
std::__1::__cxx_atomic_notify_one(void const volatile*)
std::__1::__libcpp_atomic_monitor(std::__1::__cxx_atomic_impl<long long, std::__1::__cxx_atomic_base_impl<long long> > const volatile*)
std::__1::__libcpp_atomic_monitor(void const volatile*)
std::__1::__arrive_barrier_algorithm_base(std::__1::__barrier_algorithm_base*, unsigned char)
std::__1::__destroy_barrier_algorithm_base(std::__1::__barrier_algorithm_base*)
std::__1::__construct_barrier_algorithm_base(long&)

Those will have to be added to the ABI list file (I can do that).

This revision is now accepted and ready to land.Feb 18 2020, 7:11 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2020, 8:03 AM
ldionne added inline comments.Feb 24 2020, 8:50 AM
libcxx/include/atomic
443

We forgot to update the synopsis for this header with the C++20 Synchronization Library. @__simt__ would you be willing to do that in a followup change?

teemperor added inline comments.
libcxx/include/module.modulemap
234

Maybe I'm missing something here, but due to this submodule we are always parsing the barrier header even when building the module with a language standard < C++14. This means that everyone using C++11 is no longer able to use the 'std' Clang module after this commit. Is this intentional?

ldionne added inline comments.Feb 24 2020, 10:50 AM
libcxx/include/module.modulemap
234

No, this is not intentional. Sorry, there were several failures that needed fixing after committing this (failures that were impossible to notice without throwing the change at all the build bots) -- we're getting there.

davide added a subscriber: davide.Feb 24 2020, 10:58 AM

This is the second time something like this happened. When you make such large scale changes -- make sure to run the lldb testsuite, or be ready to revert. We already had this discussion in the past, but clearly it didn't prevent the problem from happening again.

teemperor added inline comments.Feb 24 2020, 11:22 AM
libcxx/include/module.modulemap
234

No worries, pushed a fix in b61e83eb0e31c1e6006569b43bb98a61ff44ca4c

This is the second time something like this happened. When you make such large scale changes -- make sure to run the lldb testsuite, or be ready to revert. We already had this discussion in the past, but clearly it didn't prevent the problem from happening again.

Last time's discussion didn't get anywhere, as running the LLDB test suite on each commit we make to libc++ isn't a viable option.

Libc++'s test matrix is insanely large, and actually we can't even see all of it cause some uses are behind closed doors. Whenever we make a non-trivial change (and BTW this is a purely additive change), it breaks someone somewhere. And you can rest assured that we do run a lot of testing locally to make sure we don't break people before committing, but it can't catch everything. That's just the way it is, and we try to fix it as quickly as possible -- I've spent all day so far trying to fix the consequences of applying this patch. This is not a lack of diligence, it's just that the nature of libc++ makes it difficult to test comprehensively.

I don't know what this commit broke in LLDB, if anything, but instead it's more useful to comment here with a link to the failure so we can help fix it. Being combative is no help, as we're all in the same boat.

This is the second time something like this happened. When you make such large scale changes -- make sure to run the lldb testsuite, or be ready to revert. We already had this discussion in the past, but clearly it didn't prevent the problem from happening again.

Last time's discussion didn't get anywhere, as running the LLDB test suite on each commit we make to libc++ isn't a viable option.

Libc++'s test matrix is insanely large, and actually we can't even see all of it cause some uses are behind closed doors. Whenever we make a non-trivial change (and BTW this is a purely additive change), it breaks someone somewhere. And you can rest assured that we do run a lot of testing locally to make sure we don't break people before committing, but it can't catch everything. That's just the way it is, and we try to fix it as quickly as possible -- I've spent all day so far trying to fix the consequences of applying this patch. This is not a lack of diligence, it's just that the nature of libc++ makes it difficult to test comprehensively.

I don't know what this commit broke in LLDB, if anything, but instead it's more useful to comment here with a link to the failure so we can help fix it. Being combative is no help, as we're all in the same boat.

I would like to reiterate that the policy in LLVM is that commit that break projects can be reverted willy-nilly.
I would also like to stress that my use cases are not behind closed doors -- LLDB is part of the LLVM umbrella.
I thought we reached an agreement last time, but looks like there was a miscommunication or misunderstanding on your side, so let me reiterate that you have two options:

  1. Run the lldb testsuite for changes that impact layout of structures -- or in any case you consider non-trivial, or if you don't think you can do this, at least ping somebody from lldb to take a look at the changes before they're committed. This of course requires some judgement on your side. When in doubt ask, as sending an e-mail is cheap.
  2. You commit without pre-commit checking lldb because you consider the additive cost of running ninja check-lldb is prohibitive. This is your choice, but don't be surprised if people will revert your commit if it breaks things.

Hopefully this clarifies my position.

FWIW, this change broke building for windows. It seemed fairly straightforward to fix though, see D75102.

Hi, a bisect shows that this patch seems to cause time.h to not be found for us:

[1958/45287] CXX kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o
FAILED: kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o
../../../recipe_cleanup/clangEPCywe/bin/clang++ -MD -MF kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o.d -o kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o -DTOOLCHAIN_VERSION=/b/s/w/ir/k/recipe_cleanup/clangEPCywe/bin -DZX_ASSERT_LEVEL=2 -DWITH_FRAME_POINTERS=1 -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS=1 -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -DKERNEL_BASE=0xffffffff80100000 -DSMP_MAX_CPUS=32 -D_KERNEL -DLK -DENABLE_PANIC_SHELL -DWITH_DEBUG_LINEBUFFER -DZIRCON_TOOLCHAIN -DLK_DEBUGLEVEL=2 -DWITH_KERNEL_PCIE -DKERNEL_RETPOLINE=1 -DWITH_UNIFIED_SCHEDULER=1 -DSCHEDULER_TRACING_LEVEL=0 -DARCH_X86 -DKERNEL_LOAD_OFFSET=0x00100000 -D_LIBCPP_DISABLE_EXTERN_TEMPLATE -I../../zircon/kernel/include -I../../zircon/kernel/lib/libc/include -I../../zircon/kernel/lib/ktl/include -I../../zircon/kernel/lib/io/include -I../../zircon/kernel/lib/heap/include -I../../zircon/system/ulib/lazy_init/include -I../../zircon/system/ulib/lockdep/include -I../../zircon/system/ulib/ffl/include -I../../zircon/kernel/vm/include -I../../zircon/kernel/lib/user_copy/include -I../../zircon/system/ulib/zircon-internal/include -I../../zircon/kernel/lib/ktrace/include -I../../zircon/system/ulib/fbl/include -I../../zircon/kernel/lib/fbl/include -I../../zircon/system/public -I../../zircon/kernel/arch/x86/include -I../../zircon/system/ulib/bitmap/include -I../../zircon/kernel/arch/x86/page_tables/include -I../../zircon/system/ulib/hwreg/include -I../../zircon/kernel/lib/heap/include -I../../zircon/kernel/lib/io/include -I../../zircon/kernel/lib/ktl/include -idirafter ../../zircon/kernel/lib/libc/limits-dummy -fno-common --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out/default.zircon=. -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out=.. -ffile-prefix-map=/b/s/w/ir/k/fuchsia=../.. -no-canonical-prefixes -O2 -g3 -Wall -Wextra -Wno-unused-parameter -Wno-address-of-packed-member -Wnewline-eof -Wno-unknown-warning-option -Wno-c99-designator -Wno-int-in-bool-context -Wno-range-loop-analysis -fno-omit-frame-pointer -ffunction-sections -fdata-sections -Wthread-safety -Wimplicit-fallthrough -fvisibility=hidden -ftrivial-auto-var-init=pattern -Werror -Wno-error=deprecated-declarations -fpie -mretpoline -mretpoline-external-thunk -ffreestanding -include ../../zircon/kernel/include/hidden.h -fno-unwind-tables -mno-red-zone -Wformat=2 -Wvla -mno-red-zone -msoft-float -mno-mmx -mno-sse -mno-sse2 -mno-3dnow -mno-avx -mno-avx2 -mcmodel=kernel -fdata-sections -Wno-gnu-string-literal-operator-template -std=c++17 -Wconversion -Wno-sign-conversion -Wextra-semi -Wno-deprecated-copy -Wno-non-c-typedef-for-linkage -ftemplate-backtrace-limit=0 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -faligned-new=8 -fno-exceptions -c ../../zircon/kernel/lib/libc/snprintf.cc
In file included from ../../zircon/kernel/lib/libc/snprintf.cc:10:
In file included from ../../zircon/kernel/lib/ktl/include/ktl/algorithm.h:10:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/algorithm:643:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/memory:666:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/atomic:550:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/__threading_support:14:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/chrono:827:
../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/ctime:49:10: fatal error: 'time.h' file not found
#include <time.h>

Do you know if this is a known or unintended side effect of this patch? I don't think we changed anything on our side that would've caused this.

Builder: https://ci.chromium.org/p/fuchsia/builders/ci/clang_toolchain.fuchsia-x64-debug-subbuild/b8887404211928925456?

Hi, a bisect shows that this patch seems to cause time.h to not be found for us:
[...]

Do you know if this is a known or unintended side effect of this patch? I don't think we changed anything on our side that would've caused this.

This must be cause we're now including <chrono> in <atomic>. This is not really something we can (or want to) workaround, since that's in the spec. Are you folks using some fancy C library that doesn't provide <time.h>? If so, I would argue that the fact it worked before this patch is just a coincidence.

jfb added a comment.Feb 26 2020, 10:18 AM

Hi, a bisect shows that this patch seems to cause time.h to not be found for us:

[1958/45287] CXX kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o
FAILED: kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o
../../../recipe_cleanup/clangEPCywe/bin/clang++ -MD -MF kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o.d -o kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o -DTOOLCHAIN_VERSION=/b/s/w/ir/k/recipe_cleanup/clangEPCywe/bin -DZX_ASSERT_LEVEL=2 -DWITH_FRAME_POINTERS=1 -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS=1 -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -DKERNEL_BASE=0xffffffff80100000 -DSMP_MAX_CPUS=32 -D_KERNEL -DLK -DENABLE_PANIC_SHELL -DWITH_DEBUG_LINEBUFFER -DZIRCON_TOOLCHAIN -DLK_DEBUGLEVEL=2 -DWITH_KERNEL_PCIE -DKERNEL_RETPOLINE=1 -DWITH_UNIFIED_SCHEDULER=1 -DSCHEDULER_TRACING_LEVEL=0 -DARCH_X86 -DKERNEL_LOAD_OFFSET=0x00100000 -D_LIBCPP_DISABLE_EXTERN_TEMPLATE -I../../zircon/kernel/include -I../../zircon/kernel/lib/libc/include -I../../zircon/kernel/lib/ktl/include -I../../zircon/kernel/lib/io/include -I../../zircon/kernel/lib/heap/include -I../../zircon/system/ulib/lazy_init/include -I../../zircon/system/ulib/lockdep/include -I../../zircon/system/ulib/ffl/include -I../../zircon/kernel/vm/include -I../../zircon/kernel/lib/user_copy/include -I../../zircon/system/ulib/zircon-internal/include -I../../zircon/kernel/lib/ktrace/include -I../../zircon/system/ulib/fbl/include -I../../zircon/kernel/lib/fbl/include -I../../zircon/system/public -I../../zircon/kernel/arch/x86/include -I../../zircon/system/ulib/bitmap/include -I../../zircon/kernel/arch/x86/page_tables/include -I../../zircon/system/ulib/hwreg/include -I../../zircon/kernel/lib/heap/include -I../../zircon/kernel/lib/io/include -I../../zircon/kernel/lib/ktl/include -idirafter ../../zircon/kernel/lib/libc/limits-dummy -fno-common --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out/default.zircon=. -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out=.. -ffile-prefix-map=/b/s/w/ir/k/fuchsia=../.. -no-canonical-prefixes -O2 -g3 -Wall -Wextra -Wno-unused-parameter -Wno-address-of-packed-member -Wnewline-eof -Wno-unknown-warning-option -Wno-c99-designator -Wno-int-in-bool-context -Wno-range-loop-analysis -fno-omit-frame-pointer -ffunction-sections -fdata-sections -Wthread-safety -Wimplicit-fallthrough -fvisibility=hidden -ftrivial-auto-var-init=pattern -Werror -Wno-error=deprecated-declarations -fpie -mretpoline -mretpoline-external-thunk -ffreestanding -include ../../zircon/kernel/include/hidden.h -fno-unwind-tables -mno-red-zone -Wformat=2 -Wvla -mno-red-zone -msoft-float -mno-mmx -mno-sse -mno-sse2 -mno-3dnow -mno-avx -mno-avx2 -mcmodel=kernel -fdata-sections -Wno-gnu-string-literal-operator-template -std=c++17 -Wconversion -Wno-sign-conversion -Wextra-semi -Wno-deprecated-copy -Wno-non-c-typedef-for-linkage -ftemplate-backtrace-limit=0 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -faligned-new=8 -fno-exceptions -c ../../zircon/kernel/lib/libc/snprintf.cc
In file included from ../../zircon/kernel/lib/libc/snprintf.cc:10:
In file included from ../../zircon/kernel/lib/ktl/include/ktl/algorithm.h:10:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/algorithm:643:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/memory:666:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/atomic:550:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/__threading_support:14:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/chrono:827:
../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/ctime:49:10: fatal error: 'time.h' file not found
#include <time.h>

Do you know if this is a known or unintended side effect of this patch? I don't think we changed anything on our side that would've caused this.

Builder: https://ci.chromium.org/p/fuchsia/builders/ci/clang_toolchain.fuchsia-x64-debug-subbuild/b8887404211928925456?

Is zircon freestanding, and not providing chrono (which then includes time.h)? That would explain the issue, we'd need to fix freestanding atomic by not exposing time-related stuff if time.h ins't available.

In D68480#1893987, @jfb wrote:

Hi, a bisect shows that this patch seems to cause time.h to not be found for us:

[...]

Do you know if this is a known or unintended side effect of this patch? I don't think we changed anything on our side that would've caused this.

Builder: https://ci.chromium.org/p/fuchsia/builders/ci/clang_toolchain.fuchsia-x64-debug-subbuild/b8887404211928925456?

Is zircon freestanding, and not providing chrono (which then includes time.h)? That would explain the issue, we'd need to fix freestanding atomic by not exposing time-related stuff if time.h ins't available.

So, actually, I think we need to have a discussion about the support of Freestanding in libc++. We should either implement it properly and have testers for it, or stop pretending that we do. Right now all I see us doing is adding complexity to our code to fix stuff that breaks on user configurations we don't know about, understand or test. As someone who has to maintain the code with that complexity, this concerns me.

And I do care about Freestanding -- I think it's important and we should support it. But I also think that we should not do it halfway like right now. In the current state of things, I literally have no way to ensure that we won't break that configuration again tomorrow. @leonardchan @phosek Is there a way that you could add a libc++ builder that runs freestanding?

jfb added a comment.Feb 26 2020, 10:37 AM
In D68480#1893987, @jfb wrote:

Hi, a bisect shows that this patch seems to cause time.h to not be found for us:

[...]

Do you know if this is a known or unintended side effect of this patch? I don't think we changed anything on our side that would've caused this.

Builder: https://ci.chromium.org/p/fuchsia/builders/ci/clang_toolchain.fuchsia-x64-debug-subbuild/b8887404211928925456?

Is zircon freestanding, and not providing chrono (which then includes time.h)? That would explain the issue, we'd need to fix freestanding atomic by not exposing time-related stuff if time.h ins't available.

So, actually, I think we need to have a discussion about the support of Freestanding in libc++. We should either implement it properly and have testers for it, or stop pretending that we do. Right now all I see us doing is adding complexity to our code to fix stuff that breaks on user configurations we don't know about, understand or test. As someone who has to maintain the code with that complexity, this concerns me.

And I do care about Freestanding -- I think it's important and we should support it. But I also think that we should not do it halfway like right now. In the current state of things, I literally have no way to ensure that we won't break that configuration again tomorrow. @leonardchan @phosek Is there a way that you could add a libc++ builder that runs freestanding?

Agreed, it seems like we want to figure out what individual freestanding subsets can "carve out" of regular C++, and how to do so cleanly. "time" seems like an easy thing to carve out, but it shouldn't just be done here, it should be part of the configuration header, applied consistently through libc++, and extensively tested. Ideally it wouldn't just be a builder: you'd have a directory which tests freestanding configuration options (i.e. "can I include atomic without time support?").

@leonardchan @phosek Is there a way that you could add a libc++ builder that runs freestanding?

We have it as a TODO but can bump it in priority.

Agreed, it seems like we want to figure out what individual freestanding subsets can "carve out" of regular C++, and how to do so cleanly. "time" seems like an easy thing to carve out, but it shouldn't just be done here, it should be part of the configuration header, applied consistently through libc++, and extensively tested. Ideally it wouldn't just be a builder: you'd have a directory which tests freestanding configuration options (i.e. "can I include atomic without time support?").

How simple would it be to carve specifically time.h out? I'm unfamiliar with libcxx internals + configurations. I'm also not insisting that progress on this patch be halted, but I'd like to know if there's anything we could do at least now to provide a workaround for freestanding targets,

jfb added a comment.Feb 26 2020, 2:04 PM

@leonardchan @phosek Is there a way that you could add a libc++ builder that runs freestanding?

We have it as a TODO but can bump it in priority.

Agreed, it seems like we want to figure out what individual freestanding subsets can "carve out" of regular C++, and how to do so cleanly. "time" seems like an easy thing to carve out, but it shouldn't just be done here, it should be part of the configuration header, applied consistently through libc++, and extensively tested. Ideally it wouldn't just be a builder: you'd have a directory which tests freestanding configuration options (i.e. "can I include atomic without time support?").

How simple would it be to carve specifically time.h out? I'm unfamiliar with libcxx internals + configurations. I'm also not insisting that progress on this patch be halted, but I'd like to know if there's anything we could do at least now to provide a workaround for freestanding targets,

As a non-libc++ maintainer (so take what I say with some doubt), I'd imagine things being detected in libcxx/include/__config, say through __has_include(time.h), and then you'd have a macro which says "no time". I'd then figure out the transitive inclusions of time.h in all libc++ headers, figure out want pulls them it, and use the macro to not expose the corresponding functions / types on those platforms.

You'd then need a decent amount of testing to make sure this works well.

Concretely here, there are a few atomic functions which require chrono, so you'd want to conditionally include chrono and conditionally define those functions. I think the patch which does this should also handle time in other libc++ headers, so we're not in a situation of "we kinda support having no time.h, but not consistently".

ldionne added inline comments.Mar 18 2020, 2:14 PM
libcxx/include/atomic
443
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 18 2020, 2:14 PM
__simt__ marked an inline comment as done.Mar 18 2020, 3:09 PM
__simt__ added inline comments.
libcxx/include/atomic
443

Yep. Thanks for the ping.