assert

assert

[Sage Weil]

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.

2. Replace all other instances of assert with ceph_assert.

That's a lot of work, though.  Other ideas?

sage

RE: assert

[Ramesh Chander]

> -----Original Message-----
> From: ceph-devel-owner@vger.kernel.org [mailto:ceph-devel-
> owner@vger.kernel.org] On Behalf Of Sage Weil
> Sent: Monday, June 27, 2016 9:47 PM
> To: ceph-devel@vger.kernel.org
> Subject: assert
>
> 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.
>
> 2. Replace all other instances of assert with ceph_assert.

I think doing this should be more simple and portable. Hide all details in ceph_assert.

>
> That's a lot of work, though.  Other ideas?
>
> sage
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
PLEASE NOTE: The information contained in this electronic mail message is intended only for the use of the designated recipient(s) named above. If the reader of this message is not the intended recipient, you are hereby notified that you have received this message in error and that any review, dissemination, distribution, or copying of this message is strictly prohibited. If you have received this communication in error, please notify the sender by telephone or e-mail (as shown above) immediately and destroy any and all copies of this message in your possession (whether hard copies or electronically stored copies).

Re: assert

[John Spray]

On Mon, Jun 27, 2016 at 5:16 PM, Sage Weil <sweil@redhat.com> 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).

John


>
> sage
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: assert

[Sage Weil]

On Mon, 27 Jun 2016, John Spray wrote:
> On Mon, Jun 27, 2016 at 5:16 PM, Sage Weil <sweil@redhat.com> 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

RE: assert

[Allen Samuels]

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@SanDisk.com


> -----Original Message-----
> From: ceph-devel-owner@vger.kernel.org [mailto:ceph-devel-
> owner@vger.kernel.org] On Behalf Of Sage Weil
> Sent: Monday, June 27, 2016 9:45 AM
> To: John Spray <jspray@redhat.com>
> Cc: Ceph Development <ceph-devel@vger.kernel.org>
> Subject: Re: assert
> 
> On Mon, 27 Jun 2016, John Spray wrote:
> > On Mon, Jun 27, 2016 at 5:16 PM, Sage Weil <sweil@redhat.com> 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@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

RE: assert

[Somnath Roy]

Yes, if we can move some assert out in the io path for performance build that will definitely save some cpu cycles in the io path.  +1 for this proposal.

Thanks & Regards
Somnath

-----Original Message-----
From: ceph-devel-owner@vger.kernel.org [mailto:ceph-devel-owner@vger.kernel.org] On Behalf Of Sage Weil
Sent: Monday, June 27, 2016 9:45 AM
To: John Spray
Cc: Ceph Development
Subject: Re: assert

On Mon, 27 Jun 2016, John Spray wrote:
> On Mon, Jun 27, 2016 at 5:16 PM, Sage Weil <sweil@redhat.com> 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@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html
PLEASE NOTE: The information contained in this electronic mail message is intended only for the use of the designated recipient(s) named above. If the reader of this message is not the intended recipient, you are hereby notified that you have received this message in error and that any review, dissemination, distribution, or copying of this message is strictly prohibited. If you have received this communication in error, please notify the sender by telephone or e-mail (as shown above) immediately and destroy any and all copies of this message in your possession (whether hard copies or electronically stored copies).

Re: assert

[kefu chai]

On Tue, Jun 28, 2016 at 12:16 AM, Sage Weil <sweil@redhat.com> 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.

yeah, we do need this. some of the tests are actually using assert()
as ASSERT_TRUE().
i posted a PR for it: https://github.com/ceph/ceph/pull/9975.

>
> 1. Do an audit and replace assert(0) et al with ceph_abort(const char
> *msg), or similar.
>
> 2. Replace all other instances of assert with ceph_assert.
>
> That's a lot of work, though.  Other ideas?
>
> sage
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Regards
Kefu Chai

Re: assert

