This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Fix hang in try_acquire with __atomic_semaphore_base
ClosedPublic

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

Details

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

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
111

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
111

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
111

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.

Quuxplusone commandeered this revision.Nov 4 2021, 9:25 PM
Quuxplusone added a reviewer: pabusse.

@ldionne, now that we're using __atomic_semaphore_base unconditionally, I think this patch has become super important. I confirm that the new test really does hang forever on my Macbook, and that this change to <semaphore> fixes the hang.

I think we should land this ASAP in both main and 13.x (assuming you're still planning to cherry-pick D110110). Having D110110 without this patch too, is a pretty bad state to be in.

ldionne accepted this revision.Nov 5 2021, 12:50 PM

@ldionne, now that we're using __atomic_semaphore_base unconditionally, I think this patch has become super important. I confirm that the new test really does hang forever on my Macbook, and that this change to <semaphore> fixes the hang.

I think we should land this ASAP in both main and 13.x (assuming you're still planning to cherry-pick D110110). Having D110110 without this patch too, is a pretty bad state to be in.

Agreed. Thanks for rebasing, Arthur. Let's ship this and cherry-pick to 13.x.

This revision is now accepted and ready to land.Nov 5 2021, 12:50 PM