Skip to content

Commit

Permalink
Explicitly qualify atomic_wait and atomic_notify function calls
Browse files Browse the repository at this point in the history
Summary:
`std::atomic_wait()` and `std::atomic_notify_one()` are marked as unavailable.
Not really sure what that means, but it seems to be breaking some open source
builds #1527.  Explicitly qualify the
calls into those functions to try and fix the build break.

Since we cannot conditionally import either of the above (because they are
marked as unavailable), we can't rely on the standard implementations.  To
prevent ADL from kicking in when the standard-library defines these, we fully
qualify these and use `tag_invoke` for the customization points used in tests.

Reviewed By: davidtgoldblatt

Differential Revision: D26742072

fbshipit-source-id: 9f44bbfd37530f5ecffa3e03d673cfb1df953299
  • Loading branch information
aary authored and facebook-github-bot committed Mar 8, 2021
1 parent 89a3d4a commit 8477e25
Show file tree
Hide file tree
Showing 6 changed files with 91 additions and 36 deletions.
4 changes: 2 additions & 2 deletions folly/lang/CustomizationPoint.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
namespace folly {

This comment has been minimized.

Copy link
@ldionne

ldionne May 3, 2021

Contributor

@aary FWIW libc++ marks the synchronization library as unavailable on Apple platforms because we couldn't guarantee that is was going to be ABI stable yet. See https://reviews.llvm.org/D96790 for tracking updates to this.


// Using 'auto' for non-type template parameters is only possible from C++17
#if __cplusplus >= 201703L
#if FOLLY_CPLUSPLUS >= 201703L

// cpo_t<CPO>
//
Expand All @@ -71,6 +71,6 @@ namespace folly {
template <const auto& Tag>
using cpo_t = std::decay_t<decltype(Tag)>;

#endif // __cplusplus >= 201703L
#endif // FOLLY_CPLUSPLUS >= 201703L

} // namespace folly
27 changes: 12 additions & 15 deletions folly/synchronization/AtomicNotification-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@

#pragma once

#include <condition_variable>
#include <cstdint>

#include <folly/detail/Futex.h>
#include <folly/synchronization/ParkingLot.h>

Expand Down Expand Up @@ -120,31 +117,31 @@ void atomic_notify_all_impl(const Atom<Integer, Args...>* atomic) {
return UnparkControl::RemoveContinue;
});
}
} // namespace atomic_notification
} // namespace detail

template <typename Integer>
void atomic_wait(const std::atomic<Integer>* atomic, Integer expected) {
detail::atomic_notification::atomic_wait_impl(atomic, expected);
void tag_invoke(
atomic_wait_fn, const std::atomic<Integer>* atomic, Integer expected) {
atomic_wait_impl(atomic, expected);
}

template <typename Integer, typename Clock, typename Duration>
std::cv_status atomic_wait_until(
std::cv_status tag_invoke(
atomic_wait_until_fn,
const std::atomic<Integer>* atomic,
Integer expected,
const std::chrono::time_point<Clock, Duration>& deadline) {
return detail::atomic_notification::atomic_wait_until_impl(
atomic, expected, deadline);
return atomic_wait_until_impl(atomic, expected, deadline);
}

template <typename Integer>
void atomic_notify_one(const std::atomic<Integer>* atomic) {
detail::atomic_notification::atomic_notify_one_impl(atomic);
void tag_invoke(atomic_notify_one_fn, const std::atomic<Integer>* atomic) {
atomic_notify_one_impl(atomic);
}

template <typename Integer>
void atomic_notify_all(const std::atomic<Integer>* atomic) {
detail::atomic_notification::atomic_notify_all_impl(atomic);
void tag_invoke(atomic_notify_all_fn, const std::atomic<Integer>* atomic) {
atomic_notify_all_impl(atomic);
}

} // namespace atomic_notification
} // namespace detail
} // namespace folly
69 changes: 58 additions & 11 deletions folly/synchronization/AtomicNotification.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,67 @@ namespace folly {
* is identical to futexWaitUntil() and returns std::cv_status
*/
// mimic: std::atomic_wait, p1135r0
template <typename Integer>
void atomic_wait(const std::atomic<Integer>* atomic, Integer old);
template <typename Integer, typename Clock, typename Duration>
std::cv_status atomic_wait_until(
const std::atomic<Integer>* atomic,
Integer old,
const std::chrono::time_point<Clock, Duration>& deadline);
namespace detail {
namespace atomic_notification {
struct atomic_wait_fn {
public:
template <typename Atomic, typename Integer>
constexpr void operator()(const Atomic* atomic, Integer integer) const {
tag_invoke(*this, atomic, integer);
}
};
} // namespace atomic_notification
} // namespace detail
constexpr inline auto atomic_wait =
detail::atomic_notification::atomic_wait_fn{};

// mimic: std::atomic_wait_until, p1135r0
namespace detail {
namespace atomic_notification {
struct atomic_wait_until_fn {
public:
template <typename Atomic, typename Integer, typename Clock, typename Dur>
constexpr std::cv_status operator()(
const Atomic* atomic,
Integer old,
const std::chrono::time_point<Clock, Dur>& deadline) const {
return tag_invoke(*this, atomic, old, deadline);
}
};
} // namespace atomic_notification
} // namespace detail
constexpr inline auto atomic_wait_until =
detail::atomic_notification::atomic_wait_until_fn{};

// mimic: std::atomic_notify_one, p1135r0
template <typename Integer>
void atomic_notify_one(const std::atomic<Integer>* atomic);
namespace detail {
namespace atomic_notification {
struct atomic_notify_one_fn {
public:
template <typename Atomic>
constexpr void operator()(const Atomic* atomic) const {
tag_invoke(*this, atomic);
}
};
} // namespace atomic_notification
} // namespace detail
constexpr inline auto atomic_notify_one =
detail::atomic_notification::atomic_notify_one_fn{};

// mimic: std::atomic_notify_all, p1135r0
template <typename Integer>
void atomic_notify_all(const std::atomic<Integer>* atomic);
namespace detail {
namespace atomic_notification {
struct atomic_notify_all_fn {
public:
template <typename Atomic>
constexpr void operator()(Atomic* atomic) const {
tag_invoke(*this, atomic);
}
};
} // namespace atomic_notification
} // namespace detail
constexpr inline auto atomic_notify_all =
detail::atomic_notification::atomic_notify_all_fn{};

// mimic: std::atomic_uint_fast_wait_t, p1135r0
using atomic_uint_fast_wait_t = std::atomic<std::uint32_t>;
Expand Down
7 changes: 5 additions & 2 deletions folly/synchronization/DistributedMutex-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1040,7 +1040,7 @@ inline void recordTimedWaiterAndClearTimedBit(
template <typename Atomic>
void wakeTimedWaiters(Atomic* state, bool timedWaiters) {
if (UNLIKELY(timedWaiters)) {
atomic_notify_one(state);
folly::atomic_notify_one(state); // evade ADL
}
}

Expand Down Expand Up @@ -1665,7 +1665,10 @@ auto timedLock(Atomic& state, Deadline deadline, MakeProxy proxy) {

// wait on the futex until signalled, if we get a timeout, the try_lock
// fails
auto result = atomic_wait_until(&state, previous | data, deadline);
auto result = folly::atomic_wait_until( // evade ADL
&state,
previous | data,
deadline);
if (result == std::cv_status::timeout) {
return proxy(nullptr, std::uintptr_t{0}, false);
}
Expand Down
6 changes: 4 additions & 2 deletions folly/synchronization/test/DistributedMutexTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <folly/Synchronized.h>
#include <folly/container/Array.h>
#include <folly/container/Foreach.h>
#include <folly/lang/CustomizationPoint.h>
#include <folly/portability/GTest.h>
#include <folly/synchronization/Baton.h>
#include <folly/test/DeterministicSchedule.h>
Expand Down Expand Up @@ -174,15 +175,16 @@ folly::detail::FutexResult futexWaitImpl(
}

template <typename Clock, typename Duration>
std::cv_status atomic_wait_until(
std::cv_status tag_invoke(
cpo_t<atomic_wait_until>,
const ManualAtomic<std::uintptr_t>*,
std::uintptr_t,
const std::chrono::time_point<Clock, Duration>&) {
ManualSchedule::beforeSharedAccess();
return std::cv_status::no_timeout;
}

void atomic_notify_one(const ManualAtomic<std::uintptr_t>*) {
void tag_invoke(cpo_t<atomic_notify_one>, const ManualAtomic<std::uintptr_t>*) {
ManualSchedule::beforeSharedAccess();
}
} // namespace test
Expand Down
14 changes: 10 additions & 4 deletions folly/test/DeterministicSchedule.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
#include <folly/ScopeGuard.h>
#include <folly/concurrency/CacheLocality.h>
#include <folly/detail/Futex.h>
#include <folly/lang/CustomizationPoint.h>
#include <folly/synchronization/AtomicNotification.h>
#include <folly/synchronization/detail/AtomicUtils.h>
#include <folly/synchronization/test/Semaphore.h>

Expand Down Expand Up @@ -684,18 +686,22 @@ detail::FutexResult deterministicFutexWaitImpl(
* waits and wakes
*/
template <typename Integer>
void atomic_wait(const DeterministicAtomic<Integer>*, Integer) {}
void tag_invoke(
cpo_t<atomic_wait>, const DeterministicAtomic<Integer>*, Integer) {}
template <typename Integer, typename Clock, typename Duration>
std::cv_status atomic_wait_until(
std::cv_status tag_invoke(
cpo_t<atomic_wait_until>,
const DeterministicAtomic<Integer>*,
Integer,
const std::chrono::time_point<Clock, Duration>&) {
return std::cv_status::no_timeout;
}
template <typename Integer>
void atomic_notify_one(const DeterministicAtomic<Integer>*) {}
void tag_invoke(cpo_t<atomic_notify_one>, const DeterministicAtomic<Integer>*) {
}
template <typename Integer>
void atomic_notify_all(const DeterministicAtomic<Integer>*) {}
void tag_invoke(cpo_t<atomic_notify_all>, const DeterministicAtomic<Integer>*) {
}

/**
* DeterministicMutex is a drop-in replacement of std::mutex that
Expand Down

11 comments on commit 8477e25

@AlexHooperDev
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

when will this release be deployed please? Flipper-Folly is currently breaking all react native builds using Xcode 12.5, as shown here

@lParraMediaCurrent
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

when will this release be deployed please? Flipper-Folly is currently breaking all react native builds using Xcode 12.5, as shown here

+1

@shankerdhand
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

when will this release be deployed please? Flipper-Folly is currently breaking all react native builds using Xcode 12.5, as shown here

+1

@yfeldblum
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is released from the folly side. However, flipper and react-native may be pinned to older revisions of folly and have not yet updated their pins and picked up this change.

@hmunoz
Copy link

@hmunoz hmunoz commented on 8477e25 May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very thanks , in my case only add line decribe in folly/synchronization/DistributedMutex-inl.h
and work

@WilbertJanney
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very thanks , in my case only add line decribe in folly/synchronization/DistributedMutex-inl.h
and work

Where is the folly folder you are describing?

@aprilmintacpineda
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hoping for this to be released soon, currently preventing us from upgrading to 0.64.1 from 0.63.4

@binitgurzu
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still looking for solutions

@xtealer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same error right now

@aprilmintacpineda
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upgrade to 0.65, it's fixed there.

@Mendez252
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works to me:

  • Open "DistributedMutex-inl.h" (VSCode) //Use ctrl + p if you dont find it

  • find this block of code:

    template
    void wakeTimedWaiters(Atomic* state, bool timedWaiters) {
    if (UNLIKELY(timedWaiters)) {
    atomic_notify_one(state); <---- replace this line with this----> folly::atomic_notify_one(state); // evade ADL
    }
    }

  • clean build folder on xCode

  • build again

Please sign in to comment.