[Matt Benjamin]

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@redhat.com>
> To: "John Spray" <jspray@redhat.com>
> Cc: "Ceph Development" <ceph-devel@vger.kernel.org>
> 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@redhat.com> 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@vger.kernel.org
> 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

Re: assert

[Adam C. Emerson]

On 24/08/2016, Matt Benjamin wrote:
> 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).

If we want to go for brevity, we could have ceph_assert() be the
unmarked case that functions just like assert macro.

Then we could use something like ceph_demand() (ceph_ultimatum?
ceph_blackmail?) to indicate the case that isn't compiled out.

Personally, I would just like to get rid of Ceph's assert.h and its
unending war with every other header file in the world. At one point
killed off the header and linked in an object that overrode
__assert_fail. This worked very well, though it would require handling
every C library implementation we're compiled against.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

Re: assert

[Casey Bodley]

On 08/24/2016 11:45 AM, Adam C. Emerson wrote:
> On 24/08/2016, Matt Benjamin wrote:
>> 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).
> If we want to go for brevity, we could have ceph_assert() be the
> unmarked case that functions just like assert macro.
>
> Then we could use something like ceph_demand() (ceph_ultimatum?
> ceph_blackmail?) to indicate the case that isn't compiled out.
>
> Personally, I would just like to get rid of Ceph's assert.h and its
> unending war with every other header file in the world. At one point
> killed off the header and linked in an object that overrode
> __assert_fail. This worked very well, though it would require handling
> every C library implementation we're compiled against.
>

I dug up an old commit as an example of overriding libc's 
__assert_fail() function, which would allow us to use the system's 
assert.h as is: 
https://github.com/cohortfsllc/cohortfs-ceph/commit/40956ad3661192462246ed7e5435522c5708ffbc

Re: assert

[Sage Weil]

On Wed, 24 Aug 2016, Adam C. Emerson wrote:
> On 24/08/2016, Matt Benjamin wrote:
> > 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).
> 
> If we want to go for brevity, we could have ceph_assert() be the
> unmarked case that functions just like assert macro.
> 
> Then we could use something like ceph_demand() (ceph_ultimatum?
> ceph_blackmail?) to indicate the case that isn't compiled out.
> 
> Personally, I would just like to get rid of Ceph's assert.h and its
> unending war with every other header file in the world. At one point
> killed off the header and linked in an object that overrode
> __assert_fail. This worked very well, though it would require handling
> every C library implementation we're compiled against.

This is appealing, except:

> > 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 think not getting the assertion condition and line number in the ceph 
log is a deal breaker.


I would go for ceph_abort (abort with message in log etc), ceph_assert 
(may get compiled out in the future), and ceph_demand (never compiled 
out).

sage

Re: assert

[Sage Weil]

On Wed, 24 Aug 2016, Casey Bodley wrote:
> On 08/24/2016 11:45 AM, Adam C. Emerson wrote:
> > On 24/08/2016, Matt Benjamin wrote:
> > > 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).
> > If we want to go for brevity, we could have ceph_assert() be the
> > unmarked case that functions just like assert macro.
> > 
> > Then we could use something like ceph_demand() (ceph_ultimatum?
> > ceph_blackmail?) to indicate the case that isn't compiled out.
> > 
> > Personally, I would just like to get rid of Ceph's assert.h and its
> > unending war with every other header file in the world. At one point
> > killed off the header and linked in an object that overrode
> > __assert_fail. This worked very well, though it would require handling
> > every C library implementation we're compiled against.
> > 
> 
> I dug up an old commit as an example of overriding libc's __assert_fail()
> function, which would allow us to use the system's assert.h as is:
> https://github.com/cohortfsllc/cohortfs-ceph/commit/40956ad3661192462246ed7e5435522c5708ffbc

Oh, this looks nice.

In that case, we still need a ceph_abort (and an associated cleanup).

I think we also still need some distinction between asserts that are safe 
to compile out and those that are not, though.

sage

Re: assert

[Adam C. Emerson]

