I'm with John, converting the code is a bit tedious but low risk. It can be scripted. I'd also like to see assert(0) replaced with more meaningful error messages. But not at the expense of delaying the conversion to cmake. Allen Samuels SanDisk |a Western Digital brand 2880 Junction Avenue, San Jose, CA 95134 T: +1 408 801 7030| M: +1 408 780 6416 allen.samuels@xxxxxxxxxxx > -----Original Message----- > From: ceph-devel-owner@xxxxxxxxxxxxxxx [mailto:ceph-devel- > owner@xxxxxxxxxxxxxxx] On Behalf Of Sage Weil > Sent: Monday, June 27, 2016 9:45 AM > To: John Spray <jspray@xxxxxxxxxx> > Cc: Ceph Development <ceph-devel@xxxxxxxxxxxxxxx> > Subject: Re: assert > > On Mon, 27 Jun 2016, John Spray wrote: > > On Mon, Jun 27, 2016 at 5:16 PM, Sage Weil <sweil@xxxxxxxxxx> wrote: > > > We have a problem with assert. Actually, a few problems. > > > > > > 1. Our code is written assuming that assert is never compiled out. > > > We have lots of assert(0) and assert(0 == "informative failure > > > message") instances all over the code that should really be something like > abort(). > > > If you compile out assert, things won't work (well, fail) as expected. > > > > > > 2. We have a special implementation of assert in include/assert.h > > > that will log the assertion failure to the global debug log, along > > > with a stack track, before crashing. This is normally fine, except > > > that lots of random library or system headers include the system > > > assert (/usr/include/assert.h), which is written such that it does > > > not lib nicely with another definition. From /usr/include/assert.h: > > > > > > #ifdef _ASSERT_H > > > > > > # undef _ASSERT_H > > > # undef assert > > > # undef __ASSERT_VOID_CAST > > > > > > # ifdef __USE_GNU > > > # undef assert_perror > > > # endif > > > > > > #endif /* assert.h */ > > > > > > #define _ASSERT_H 1 > > > > > > That means that if you include our assert, and then some other > > > random header includes /usr/include/assert, it will (silently!) clobber > ours. > > > > > > 3. This is all highlighted by the switch to cmake. The current > > > defautl build does not define NDEBUG, which means that assert is > > > compiled away, and we get a bunch of unused variable warnings from > > > code like > > > > > > r = read(...); > > > assert(r > 0); > > > > > > So... what do to? We can fix these instances when we find them, but > > > the #include order is inherently fragile. > > > > > > Note that using the system assert isn't a total disaster: system > > > assert will trigger an abort, which will trigger the SIGABRT handler > > > which *also* dumps a stack trace to the debug log. The problem is > > > that it doesn't show the assertion condition and line number. > > > > > > > > > I suggest: > > > > > > 0. Switch cmake to a debug but optimized build. Ensure the release > > > builds do the same. > > > > > > 1. Do an audit and replace assert(0) et al with ceph_abort(const > > > char *msg), or similar. > > > > Definitely support this: I find the assert(0)s kind of ugly anyway. > > > > > 2. Replace all other instances of assert with ceph_assert. > > > > Annoying but probably necessary. assert() amongst other things bit me > > recently when importing Python.h from Ceph code. > > > > > That's a lot of work, though. Other ideas? > > > > Down the line from this, I would also like us to distinguish "high > > level" asserts that indicate logical inconsistency rather than e.g. > > malformed structures, so that we can dump even more info in these > > cases (http://tracker.ceph.com/issues/11174). > > That makes sense. I have a related question too whether or not we want to > make a distinction between asserts that we can compile out in a > release/performance build and those we cannot. For example, stuff in > peering we would probably never want to compile out, and we are using > assert only because it is a convenient shorthand for "if (!x) crash". > > Maybe ceph_assert_always vs ceph_assert? > > sage > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the > body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
