ZFS_DEBUG + enable ZFS ASSERTS with INVARIANTS any objections?

[Justin T. Gibbs]

On Jun 29, 2013, at 5:47 PM, "Steven Hartland" <smh at freebsd.org> wrote:

> The attached patch adds a ZFS_DEBUG option as well
> as enabling ZFS ASSERTS by default when the kernel
> is compiled with INVARIANTS.
> 
> This should enable us to get better test coverage
> from CURRENT users and has already enabled me to
> catch a number of issues when testing code + fix
> for invalid ASSERT removed by r252390.
> 
> So the question is there any objections to this
> patch?
> 
>   Regards
>   Steve<zfs-debug.patch>_______________________________________________
> zfs-devel at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/zfs-devel
> To unsubscribe, send any mail to "zfs-devel-unsubscribe at freebsd.org"

The patch seems to be only a partial solution. If DEBUG is the
OpenSolaris equivalent of our INVARIANTS, shouldnt DEBUG be defined
in all contexts when INVARIANTS is enabled?  Otherwise, ASSERTS
that depend on state maintained in #if DEBUG code in a .c file will
unexpectedly fail.

Just leaving DEBUG defined upon exit of sys/debug.h isn't enough.
It appears that dtrace.c uses DEBUG without including (at least
directly) this header.  This usage (just like INVARIANTS) should
be supported.

In your patch, everyone does gets DEBUG if ZFS_DEBUG is set.  While
convenient for us ZFS developers, this should also work for someone
working on DTrace, or a bug in the OpenSolaris nvpair code, etc.
So I'd argue that the option is misnamed for at least the OpenSolaris
module.

On possible solution to this problem would be to grep opt_global.h
from within the Makefile for OpenSolaris-ish modules:

INVARIANTS_ENABLED!=   grep INVARIANTS ${KERNBUILDDIR}/opt_global.h || true
.if !empty(INVARIANTS_ENABLED)
CFLAGS += -DDEBUG
.endif

If ZFS_DEBUG were also made a global option, similar logic could
fail the ZFS module build if ZFS_DEBUG is enabled without INVARIANTS.

--
Justin