Page MenuHomePhabricator

[libcxx] Fix hang in try_acquire with __atomic_semaphore_base
Needs ReviewPublic

Authored by pabusse on Mar 10 2021, 5:12 AM.

Details

Reviewers
__simt__
mclow.lists
ldionne
Group Reviewers
Restricted Project
Summary

On platforms with _LIBCPP_NO_NATIVE_SEMAPHORES, try_acquire blocks instead of returning false.

This is because libcpp_thread_poll_with_backoff interprets zero as meaning infinite, causing try_acquire to wait indefinitely when atomic_semaphore_base is used.

Diff Detail

Unit TestsFailed

TimeTest
14,410 mslibcxx CI AArch64 > libc++.std/thread/futures/futures_async::async.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/local/bin/c++ /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/libcxx/test/std/thread/futures/futures.async/async.pass.cpp -v --target=aarch64-linux-gnu -include /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/projects/libcxx/__config_site -include /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/libcxx/include -I/home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2a -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/projects/libcxx/test/std/thread/futures/futures.async/Output/async.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -L/home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/./lib -Wl,-rpath,/home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -lc++experimental -o /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx…

Event Timeline

pabusse created this revision.Mar 10 2021, 5:12 AM
pabusse requested review of this revision.Mar 10 2021, 5:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2021, 5:12 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/include/semaphore
114

So __libcpp_thread_poll_with_backoff treats 0=infinite, but __libcpp_semaphore_wait_timed (line 167) treats 0=0? That sounds like a recipe for bugs. Is there any appetite to just make libc++ treat 0=0 uniformly across the board?
(Then, if someone wants to wait for a billion seconds, they just pass 1000000000 as the timeout — I don't see any practical need to overload 0 with that extra meaning.)

pabusse added inline comments.Mar 10 2021, 6:15 AM
libcxx/include/semaphore
114

That's correct.

I didn't modify the semantics of __libcpp_thread_poll_with_backoff straight away as it is also used in some other contexts with zero-intending-infinite. I agree however that this might be the right change to make.

__simt__ added inline comments.Mar 10 2021, 7:40 AM
libcxx/include/semaphore
114

A large timeout would still mean you have to call it in a loop, whereas the current arrangement avoids that.

But we could make the interface more clear to avoid this kind of bug, I agree we should try.