Re: [PATCH 1/8] cleanup: get rid of ASSERT

From: Dave Chinner <david@fromorbit.com>
To: Brian Foster <bfoster@redhat.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 1/8] cleanup: get rid of ASSERT
Date: Thu, 29 Oct 2015 09:32:34 +1100	[thread overview]
Message-ID: <20151028223234.GQ8773@dastard> (raw)
In-Reply-To: <20151028115121.GA50552@bfoster.bfoster>

On Wed, Oct 28, 2015 at 07:51:22AM -0400, Brian Foster wrote:
> On Fri, Oct 16, 2015 at 12:44:54PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > ASSERT comes from the xfs/xfs.h include, and we don't ever define
> > DEBUG so we never get asserts built in. We want asserts built in for
> > testing, but not for distro packages. The debian package already
> > tries to do this by using "export DEBUG=-DNDEBUG" for the build
> > context, but seeing as we pull in #define ASSERT(ex) (0) from the
> > XFS headers it's a no-op.
> > 
> > Convert all the ASSERT calls to assert to remove this conflict with
> > the xfsprogs headers and so local developer builds are built with
> > asserts enabled.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> 
> My initial concern when reading this was that asserts were now
> unconditionally enabled. According to the man page, assert() is enabled
> unless NDEBUG is defined at include time. The debian builder apparently
> does this, but is this standard for other such utils?

For other XFS utilities? Yes. For other packages I don't maintain? I
don't know, I don't care, and I don't think it's relevant at all.
How the distro builds and packages a utility is up to the distro
maintainers - if they define -NDEBUG, then there is absolutely no
change in behaviour of xfsdump. I think the binary is effectively
unchanged as assert() is defined out in that case...

> That aside, this seems like an inconsistent approach from our other
> tools. For example, the debian/rules from xfsprogs does the same thing,
> yet DEBUG must be defined to enable asserts in the first place.

That's a issue with the way xfsprogs uses asserts - ASSERT should
never have been exported in the first, nor should the xfsdump buildi
rely on it, but here we are....

> If the
> problem is that asserts cannot be enabled, I'm wondering why the
> appropriate fix isn't to define DEBUG somewhere for debug-enabled builds
> rather than unconditionally convert all of the ASSERT() calls into
> actual assert()'s..?

Becuase it has to build with both old and new xfsprogs userspace
headers. Hence we cannot use ASSERT reliably in xfsdump at all,
because it's owned by the old xfsprogs headers and it's behaviour is
determined by whatever that xfsprogs installation defined......

> We do actually have a bunch of '#ifdef DEBUG' code
> throughout xfsdump (and DEBUG appears in 'configure' as well, though I
> have no idea if that actually does anything)...

I'm not going to pull that string and have this turn into a massive
"we have to change everything" exercise. I've built the code with
and without NDEBUG enabled, and had no problems with the assert code
either way.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs