Re: [trivial patch] KASSERT within DIAGNOSTIC

[Crystal Kolipe]

On Fri, Feb 03, 2023 at 06:08:09PM +, Miod Vallat wrote:
> Yes and no. You are right that KASSERT is a nop on non-DIAGNOSTIC
> kernels, but there are sometimes good reasons to keep a single KASSERT
> (or a set of KASSERTs) wrapped within #ifdef DIAGNOSTIC.

...

> In this case, I would keep the #ifdef because this field only exists
> #ifdef DIAGNOSTIC, so all the code operating on it should have such
> guards for consistency.

Ah, OK.  I did wonder if some of it was intentional, but since there are only
a few instances of KASSERT within #ifdef DIAGNOSTIC in the whole tree I
assumed that we weren't doing this elsewhere.

Re: [trivial patch] KASSERT within DIAGNOSTIC

[Miod Vallat]

> Here are a few more instances of KASSERT being placed in an #ifdef DIAGNOSTIC
> block.  This is redundant, as KASSERT is already a nop if DIAGNOSTIC is not
> defined.

Yes and no. You are right that KASSERT is a nop on non-DIAGNOSTIC
kernels, but there are sometimes good reasons to keep a single KASSERT
(or a set of KASSERTs) wrapped within #ifdef DIAGNOSTIC.

> Index: crypto/blake2s.c

In this case, the removal is valid and worth doing (in blake2s.h too).

> Index: dev/ic/ahci.c
> ===
> RCS file: /cvs/src/sys/dev/ic/ahci.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 ahci.c
> --- dev/ic/ahci.c 9 Apr 2022 20:10:26 -   1.38
> +++ dev/ic/ahci.c 19 Jan 2023 13:30:37 -
> @@ -2472,19 +2472,16 @@ ahci_put_err_ccb(struct ahci_ccb *ccb)
>  
>   splassert(IPL_BIO);
>  
> -#ifdef DIAGNOSTIC
>   KASSERT(ap->ap_err_busy);
> -#endif
> +

In this case, I would keep the #ifdef because this field only exists
#ifdef DIAGNOSTIC, so all the code operating on it should have such
guards for consistency.

>   /* No commands may be active on the chip */
>   sact = ahci_pread(ap, AHCI_PREG_SACT);
>   if (sact != 0)
>   printf("ahci_put_err_ccb but SACT %08x != 0?\n", sact);
>   KASSERT(ahci_pread(ap, AHCI_PREG_CI) == 0);
>  
> -#ifdef DIAGNOSTIC
>   /* Done with the CCB */
>   KASSERT(ccb == ap->ap_ccb_err);
> -#endif

In this case, on the other hand, the #ifdef can be removed.

