From cbaa579706d60e8b44e5b90f8db1fad177129006 Mon Sep 17 00:00:00 2001 From: Maxime Arthaud Date: Fri, 15 Dec 2023 02:39:37 -0800 Subject: [PATCH] Remove SPARTA_ASSERT in favor of SPARTA_RUNTIME_CHECK Summary: We currently have two different types of runtime checks: * `SPARTA_ASSERT`, which is just an alias for the C `assert()` * `SPARTA_RUNTIME_CHECK` which throws an exception This diff proposes to remove `SPARTA_ASSERT` in favor of `SPARTA_RUNTIME_CHECK`, for the following reasons: * `SPARTA_RUNTIME_CHECK` throws an exception, which can be caught and properly written out, rather than crashing the whole program. This can be important in multithreaded environments * `assert()` might not be recommended in certain environments. For instance, `redex` defines its own `assert` macros. Reviewed By: arnaudvenet Differential Revision: D52162691 fbshipit-source-id: e420125f4f689f5fe1c6d761cb4a034f83fdbb71 --- sparta/include/sparta/Exceptions.h | 6 ------ sparta/include/sparta/IntervalDomain.h | 30 ++++++++++++++++++++------ sparta/include/sparta/ThreadPool.h | 2 +- sparta/include/sparta/WorkQueue.h | 28 +++++++++++++----------- 4 files changed, 40 insertions(+), 26 deletions(-) diff --git a/sparta/include/sparta/Exceptions.h b/sparta/include/sparta/Exceptions.h index cab3b85bb4..546538881f 100644 --- a/sparta/include/sparta/Exceptions.h +++ b/sparta/include/sparta/Exceptions.h @@ -7,7 +7,6 @@ #pragma once -#include #include #include @@ -70,11 +69,6 @@ using operation_name = } \ while (0) -/* - * An assert-like macro that terminates the program. - */ -#define SPARTA_ASSERT(m) assert(m) - /* * Indicates that the given variable is unused, to prevent compiler warnings. */ diff --git a/sparta/include/sparta/IntervalDomain.h b/sparta/include/sparta/IntervalDomain.h index 52bab15d4f..c9b637358d 100644 --- a/sparta/include/sparta/IntervalDomain.h +++ b/sparta/include/sparta/IntervalDomain.h @@ -55,21 +55,33 @@ class IntervalDomain final : public AbstractDomain> { /* [lb, ub] */ static IntervalDomain finite(Num lb, Num ub) { - SPARTA_ASSERT(MIN < lb && "interval not bounded below."); - SPARTA_ASSERT(lb <= ub && "interval inverted."); - SPARTA_ASSERT(ub < MAX && "interval not bounded above."); + SPARTA_RUNTIME_CHECK(MIN < lb, + invalid_argument() + << argument_name("lb") + << error_msg("interval not bounded below.")); + SPARTA_RUNTIME_CHECK(lb <= ub, + invalid_argument() << argument_name("lb") + << error_msg("interval inverted.")); + SPARTA_RUNTIME_CHECK(ub < MAX, + invalid_argument() + << argument_name("ub") + << error_msg("interval not bounded above.")); return {lb, ub}; } /* [lb, +inf] */ static IntervalDomain bounded_below(Num lb) { - SPARTA_ASSERT(MIN < lb && "interval underflow"); + SPARTA_RUNTIME_CHECK(MIN < lb, + invalid_argument() << argument_name("lb") + << error_msg("interval underflow")); return {lb, MAX}; } /* [-inf, ub] */ static IntervalDomain bounded_above(Num ub) { - SPARTA_ASSERT(ub < MAX && "interval overflow."); + SPARTA_RUNTIME_CHECK(ub < MAX, + invalid_argument() << argument_name("ub") + << error_msg("interval overflow.")); return {MIN, ub}; } @@ -87,7 +99,9 @@ class IntervalDomain final : public AbstractDomain> { /* Inclusive lower-bound of the interval, assuming interval is not bottom. */ Num lower_bound() const { - SPARTA_ASSERT(!is_bottom()); + SPARTA_RUNTIME_CHECK( + !is_bottom(), + invalid_argument() << error_msg("cannot call lower_bound() on bottom")); return m_lb; } @@ -96,7 +110,9 @@ class IntervalDomain final : public AbstractDomain> { * Guaranteed to be greater than or equal to lower_bound(). */ Num upper_bound() const { - SPARTA_ASSERT(!is_bottom()); + SPARTA_RUNTIME_CHECK( + !is_bottom(), + invalid_argument() << error_msg("cannot call upper_bound() on bottom")); return m_ub; } diff --git a/sparta/include/sparta/ThreadPool.h b/sparta/include/sparta/ThreadPool.h index d6d7cdf109..a376ce12f5 100644 --- a/sparta/include/sparta/ThreadPool.h +++ b/sparta/include/sparta/ThreadPool.h @@ -80,7 +80,7 @@ class ThreadPool : public AsyncRunner { void run_async_bound(std::function bound_f) override { { std::lock_guard lock(m_mutex); - SPARTA_ASSERT(!m_joining); + SPARTA_RUNTIME_CHECK(!m_joining, internal_error()); if (m_waiting == 0) { m_threads.push_back(create_thread(std::move(bound_f))); return; diff --git a/sparta/include/sparta/WorkQueue.h b/sparta/include/sparta/WorkQueue.h index 542ced6825..b429a3ee2f 100644 --- a/sparta/include/sparta/WorkQueue.h +++ b/sparta/include/sparta/WorkQueue.h @@ -132,7 +132,7 @@ class WorkerState final { * `WorkQueue::add_item()` as the latter is not thread-safe. */ void push_task(Input task) { - SPARTA_ASSERT(m_can_push_task); + SPARTA_RUNTIME_CHECK(m_can_push_task, internal_error()); auto* node = new Node{std::move(task), m_additional_tasks.load()}; do { if (node->prev == nullptr) { @@ -152,7 +152,7 @@ class WorkerState final { void set_running(bool running) { if (m_running && !running) { auto num = m_state_counters->num_running.fetch_sub(1); - SPARTA_ASSERT(num > 0); + SPARTA_RUNTIME_CHECK(num > 0, internal_error()); SPARTA_UNUSED_VARIABLE(num); } else if (!m_running && running) { m_state_counters->num_running.fetch_add(1); @@ -179,7 +179,7 @@ class WorkerState final { if (i < size) { if (size - 1 == i) { auto num = m_state_counters->num_non_empty_initial.fetch_sub(1); - SPARTA_ASSERT(num > 0); + SPARTA_RUNTIME_CHECK(num > 0, internal_error()); SPARTA_UNUSED_VARIABLE(num); } other->set_running(true); @@ -196,7 +196,7 @@ class WorkerState final { // node holds the element we intend to remove. if (node->prev == nullptr) { auto num = m_state_counters->num_non_empty_additional.fetch_sub(1); - SPARTA_ASSERT(num > 0); + SPARTA_RUNTIME_CHECK(num > 0, internal_error()); SPARTA_UNUSED_VARIABLE(num); } // We can't just delete the node right here, as there may be racing @@ -294,7 +294,8 @@ WorkQueue::WorkQueue(Executor executor, m_state_counters(num_threads), m_can_push_task(push_tasks_while_running), m_async_runner(async_runner) { - SPARTA_ASSERT(num_threads >= 1); + SPARTA_RUNTIME_CHECK(num_threads >= 1, invalid_argument() + << argument_name("num_threads")); for (unsigned int i = 0; i < m_num_threads; ++i) { m_states.emplace_back(std::make_unique>( i, &m_state_counters, m_can_push_task)); @@ -304,13 +305,14 @@ WorkQueue::WorkQueue(Executor executor, template void WorkQueue::add_item(Input task) { m_insert_idx = (m_insert_idx + 1) % m_num_threads; - SPARTA_ASSERT(m_insert_idx < m_states.size()); + SPARTA_RUNTIME_CHECK(m_insert_idx < m_states.size(), internal_error()); m_states[m_insert_idx]->m_initial_tasks.push_back(std::move(task)); } template void WorkQueue::add_item(Input task, size_t worker_id) { - SPARTA_ASSERT(worker_id < m_states.size()); + SPARTA_RUNTIME_CHECK(worker_id < m_states.size(), + invalid_argument() << argument_name("worker_id")); m_states[worker_id]->m_initial_tasks.push_back(std::move(task)); } @@ -400,7 +402,7 @@ void WorkQueue::run_all() { run_in_parallel(worker); for (size_t i = 0; i < m_num_threads; ++i) { - SPARTA_ASSERT(!m_states[i]->m_running); + SPARTA_RUNTIME_CHECK(!m_states[i]->m_running, internal_error()); } if (exception) { @@ -408,11 +410,13 @@ void WorkQueue::run_all() { } for (size_t i = 0; i < m_num_threads; ++i) { - SPARTA_ASSERT( + SPARTA_RUNTIME_CHECK( m_states[i]->m_next_initial_task.load(std::memory_order_relaxed) == - m_states[i]->m_initial_tasks.size()); - SPARTA_ASSERT(m_states[i]->m_additional_tasks.load( - std::memory_order_relaxed) == nullptr); + m_states[i]->m_initial_tasks.size(), + internal_error()); + SPARTA_RUNTIME_CHECK(m_states[i]->m_additional_tasks.load( + std::memory_order_relaxed) == nullptr, + internal_error()); } }