Skip to content

Commit

Permalink
assert: abort() rather than throw
Browse files Browse the repository at this point in the history 
Assertion failure should be big. It should be catastrophic. It should
make an impression!

It should also not throw exceptions. This change does exactly that.

More specifically it calls abort() so we don't throw out stack frames or
otherwise mess up debugging.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
  • Loading branch information
@adamemerson
adamemerson committed Dec 11, 2015
1 parent e5536b2 commit f07e029
Show file tree
Hide file tree
Showing 12 changed files with 282 additions and 305 deletions.
10 changes: 6 additions & 4 deletions src/common/assert.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ namespace ceph {
g_assert_context = cct;
}

void __ceph_assert_fail(const char *assertion, const char *file, int line, const char *func)
void __ceph_assert_fail(const char *assertion, const char *file, int line,
const char *func)
{
ostringstream tss;
tss << ceph_clock_now(g_assert_context);
Expand Down Expand Up @@ -74,10 +75,11 @@ namespace ceph {
g_assert_context->_log->dump_recent();
}

throw FailedAssertion(bt);
abort();
}

void __ceph_assertf_fail(const char *assertion, const char *file, int line, const char *func, const char* msg, ...)
void __ceph_assertf_fail(const char *assertion, const char *file, int line,
const char *func, const char* msg, ...)
{
ostringstream tss;
tss << ceph_clock_now(g_assert_context);
Expand Down Expand Up @@ -143,7 +145,7 @@ namespace ceph {
g_assert_context->_log->dump_recent();
}

throw FailedAssertion(bt);
abort();
}

