Skip to content

Commit

Permalink
Remove SPARTA_ASSERT in favor of SPARTA_RUNTIME_CHECK
Browse files Browse the repository at this point in the history
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
  • Loading branch information
arthaud authored and facebook-github-bot committed Dec 15, 2023
1 parent 542b1de commit 345a3c5
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 26 deletions.
6 changes: 0 additions & 6 deletions include/sparta/Exceptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#pragma once

#include <cassert>
#include <exception>

#include <boost/config.hpp>
Expand Down Expand Up @@ -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.
*/
Expand Down
30 changes: 23 additions & 7 deletions include/sparta/IntervalDomain.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,33 @@ class IntervalDomain final : public AbstractDomain<IntervalDomain<Num>> {

/* [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};
}

Expand All @@ -87,7 +99,9 @@ class IntervalDomain final : public AbstractDomain<IntervalDomain<Num>> {

/* 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;
}

Expand All @@ -96,7 +110,9 @@ class IntervalDomain final : public AbstractDomain<IntervalDomain<Num>> {
* 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;
}

Expand Down
2 changes: 1 addition & 1 deletion include/sparta/ThreadPool.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class ThreadPool : public AsyncRunner {
void run_async_bound(std::function<void()> bound_f) override {
{
std::lock_guard<std::mutex> 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;
Expand Down
28 changes: 16 additions & 12 deletions include/sparta/WorkQueue.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -294,7 +294,8 @@ WorkQueue<Input, Executor>::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<WorkerState<Input>>(
i, &m_state_counters, m_can_push_task));
Expand All @@ -304,13 +305,14 @@ WorkQueue<Input, Executor>::WorkQueue(Executor executor,
template <class Input, typename Executor>
void WorkQueue<Input, Executor>::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 <class Input, typename Executor>
void WorkQueue<Input, Executor>::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));
}

Expand Down Expand Up @@ -400,19 +402,21 @@ void WorkQueue<Input, Executor>::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) {
std::rethrow_exception(exception);
}

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());
}
}

Expand Down

0 comments on commit 345a3c5

Please sign in to comment.