Mark Nelson called attention to this topic again yesterday in #ceph-devel. We favor retaining the ability to re-declare specific asserts such that they are, as with assert in general historically, compiled out. I think it's reasonable if this is a marked case. That seems to imply that if ceph_assert is the unmarked case, it won't be compiled out, so is essentially ceph_assert_always, as below (or else that form should be used...it's a little long). Matt ----- Original Message ----- > From: "Sage Weil" <sweil@xxxxxxxxxx> > To: "John Spray" <jspray@xxxxxxxxxx> > Cc: "Ceph Development" <ceph-devel@xxxxxxxxxxxxxxx> > Sent: Monday, June 27, 2016 12:45:10 PM > 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 > -- Matt Benjamin Red Hat, Inc. 315 West Huron Street, Suite 140A Ann Arbor, Michigan 48103 http://www.redhat.com/en/technologies/storage tel. 734-707-0660 fax. 734-769-8938 cel. 734-216-5309 -- 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