void __ceph_assert_warn(const char *assertion, const char *file,
Expand Down
6 changes: 0 additions & 6 deletions src/include/assert.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,6 @@ class CephContext;
namespace ceph {

struct BackTrace;

struct FailedAssertion {
BackTrace *backtrace;
FailedAssertion(BackTrace *bt) : backtrace(bt) {}
};

#endif


Expand Down
11 changes: 3 additions & 8 deletions src/librados-config.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,9 @@ int main(int argc, const char **argv)
bool opt_version = false;
bool opt_vernum = false;

try {
global_init(NULL, args, CEPH_ENTITY_TYPE_CLIENT, CODE_ENVIRONMENT_UTILITY,
CINIT_FLAG_NO_DEFAULT_CONFIG_FILE);
common_init_finish(g_ceph_context);
} catch (ceph::FailedAssertion &a) {
cout << "ceph::FailedAssertion thrown, exit." << std::endl;
exit(1);
}
global_init(NULL, args, CEPH_ENTITY_TYPE_CLIENT, CODE_ENVIRONMENT_UTILITY,
CINIT_FLAG_NO_DEFAULT_CONFIG_FILE);
common_init_finish(g_ceph_context);
for (std::vector<const char*>::iterator i = args.begin();
i != args.end(); ) {
if (strcmp(*i, "--") == 0) {
Expand Down
11 changes: 2 additions & 9 deletions src/test/bench_log.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,8 @@ int main(int argc, const char **argv)
for (int i=0; i<threads; i++) {
T *t = ls.front();
ls.pop_front();
try {
t->join();
}
catch (ceph::FailedAssertion &a) {
cout << "Failed assert in join(), exit." << std::endl;
delete t;
return -1;
}
delete t;
t->join();
delete t;
}

utime_t t = ceph_clock_now(NULL);
Expand Down
44 changes: 22 additions & 22 deletions src/test/bufferlist.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -445,8 +445,8 @@ TEST(BufferPtr, constructors) {
EXPECT_EQ(original.get_raw(), ptr.get_raw());
EXPECT_EQ(2, ptr.raw_nref());
EXPECT_EQ(0, ::memcmp(original.c_str(), ptr.c_str(), len));
EXPECT_THROW(bufferptr(original, 0, original.length() + 1), FailedAssertion);
EXPECT_THROW(bufferptr(bufferptr(), 0, 0), FailedAssertion);
EXPECT_DEATH(bufferptr(original, 0, original.length() + 1), "");
EXPECT_DEATH(bufferptr(bufferptr(), 0, 0), "");
}
}

Expand Down Expand Up @@ -592,14 +592,14 @@ TEST(BufferPtr, accessors) {
EXPECT_EQ('X', ptr.c_str()[0]);
{
bufferptr ptr;
EXPECT_THROW(ptr.c_str(), FailedAssertion);
EXPECT_THROW(ptr[0], FailedAssertion);
EXPECT_DEATH(ptr.c_str(), "");
EXPECT_DEATH(ptr[0], "");
}
EXPECT_EQ('X', const_ptr.c_str()[0]);
{
const bufferptr const_ptr;
EXPECT_THROW(const_ptr.c_str(), FailedAssertion);
EXPECT_THROW(const_ptr[0], FailedAssertion);
EXPECT_DEATH(const_ptr.c_str(), "");
EXPECT_DEATH(const_ptr[0], "");
}
EXPECT_EQ(len, const_ptr.length());
EXPECT_EQ((unsigned)0, const_ptr.offset());
Expand All @@ -616,13 +616,13 @@ TEST(BufferPtr, accessors) {
bufferptr ptr;
EXPECT_EQ((unsigned)0, ptr.unused_tail_length());
}
EXPECT_THROW(ptr[len], FailedAssertion);
EXPECT_THROW(const_ptr[len], FailedAssertion);
EXPECT_DEATH(ptr[len], "");
EXPECT_DEATH(const_ptr[len], "");
{
const bufferptr const_ptr;
EXPECT_THROW(const_ptr.raw_c_str(), FailedAssertion);
EXPECT_THROW(const_ptr.raw_length(), FailedAssertion);
EXPECT_THROW(const_ptr.raw_nref(), FailedAssertion);
EXPECT_DEATH(const_ptr.raw_c_str(), "");
EXPECT_DEATH(const_ptr.raw_length(), "");
EXPECT_DEATH(const_ptr.raw_nref(), "");
}
EXPECT_NE((const char *)NULL, const_ptr.raw_c_str());
EXPECT_EQ(len, const_ptr.raw_length());
Expand Down Expand Up @@ -668,7 +668,7 @@ TEST(BufferPtr, is_zero) {
TEST(BufferPtr, copy_out) {
{
const bufferptr ptr;
EXPECT_THROW(ptr.copy_out((unsigned)0, (unsigned)0, NULL), FailedAssertion);
EXPECT_DEATH(ptr.copy_out((unsigned)0, (unsigned)0, NULL), "");
}
{
char in[] = "ABC";
Expand Down Expand Up @@ -703,13 +703,13 @@ TEST(BufferPtr, copy_out_bench) {
TEST(BufferPtr, copy_in) {
{
bufferptr ptr;
EXPECT_THROW(ptr.copy_in((unsigned)0, (unsigned)0, NULL), FailedAssertion);
EXPECT_DEATH(ptr.copy_in((unsigned)0, (unsigned)0, NULL), "");
}
{
char in[] = "ABCD";
bufferptr ptr(2);
EXPECT_THROW(ptr.copy_in((unsigned)0, strlen(in) + 1, NULL), FailedAssertion);
EXPECT_THROW(ptr.copy_in(strlen(in) + 1, (unsigned)0, NULL), FailedAssertion);
EXPECT_DEATH(ptr.copy_in((unsigned)0, strlen(in) + 1, NULL), "");
EXPECT_DEATH(ptr.copy_in(strlen(in) + 1, (unsigned)0, NULL), "");
ptr.copy_in((unsigned)0, (unsigned)2, in);
EXPECT_EQ(in[0], ptr[0]);
EXPECT_EQ(in[1], ptr[1]);
Expand Down Expand Up @@ -737,13 +737,13 @@ TEST(BufferPtr, copy_in_bench) {
TEST(BufferPtr, append) {
{
bufferptr ptr;
EXPECT_THROW(ptr.append('A'), FailedAssertion);
EXPECT_THROW(ptr.append("B", (unsigned)1), FailedAssertion);
EXPECT_DEATH(ptr.append('A'), "");
EXPECT_DEATH(ptr.append("B", (unsigned)1), "");
}
{
bufferptr ptr(2);
EXPECT_THROW(ptr.append('A'), FailedAssertion);
EXPECT_THROW(ptr.append("B", (unsigned)1), FailedAssertion);
EXPECT_DEATH(ptr.append('A'), "");
EXPECT_DEATH(ptr.append("B", (unsigned)1), "");
ptr.set_length(0);
ptr.append('A');
EXPECT_EQ((unsigned)1, ptr.length());
Expand Down Expand Up @@ -776,7 +776,7 @@ TEST(BufferPtr, append_bench) {
TEST(BufferPtr, zero) {
char str[] = "XXXX";
bufferptr ptr(buffer::create_static(strlen(str), str));
EXPECT_THROW(ptr.zero(ptr.length() + 1, 0), FailedAssertion);
EXPECT_DEATH(ptr.zero(ptr.length() + 1, 0), "");
ptr.zero(1, 1);
EXPECT_EQ('X', ptr[0]);
EXPECT_EQ('\0', ptr[1]);
Expand Down Expand Up @@ -1882,7 +1882,7 @@ TEST(BufferList, append) {
bufferptr in(back);
EXPECT_EQ((unsigned)1, bl.buffers().size());
EXPECT_EQ((unsigned)1, bl.length());
EXPECT_THROW(bl.append(in, (unsigned)100, (unsigned)100), FailedAssertion);
EXPECT_DEATH(bl.append(in, (unsigned)100, (unsigned)100), "");
EXPECT_LT((unsigned)0, in.unused_tail_length());
in.append('B');
bl.append(in, back.end(), 1);
Expand Down Expand Up @@ -2398,7 +2398,7 @@ TEST(BufferList, zero) {
bufferptr ptr(s[i], strlen(s[i]));
bl.push_back(ptr);
}
EXPECT_THROW(bl.zero((unsigned)0, (unsigned)2000), FailedAssertion);
EXPECT_DEATH(bl.zero((unsigned)0, (unsigned)2000), "");
bl.zero((unsigned)2, (unsigned)5);
EXPECT_EQ(0, ::memcmp("AB\0\0\0\0\0HIKLM", bl.c_str(), 9));
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/ceph_compatset.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ TEST(CephCompatSet, AllSet) {
CompatSet::FeatureSet ro;
CompatSet::FeatureSet incompat;

EXPECT_THROW(compat.insert(CompatSet::Feature(0, "test")), FailedAssertion);
EXPECT_THROW(compat.insert(CompatSet::Feature(64, "test")), FailedAssertion);
EXPECT_DEATH(compat.insert(CompatSet::Feature(0, "test")), "");
EXPECT_DEATH(compat.insert(CompatSet::Feature(64, "test")), "");

for (int i = 1; i < 64; i++) {
stringstream cname;
Expand Down
66 changes: 33 additions & 33 deletions src/test/common/Throttle.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ class ThrottleTest : public ::testing::Test {
int64_t count;
bool waited;

Thread_get(Throttle& _throttle, int64_t _count) :
Thread_get(Throttle& _throttle, int64_t _count) :
throttle(_throttle),
count(_count),
waited(false)
{
}

virtual void *entry() {
waited = throttle.get(count);
throttle.put(count);
Expand All @@ -54,9 +54,9 @@ class ThrottleTest : public ::testing::Test {
};

TEST_F(ThrottleTest, Throttle) {
ASSERT_THROW({
ASSERT_DEATH({
Throttle throttle(g_ceph_context, "throttle", -1);
}, FailedAssertion);
}, "");

int64_t throttle_max = 10;
Throttle throttle(g_ceph_context, "throttle", throttle_max);
Expand All @@ -67,7 +67,7 @@ TEST_F(ThrottleTest, Throttle) {
TEST_F(ThrottleTest, take) {
int64_t throttle_max = 10;
Throttle throttle(g_ceph_context, "throttle", throttle_max);
ASSERT_THROW(throttle.take(-1), FailedAssertion);
ASSERT_DEATH(throttle.take(-1), "");
ASSERT_EQ(throttle.take(throttle_max), throttle_max);
ASSERT_EQ(throttle.take(throttle_max), throttle_max * 2);
}
Expand All @@ -83,18 +83,18 @@ TEST_F(ThrottleTest, get) {
ASSERT_EQ(throttle.put(throttle_max), 0);
}

ASSERT_THROW(throttle.get(-1), FailedAssertion);
ASSERT_FALSE(throttle.get(5));
ASSERT_EQ(throttle.put(5), 0);
ASSERT_DEATH(throttle.get(-1), "");
ASSERT_FALSE(throttle.get(5));
ASSERT_EQ(throttle.put(5), 0);

ASSERT_FALSE(throttle.get(throttle_max));
ASSERT_FALSE(throttle.get_or_fail(1));
ASSERT_FALSE(throttle.get(1, throttle_max + 1));
ASSERT_EQ(throttle.put(throttle_max + 1), 0);
ASSERT_FALSE(throttle.get(0, throttle_max));
ASSERT_FALSE(throttle.get(throttle_max));
ASSERT_FALSE(throttle.get_or_fail(1));
ASSERT_EQ(throttle.put(throttle_max), 0);
ASSERT_FALSE(throttle.get(throttle_max));
ASSERT_FALSE(throttle.get_or_fail(1));
ASSERT_FALSE(throttle.get(1, throttle_max + 1));
ASSERT_EQ(throttle.put(throttle_max + 1), 0);
ASSERT_FALSE(throttle.get(0, throttle_max));
ASSERT_FALSE(throttle.get(throttle_max));
ASSERT_FALSE(throttle.get_or_fail(1));
ASSERT_EQ(throttle.put(throttle_max), 0);

useconds_t delay = 1;

Expand All @@ -103,24 +103,24 @@ TEST_F(ThrottleTest, get) {
do {
cout << "Trying (1) with delay " << delay << "us\n";

ASSERT_FALSE(throttle.get(throttle_max));
ASSERT_FALSE(throttle.get_or_fail(throttle_max));
ASSERT_FALSE(throttle.get(throttle_max));
ASSERT_FALSE(throttle.get_or_fail(throttle_max));

Thread_get t(throttle, 7);
t.create();
usleep(delay);
ASSERT_EQ(throttle.put(throttle_max), 0);
ASSERT_EQ(throttle.put(throttle_max), 0);
t.join();

if (!(waited = t.waited))
delay *= 2;
} while(!waited);

do {
cout << "Trying (2) with delay " << delay << "us\n";

ASSERT_FALSE(throttle.get(throttle_max / 2));
ASSERT_FALSE(throttle.get_or_fail(throttle_max));
ASSERT_FALSE(throttle.get_or_fail(throttle_max));

Thread_get t(throttle, throttle_max);
t.create();
Expand All @@ -138,13 +138,13 @@ TEST_F(ThrottleTest, get) {
if (!(waited = t.waited && u.waited))
delay *= 2;
} while(!waited);

}

TEST_F(ThrottleTest, get_or_fail) {
{
Throttle throttle(g_ceph_context, "throttle");

ASSERT_TRUE(throttle.get_or_fail(5));
ASSERT_TRUE(throttle.get_or_fail(5));
}
Expand All @@ -161,8 +161,8 @@ TEST_F(ThrottleTest, get_or_fail) {
ASSERT_FALSE(throttle.get_or_fail(throttle_max * 2));
ASSERT_EQ(throttle.put(throttle_max * 2), 0);

ASSERT_TRUE(throttle.get_or_fail(throttle_max));
ASSERT_FALSE(throttle.get_or_fail(1));
ASSERT_TRUE(throttle.get_or_fail(throttle_max));
ASSERT_FALSE(throttle.get_or_fail(1));
ASSERT_EQ(throttle.put(throttle_max), 0);
}
}
Expand All @@ -185,16 +185,16 @@ TEST_F(ThrottleTest, wait) {
cout << "Trying (3) with delay " << delay << "us\n";

ASSERT_FALSE(throttle.get(throttle_max / 2));
ASSERT_FALSE(throttle.get_or_fail(throttle_max));
ASSERT_FALSE(throttle.get_or_fail(throttle_max));

Thread_get t(throttle, throttle_max);
t.create();
usleep(delay);

//
// Throttle::_reset_max(int64_t m) used to contain a test
// that blocked the following statement, only if
// the argument was greater than throttle_max.
// that blocked the following statement, only if
// the argument was greater than throttle_max.
// Although a value lower than throttle_max would cover
// the same code in _reset_max, the throttle_max * 100
// value is left here to demonstrate that the problem
Expand All @@ -220,7 +220,7 @@ TEST_F(ThrottleTest, destructor) {
int64_t throttle_max = 10;
Throttle *throttle = new Throttle(g_ceph_context, "throttle", throttle_max);

ASSERT_FALSE(throttle->get(5));
ASSERT_FALSE(throttle->get(5));

t = new Thread_get(*throttle, 7);
t->create();
Expand All @@ -238,8 +238,8 @@ TEST_F(ThrottleTest, destructor) {
} while(!blocked);
delete throttle;
}
{ //

{ //
// The thread is left hanging, otherwise it will abort().
// Deleting the Throttle on which it is waiting creates a
// inconsistency that will be detected: the Throttle object that
Expand All @@ -265,8 +265,8 @@ int main(int argc, char **argv) {

/*
* Local Variables:
* compile-command: "cd ../.. ;
* make unittest_throttle ;
* compile-command: "cd ../.. ;
* make unittest_throttle ;
* ./unittest_throttle # --gtest_filter=ThrottleTest.destructor \
* --log-to-stderr=true --debug-filestore=20
* "
Expand Down

0 comments on commit f07e029

Please sign in to comment.