> Index: dev/wscons/wsemul_sun.c
> ===
> RCS file: /cvs/src/sys/dev/wscons/wsemul_sun.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 wsemul_sun.c
> --- dev/wscons/wsemul_sun.c   25 May 2020 09:55:49 -  1.34
> +++ dev/wscons/wsemul_sun.c   19 Jan 2023 13:30:38 -
> @@ -242,9 +242,9 @@ wsemul_sun_attach(int console, const str
>  
>   if (console) {
>   edp = _sun_console_emuldata;
> -#ifdef DIAGNOSTIC
> +
>   KASSERT(edp->console == 1);
> -#endif
> +

Same as for ahci: The `console' field only exists #ifdef DIAGNOSTIC, so
the guards must remain (and in that respect, wsemul_vt100.c r1.40 was a
mistake).

> Index: net/wg_noise.c

The removal is valid here.

[trivial patch] KASSERT within DIAGNOSTIC

[Crystal Kolipe]

Here are a few more instances of KASSERT being placed in an #ifdef DIAGNOSTIC
block.  This is redundant, as KASSERT is already a nop if DIAGNOSTIC is not
defined.

Index: crypto/blake2s.c
===
RCS file: /cvs/src/sys/crypto/blake2s.c,v
retrieving revision 1.2
diff -u -p -r1.2 blake2s.c
--- crypto/blake2s.c22 Jul 2020 13:54:30 -  1.2
+++ crypto/blake2s.c19 Jan 2023 13:30:37 -
@@ -73,9 +73,7 @@ static inline void blake2s_init_param(st
 
 void blake2s_init(struct blake2s_state *state, const size_t outlen)
 {
-#ifdef DIAGNOSTIC
KASSERT(!(!outlen || outlen > BLAKE2S_HASH_SIZE));
-#endif
blake2s_init_param(state, 0x0101 | outlen);
state->outlen = outlen;
 }
@@ -85,10 +83,8 @@ void blake2s_init_key(struct blake2s_sta
 {
uint8_t block[BLAKE2S_BLOCK_SIZE] = { 0 };
 
-#ifdef DIAGNOSTIC
KASSERT(!(!outlen || outlen > BLAKE2S_HASH_SIZE ||
  !key || !keylen || keylen > BLAKE2S_KEY_SIZE));
-#endif
 
blake2s_init_param(state, 0x0101 | keylen << 8 | outlen);
state->outlen = outlen;
@@ -105,9 +101,7 @@ static inline void blake2s_compress(stru
uint32_t v[16];
int i;
 
-#ifdef DIAGNOSTIC
KASSERT(!((nblocks > 1 && inc != BLAKE2S_BLOCK_SIZE)));
-#endif
 
while (nblocks > 0) {
blake2s_increment_counter(state, inc);

Index: dev/ic/ahci.c
===
RCS file: /cvs/src/sys/dev/ic/ahci.c,v
retrieving revision 1.38
diff -u -p -r1.38 ahci.c
--- dev/ic/ahci.c   9 Apr 2022 20:10:26 -   1.38
+++ dev/ic/ahci.c   19 Jan 2023 13:30:37 -
@@ -2472,19 +2472,16 @@ ahci_put_err_ccb(struct ahci_ccb *ccb)
 
splassert(IPL_BIO);
 
-#ifdef DIAGNOSTIC
KASSERT(ap->ap_err_busy);
-#endif
+
/* No commands may be active on the chip */
sact = ahci_pread(ap, AHCI_PREG_SACT);
if (sact != 0)
printf("ahci_put_err_ccb but SACT %08x != 0?\n", sact);
KASSERT(ahci_pread(ap, AHCI_PREG_CI) == 0);
 
-#ifdef DIAGNOSTIC
/* Done with the CCB */
KASSERT(ccb == ap->ap_ccb_err);
-#endif
 
/* Restore outstanding command state */
ap->ap_sactive = ap->ap_err_saved_sactive;

Index: dev/wscons/wsemul_sun.c
===
RCS file: /cvs/src/sys/dev/wscons/wsemul_sun.c,v
retrieving revision 1.34
diff -u -p -r1.34 wsemul_sun.c
--- dev/wscons/wsemul_sun.c 25 May 2020 09:55:49 -  1.34
+++ dev/wscons/wsemul_sun.c 19 Jan 2023 13:30:38 -
@@ -242,9 +242,9 @@ wsemul_sun_attach(int console, const str
 
if (console) {
edp = _sun_console_emuldata;
-#ifdef DIAGNOSTIC
+
KASSERT(edp->console == 1);
-#endif
+
} else {
edp = malloc(sizeof *edp, M_DEVBUF, M_NOWAIT);
if (edp == NULL)

Index: net/wg_noise.c
===
RCS file: /cvs/src/sys/net/wg_noise.c,v
retrieving revision 1.5
diff -u -p -r1.5 wg_noise.c
--- net/wg_noise.c  21 Mar 2021 18:13:59 -  1.5
+++ net/wg_noise.c  19 Jan 2023 13:30:38 -
@@ -791,12 +791,10 @@ noise_kdf(uint8_t *a, uint8_t *b, uint8_
uint8_t out[BLAKE2S_HASH_SIZE + 1];
uint8_t sec[BLAKE2S_HASH_SIZE];
 
-#ifdef DIAGNOSTIC
KASSERT(a_len <= BLAKE2S_HASH_SIZE && b_len <= BLAKE2S_HASH_SIZE &&
c_len <= BLAKE2S_HASH_SIZE);
KASSERT(!(b || b_len || c || c_len) || (a && a_len));
KASSERT(!(c || c_len) || (b && b_len));
-#endif
 
/* Extract entropy from "x" into sec */
blake2s_hmac(sec, x, ck, BLAKE2S_HASH_SIZE, x_len, NOISE_HASH_LEN);