From owner-freebsd-hackers Sun Feb 8 22:51:38 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id WAA20689 for hackers-outgoing; Sun, 8 Feb 1998 22:51:38 -0800 (PST) (envelope-from owner-freebsd-hackers@FreeBSD.ORG) Received: from ns1.yes.no (ns1.yes.no [195.119.24.10]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id WAA20661 for ; Sun, 8 Feb 1998 22:51:33 -0800 (PST) (envelope-from eivind@bitbox.follo.net) Received: from bitbox.follo.net (bitbox.follo.net [194.198.43.36]) by ns1.yes.no (8.8.7/8.8.7) with ESMTP id GAA06773; Mon, 9 Feb 1998 06:51:28 GMT Received: (from eivind@localhost) by bitbox.follo.net (8.8.6/8.8.6) id HAA03687; Mon, 9 Feb 1998 07:51:27 +0100 (MET) Message-ID: <19980209075127.63680@follo.net> Date: Mon, 9 Feb 1998 07:51:27 +0100 From: Eivind Eklund To: Michael Hancock Cc: FreeBSD Hackers Subject: Re: DIAGNOSTICS and DEBUG LOGGING (was Re: cvs commit: src/sys/conf options) References: <199802061241.EAA24833@freefall.freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Mailer: Mutt 0.88e In-Reply-To: ; from Michael Hancock on Mon, Feb 09, 1998 at 03:23:45PM +0900 Sender: owner-freebsd-hackers@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Mon, Feb 09, 1998 at 03:23:45PM +0900, Michael Hancock wrote: > Eivind, > > I'd like to see "sanity checks" (assertions) and diagnostic logging > separated. DIAGNOSTICS turns on both, but I'd like to be able to run an > assertion checking kernel without all the logging. Absolutely agreed. I was thinking of _INVARIANTS - Enable invariant/postcondition checking (expensive) _ASSERTS - Enable precondition and other cheap assertions INVARIANT_CODE - Compile in invariant functions. DIAGNOSTIC - Messages to help tracing errors; non-overwhelming amount. The difference between _INVARIANTS and INVARIANT_CODE is that INVARIANT_CODE just includes the actual code necessary to be able to check an invariant, while _INVARIANTS actually throw in the code that do calls to check the invariant. The reason for the underscores is that header files are likely to depend on those options. The separation of the INVARIANT_CODE and _INVARIANTS is to be able to support enabling invariant-checks in only some files. Separation of _INVARIANTS and _ASSERTS is that there is often a factor of >1000 difference between the cost of checking pre-conditions and the cost of checking post-conditions/data-structure invariants. Example of how the above would affect source-code: The following would be the diffs to kern/tty_subr.c to implement a clist invariant. (It is some of the code I've got lying around of the type nobody-seems-to-be-interested-so-I-won't-commit-it-until- it-seems-I-can-find-a-nice-way-to-fit-it-in). Index: tty_subr.c =================================================================== RCS file: /home/ncvs/src/sys/kern/tty_subr.c,v retrieving revision 1.28 diff -u -r1.28 tty_subr.c --- tty_subr.c 1997/10/12 20:24:09 1.28 +++ tty_subr.c 1998/02/09 06:37:34 @@ -112,6 +112,10 @@ cblock_free(cblockp) struct cblock *cblockp; { +#ifdef _ASSERTS + if ((unsigned long)cblockp & (CBLOCK-1)) + panic("Unaligned cblock in cblock_free"); +#endif if (isset(cblockp->c_quote, CBQSIZE * NBBY - 1)) bzero(cblockp->c_quote, sizeof cblockp->c_quote); cblockp->c_next = cfreelist; @@ -136,6 +140,10 @@ "clist_alloc_cblocks: M_NOWAIT malloc failed, trying M_WAITOK\n"); cbp = malloc(sizeof *cbp, M_TTYS, M_WAITOK); } +#ifdef _ASSERTS + if ((unsigned long)cbp & (CBLOCK-1)) + panic("Unaligned cblock alloced in cblock_alloc_cblocks"); +#endif /* * Freed cblocks have zero quotes and garbage elsewhere. * Set the may-have-quote bit to force zeroing the quotes. @@ -260,6 +268,117 @@ return (chr); } +#if defined(INVARIANT_CODE) || defined(_INVARIANTS) +/* Verify the clist invariant. Will panic if passed an invalid clist. + Intended for debugging of tty-drivers. */ +void +clist_invariant( + struct clist *clistp, /* clist to verify */ + char *descr /* Section of program causing panic */ +) { + struct cblock *cblockp; /* Presently examined cblock */ + struct cblock *cblockp2; /* Loop limit cblock */ + int real_cc; /* Real character count */ + int max_loop; /* Number of cblocks to check (max) */ + + if (descr == NULL) + descr = "undescribed"; + + if (clistp == NULL) + panic("clist invariant: NULL clist in %s", descr); + + /* Check for negative counts */ + if (clistp->c_cc < 0) + panic("clist invariant: negative character count(%d) in %s", + clistp->c_cc, descr); + if (clistp->c_cbcount < 0) + panic("clist invariant: negative block count (%d) in %s", + clistp->c_cbcount, descr); + if (clistp->c_cbmax < 0) + panic("clist invariant: negative max block count (%d) in %s", + clistp->c_cbmax, descr); + if (clistp->c_cbreserved < 0) + panic("clist invariant: negative reserved block count (%d) in %s", + clistp->c_cbreserved, descr); + + /* Check for generally invalid counts */ + + if (clistp->c_cc && roundup(clistp->c_cc, CBSIZE) / CBSIZE > clistp->c_cbcount) + panic("clist invariant: too few cblocks for c_cc (%d vs %d) in %s", + clistp->c_cbcount, clistp->c_cc, descr); + if (roundup(clistp->c_cc, CBSIZE) / CBSIZE + 2 < clistp->c_cbcount) + panic("clist invariant: too many cblocks for c_cc (%d vs %d) in %s", + clistp->c_cbcount, clistp->c_cc, descr); + if (clistp->c_cbcount > clistp->c_cbmax) + panic("clist invariant: more cblocks than cb_max (%d vs %d) in %s", + clistp->c_cbcount, clistp->c_cbmax, descr); + + if (clistp->c_cc == 0) { + if (clistp->c_cf && clistp->c_cl && + clistp->c_cf != clistp->c_cl) + panic("clist invariant: cf (%p) and cl (%p) differ for empty clist in %s", + clistp->c_cf, clistp->c_cl, descr); + return; + } + if (clistp->c_cf == NULL) + panic("clist invariant: cf is NULL with c_cc=%d (cl=%p) in %s", + clistp->c_cc, clistp->c_cl, descr); + if (clistp->c_cl == NULL) + panic("clist invariant: cl is NULL with c_cc=%d (cf=%p) in %s", + clistp->c_cc, clistp->c_cf, descr); + + /* Traverse clist to find actual character count and verify that the + last pointer exists */ + cblockp = (struct cblock *)((unsigned long)clistp->c_cf & ~CROUND); + real_cc = (char*)cblockp->c_info - clistp->c_cf; + if (real_cc > 0) + panic("clist invariant: cf (%p) points outside c_info block in %s", + clistp->c_cf, descr); + + /* Limit looping */ + max_loop = clistp->c_cbcount*2 + 2; + cblockp2 = cblockp; + + while (cblockp) { + if ((void*)clistp->c_cl > (void*)cblockp && + (char*)clistp->c_cl <= (char*)cblockp + CBLOCK) { + + /* We've found the last block in the list - handle character + count and other checks */ + if (clistp->c_cl < (char*)cblockp->c_info) + panic("clist invariant: cl (%p) points outside c_info block in %s", + clistp->c_cl); + real_cc += clistp->c_cl - (char*)cblockp->c_info; + if (real_cc != clistp->c_cc) + panic("clist invariant: real cc (%d) does not match c_cc (%d) in %s", + real_cc, clistp->c_cc, descr); + if (cblockp->c_next) + panic("clist invariant: non-NULL c_next (%p) on last cblock in %s", + cblockp->c_next, descr); + /* Invariant OK */ + return; + } + + /* Increase character count */ + real_cc += CBSIZE; + if (max_loop-- <= 0) { + panic("clist invariant: too long "); + } + cblockp = cblockp->c_next; + if (cblockp == cblockp2) + panic("clist invariant: loop in clist (%p) in %s", + clistp, descr); + if ((max_loop & 1) == 0) { + cblockp2 = cblockp2->c_next; + } + if (cblockp == cblockp2) + panic("clist invariant: loop in clist (%p) in %s", + clistp, descr); + } + panic("clist invariant: terminated before finding c_cl"); +} +#endif + /* * Copy 'amount' of chars, beginning at head of clist 'clistp' to * destination linear buffer 'dest'. Return number of characters @@ -277,8 +396,15 @@ int numc; int s; +#ifdef _ASSERTS + if (dest == NULL) + panic("destination is NULL in q_to_b"); +#endif s = spltty(); +#ifdef _INVARIANTS + clist_invariant(clistp, "q_to_b"); +#endif while (clistp && amount && (clistp->c_cc > 0)) { cblockp = (struct cblock *)((long)clistp->c_cf & ~CROUND); cblockn = cblockp + 1; /* pointer arithmetic! */ @@ -435,6 +561,10 @@ int startbit, endbit, num_between, numc; int s; +#ifdef _ASSERTS + if (src == NULL) + panic("clist b_to_q on NULL block"); +#endif /* * Avoid allocating an initial cblock and then not using it. * c_cc == 0 must imply c_cbount == 0. @@ -444,6 +574,9 @@ s = spltty(); +#ifdef _INVARIANTS + clist_invariant(clistp, "b_to_q"); +#endif /* * If there are no cblocks assigned to this clist yet, * then get one. > Do you think that DIAGNOSTICS can be separated into these 2 categories > without upsetting too many people? I haven't got a clue. I've been planning to try it, though. Do the above scheme look good to you? > What does DEBUG do? Can all sanity checks be moved to DEBUG? DEBUG does an insane pletoria of different things, all depending on what the driver-author (or whatever) wanted it to do. It usually turn on insane amounts of debugging information. > This would give us more clearly defined debugging flags: > > DIAGNOSTICS - Debug logging > DEBUG - Sanity checks > DDS - GDB callable debug functions. I like the distinctions, but not the names :-) Eivind. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe hackers" in the body of the message