On 24/08/2016, Sage Weil wrote:
[snip]
> This is appealing, except:
> 
> > > 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.
[snip]
> I think not getting the assertion condition and line number in the
> ceph log is a deal breaker.

We should be able to get the condition and line number in the log,
they're passed to __assert_fail() so we could pass them to
ceph_assert_fail (see the commit that Casey linked to.)

I can see arguments either way, the main one I would make AGAINST this
approach is that it makes portability/building more complicated.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

Re: assert

[Yehuda Sadeh-Weinraub]

On Wed, Aug 24, 2016 at 9:26 AM, Adam C. Emerson <aemerson@redhat.com> wrote:
> On 24/08/2016, Sage Weil wrote:
> [snip]
>> This is appealing, except:
>>
>> > > 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.
> [snip]
>> I think not getting the assertion condition and line number in the
>> ceph log is a deal breaker.
>
> We should be able to get the condition and line number in the log,
> they're passed to __assert_fail() so we could pass them to
> ceph_assert_fail (see the commit that Casey linked to.)

Will that work on any system we compile on?

>
> I can see arguments either way, the main one I would make AGAINST this
> approach is that it makes portability/building more complicated.
>
> --
> Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
> IRC: Aemerson@{RedHat, OFTC, Freenode}
> 0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: assert

[Matt Benjamin]

You're right, it's non-portable.  I think that's a strike against it.

Matt

----- Original Message -----
> From: "Yehuda Sadeh-Weinraub" <ysadehwe@redhat.com>
> To: "Sage Weil" <sweil@redhat.com>, "Matt Benjamin" <mbenjamin@redhat.com>, "John Spray" <jspray@redhat.com>, "Ceph
> Development" <ceph-devel@vger.kernel.org>, "Mark Nelson" <mnelson@redhat.com>
> Sent: Wednesday, August 24, 2016 12:38:26 PM
> Subject: Re: assert
> 
> On Wed, Aug 24, 2016 at 9:26 AM, Adam C. Emerson <aemerson@redhat.com> wrote:
> > On 24/08/2016, Sage Weil wrote:
> > [snip]
> >> This is appealing, except:
> >>
> >> > > 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.
> > [snip]
> >> I think not getting the assertion condition and line number in the
> >> ceph log is a deal breaker.
> >
> > We should be able to get the condition and line number in the log,
> > they're passed to __assert_fail() so we could pass them to
> > ceph_assert_fail (see the commit that Casey linked to.)
> 
> Will that work on any system we compile on?
> 
> >
> > I can see arguments either way, the main one I would make AGAINST this
> > approach is that it makes portability/building more complicated.
> >
> > --
> > Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
> > IRC: Aemerson@{RedHat, OFTC, Freenode}
> > 0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
> > --
> > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > 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@vger.kernel.org
> 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

Re: assert

[Adam C. Emerson]

On 24/08/2016, Yehuda Sadeh-Weinraub wrote:
> Will that work on any system we compile on?

Yes and no. As given it will work on GNU libc. For FreeBSD we would
need to change the name of the function we chunk in to __assert and
the order of arguments is different. So, to do it portably, we'd need
a shim for each C library we're ported to. There aren't /that/ many C
libraries in the world so it's not a difficult thing to make work, but
it is a bit fiddly.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

Re: assert

[Willem Jan Withagen]

On 24-8-2016 18:26, Adam C. Emerson wrote:
> On 24/08/2016, Sage Weil wrote:
> [snip]
>> This is appealing, except:
>>
>>>> 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.
> [snip]
>> I think not getting the assertion condition and line number in the
>> ceph log is a deal breaker.
> 
> We should be able to get the condition and line number in the log,
> they're passed to __assert_fail() so we could pass them to
> ceph_assert_fail (see the commit that Casey linked to.)
> 
> I can see arguments either way, the main one I would make AGAINST this
> approach is that it makes portability/building more complicated.

I'd go for the full deal.

The big advantage of doing it yourself is that you are in full control.
And certainly not dependent on any system code that does the trick
slightly different.

I had already a hard time getting FreeBSD and Linux in line over a
simple thing as ENODATA == ENOATTR. And this sounds way much more
complicated.

It might be a lot of changes, but for the first step I guess you can
sort of automate it with
  perl -pni.bak 's/assert\(/ceph_assert\(/g;' \
	`find src -name \*.c -o -name \*.cc`

and then get the refinements in when people pass by in files an know the
priorities.

--WjW

Re: assert

[Matt Benjamin]

Ok, that's fair, I guess.  If a given C runtime lacks something compatible, do we then need to do for that environment exactly what we would otherwise do in general?

Matt

----- Original Message -----
> From: "Adam C. Emerson" <aemerson@redhat.com>
> To: "Yehuda Sadeh-Weinraub" <ysadehwe@redhat.com>
> Cc: "Sage Weil" <sweil@redhat.com>, "Matt Benjamin" <mbenjamin@redhat.com>, "John Spray" <jspray@redhat.com>, "Ceph
> Development" <ceph-devel@vger.kernel.org>, "Mark Nelson" <mnelson@redhat.com>
> Sent: Wednesday, August 24, 2016 1:10:00 PM
> Subject: Re: assert
> 
> On 24/08/2016, Yehuda Sadeh-Weinraub wrote:
> > Will that work on any system we compile on?
> 
> Yes and no. As given it will work on GNU libc. For FreeBSD we would
> need to change the name of the function we chunk in to __assert and
> the order of arguments is different. So, to do it portably, we'd need
> a shim for each C library we're ported to. There aren't /that/ many C
> libraries in the world so it's not a difficult thing to make work, but
> it is a bit fiddly.
> 
> --
> Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
> IRC: Aemerson@{RedHat, OFTC, Freenode}
> 0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
> 

-- 
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

Re: assert

[Adam C. Emerson]

On 24/08/2016, Matt Benjamin wrote:
> Ok, that's fair, I guess.  If a given C runtime lacks something compatible, do we then need to do for that environment exactly what we would otherwise do in general?

Maybe? I don't think there are any such. The alternative is to
open-code the error message printing and abort into the macro body and
I would be very surprised if anyone did that. So long as the macro
calls a function, we should be able to shim in an override for it.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

Re: assert

[Jonny Grant]

On 26/09/13 17:31, Michael Kerrisk (man-pages) wrote:
> On 25.09.2013 15:18, Jon Grant wrote:
>> Hello mtk
>>
>> Would it be better to replace "his" with "their", in the interests of equality?
>>
>> http://man7.org/linux/man-pages/man3/assert.3.html
>>
>>         "The purpose of this macro is to help the programmer find bugs in his
>>         program.  The message "assertion failed in file foo.c, function
>>         do_bar(), line 1287" is of no help at all to a user."
>>
>>
>> NB. This may apply to other man pages.
>
> Hi Jon,
>
> I fixed four pages along the lines you suggest.

Great!
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: assert

[Michael Kerrisk (man-pages)]

On 25.09.2013 15:18, Jon Grant wrote:
> Hello mtk
> 
> Would it be better to replace "his" with "their", in the interests of equality?
> 
> http://man7.org/linux/man-pages/man3/assert.3.html
> 
>        "The purpose of this macro is to help the programmer find bugs in his
>        program.  The message "assertion failed in file foo.c, function
>        do_bar(), line 1287" is of no help at all to a user."
> 
> 
> NB. This may apply to other man pages.

Hi Jon,

I fixed four pages along the lines you suggest.

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
My next Linux/UNIX system programming course:
http://blog.man7.org/2013/09/linuxunix-system-programming-course.html
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

assert

[Jon Grant]

Hello mtk

Would it be better to replace "his" with "their", in the interests of equality?

http://man7.org/linux/man-pages/man3/assert.3.html

       "The purpose of this macro is to help the programmer find bugs in his
       program.  The message "assertion failed in file foo.c, function
       do_bar(), line 1287" is of no help at all to a user."


NB. This may apply to other man pages.

Best regards, Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html