15110 mdb ::printf replicated mdb ::print bitfield mistakes

Reviewed by: Gordon Ross <Gordon.W.Ross@gmail.com>
Reviewed by: Toomas Soome <tsoome@me.com>
Reviewed by: Andy Fiddaman <illumos@fiddaman.net>
Approved by: Dan McDonald <danmcd@mnx.io>

3 files changed, 196 insertions, 29 deletions

diff --git a/usr/src/cmd/mdb/common/mdb/mdb_print.c b/usr/src/cmd/mdb/common/mdb/mdb_print.c
index 4c1c0d2555..52b1b80cd9 100644
--- a/usr/src/cmd/mdb/common/mdb/mdb_print.c
+++ b/usr/src/cmd/mdb/common/mdb/mdb_print.c
@@ -27,7 +27,7 @@
  * Copyright (c) 2012, 2014 by Delphix. All rights reserved.
  * Copyright 2020 Joyent, Inc.
  * Copyright (c) 2014 Nexenta Systems, Inc. All rights reserved.
- * Copyright 2021 Oxide Computer Company
+ * Copyright 2022 Oxide Computer Company
  */
 
 #include <mdb/mdb_modapi.h>
@@ -855,6 +855,28 @@ cmd_array(uintptr_t addr, uint_t flags, int argc, const mdb_arg_t *argv)
 }
 
 /*
+ * This is a shared implementation to determine if we should treat a type as a
+ * bitfield. The parameters are the CTF encoding and the bit offset of the
+ * integer. This also exists in mdb_print.c. We consider something a bitfield
+ * if:
+ *
+ *  o The type is more than 8 bytes. This is a bit of a historical choice from
+ *    mdb and is a stranger one. The normal integer handling code generally
+ *    doesn't handle integers more than 64-bits in size. Of course neither does
+ *    the bitfield code...
+ *  o The bit count is not a multiple of 8.
+ *  o The size in bytes is not a power of 2.
+ *  o The offset is not a multiple of 8.
+ */
+boolean_t
+is_bitfield(const ctf_encoding_t *ep, ulong_t off)
+{
+	size_t bsize = ep->cte_bits / NBBY;
+	return (bsize > 8 || (ep->cte_bits % NBBY) != 0 ||
+	    (bsize & (bsize - 1)) != 0 || (off % NBBY) != 0);
+}
+
+/*
  * Print an integer bitfield in hexadecimal by reading the enclosing byte(s)
  * and then shifting and masking the data in the lower bits of a uint64_t.
  */
@@ -862,14 +884,21 @@ static int
 print_bitfield(ulong_t off, printarg_t *pap, ctf_encoding_t *ep)
 {
 	mdb_tgt_addr_t addr = pap->pa_addr + off / NBBY;
-	size_t size = (ep->cte_bits + (NBBY - 1)) / NBBY;
 	uint64_t mask = (1ULL << ep->cte_bits) - 1;
 	uint64_t value = 0;
 	uint8_t *buf = (uint8_t *)&value;
 	uint8_t shift;
-
 	const char *format;
 
+	/*
+	 * Our bitfield may straddle a byte boundary. We explicitly take the
+	 * offset of the bitfield within its byte into account when determining
+	 * the overall amount of data to copy and mask off from the underlying
+	 * data.
+	 */
+	uint_t nbits = ep->cte_bits + (off % NBBY);
+	size_t size = P2ROUNDUP(nbits, NBBY) / NBBY;
+
 	if (!(pap->pa_flags & PA_SHOWVAL))
 		return (0);
 
@@ -878,17 +907,9 @@ print_bitfield(ulong_t off, printarg_t *pap, ctf_encoding_t *ep)
 		return (0);
 	}
 
-	/*
-	 * Our bitfield may stradle a byte boundary, if so, the calculation of
-	 * size may not correctly capture that. However, off is relative to the
-	 * entire bitfield, so we first have to make that relative to the byte.
-	 */
-	if ((off % 8) + ep->cte_bits > NBBY * size) {
-		size++;
-	}
-
 	if (size > sizeof (value)) {
 		mdb_printf("??? (total bitfield too large after alignment");
+		return (0);
 	}
 
 	/*
@@ -1002,14 +1023,8 @@ print_int_val(const char *type, ctf_encoding_t *ep, ulong_t off,
 		return (0);
 	}
 
-	/*
-	 * If the size is not a power-of-two number of bytes in the range 1-8 or
-	 * power-of-two number starts in the middle of a byte then we assume it
-	 * is a bitfield and print it as such.
-	 */
 	size = ep->cte_bits / NBBY;
-	if (size > 8 || (ep->cte_bits % NBBY) != 0 || (size & (size - 1) ||
-	    (off % NBBY) != 0) != 0) {
+	if (is_bitfield(ep, off)) {
 		return (print_bitfield(off, pap, ep));
 	}
 
@@ -1764,7 +1779,7 @@ static int
 pipe_print(mdb_ctf_id_t id, ulong_t off, void *data)
 {
 	printarg_t *pap = data;
-	ssize_t size;
+	size_t size;
 	static const char *const fsp[] = { "%#r", "%#r", "%#r", "%#llr" };
 	uintptr_t value;
 	uintptr_t addr = pap->pa_addr + off / NBBY;
@@ -1823,13 +1838,12 @@ again:
 		 * For immediate values, we just print out the value.
 		 */
 		size = e.cte_bits / NBBY;
-		if (size > 8 || (e.cte_bits % NBBY) != 0 ||
-		    (size & (size - 1)) != 0) {
+		if (is_bitfield(&e, off)) {
 			return (print_bitfield(off, pap, &e));
 		}
 
 		if (mdb_tgt_aread(pap->pa_tgt, pap->pa_as, &u.i8, size,
-		    addr) != size) {
+		    addr) != (size_t)size) {
 			mdb_warn("failed to read %lu bytes at %p",
 			    (ulong_t)size, pap->pa_addr);
 			return (-1);
@@ -2578,7 +2592,7 @@ static int
 printf_signed(mdb_ctf_id_t id, uintptr_t addr, ulong_t off, char *fmt,
     boolean_t sign)
 {
-	ssize_t size;
+	size_t size;
 	mdb_ctf_id_t base;
 	ctf_encoding_t e;
 
@@ -2627,23 +2641,43 @@ printf_signed(mdb_ctf_id_t id, uintptr_t addr, ulong_t off, char *fmt,
 	 * particular, see the comments there for an explanation of the
 	 * endianness differences in this code.)
 	 */
-	if (size > 8 || (e.cte_bits % NBBY) != 0 ||
-	    (size & (size - 1)) != 0) {
+	if (is_bitfield(&e, off)) {
 		uint64_t mask = (1ULL << e.cte_bits) - 1;
 		uint64_t value = 0;
 		uint8_t *buf = (uint8_t *)&value;
 		uint8_t shift;
+		uint_t nbits;
 
 		/*
-		 * Round our size up one byte.
+		 * Our bitfield may straddle a byte boundary. We explicitly take
+		 * the offset of the bitfield within its byte into account when
+		 * determining the overall amount of data to copy and mask off
+		 * from the underlying data.
 		 */
-		size = (e.cte_bits + (NBBY - 1)) / NBBY;
+		nbits = e.cte_bits + (off % NBBY);
+		size = P2ROUNDUP(nbits, NBBY) / NBBY;
 
 		if (e.cte_bits > sizeof (value) * NBBY - 1) {
 			mdb_printf("invalid bitfield size %u", e.cte_bits);
 			return (DCMD_ABORT);
 		}
 
+		/*
+		 * Our bitfield may straddle a byte boundary, if so, the
+		 * calculation of size may not correctly capture that. However,
+		 * off is relative to the entire bitfield, so we first have to
+		 * make that relative to the byte.
+		 */
+		if ((off % NBBY) + e.cte_bits > NBBY * size) {
+			size++;
+		}
+
+		if (size > sizeof (value)) {
+			mdb_warn("??? (total bitfield too large after "
+			    "alignment\n");
+			return (DCMD_ABORT);
+		}
+
 #ifdef _BIG_ENDIAN
 		buf += sizeof (value) - size;
 		off += e.cte_bits;
diff --git a/usr/src/test/util-tests/tests/mdb/numbers/tst.bitfields.ksh b/usr/src/test/util-tests/tests/mdb/numbers/tst.bitfields.ksh
index 3f39acae92..971c798053 100644
--- a/usr/src/test/util-tests/tests/mdb/numbers/tst.bitfields.ksh
+++ b/usr/src/test/util-tests/tests/mdb/numbers/tst.bitfields.ksh
@@ -12,7 +12,7 @@
 #
 
 #
-# Copyright 2021 Oxide Computer Company
+# Copyright 2022 Oxide Computer Company
 #
 
 #
@@ -26,5 +26,81 @@ tst_prog="$tst_root/progs/bitfields"
 tst_outfile="/tmp/mdb.bitfield.out.$$"
 tst_exp="$0.out"
 
+#
+# Top level ::print
+#
 $MDB -e "first::print -t broken_t" $tst_prog > $ODIR/stdout
 $MDB -e "second::print -t broken6491_t" $tst_prog >> $ODIR/stdout
+
+#
+# ::print of specific members
+#
+$MDB -e "first::print broken_t brk_a" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_b" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_c" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_d" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_e" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_f" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_g" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_h" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_i" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_j" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_k" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_l" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_m" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t a" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t b" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t c" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t d" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t e" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t f" $tst_prog >> $ODIR/stdout
+
+#
+# ::printf of members. Note, if ::printf said '%x\n' below then we would
+# include the string "\n" (not a newline) in the output. Instead we rely
+# upon the implicit newline from mdb -e.
+#
+$MDB -e "first::printf '%x' broken_t brk_a" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_b" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_c" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_d" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_e" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_f" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_g" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_h" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_i" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_j" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_k" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_l" $tst_prog >> $ODIR/stdout
+$MDB -e "first::printf '%x' broken_t brk_m" $tst_prog >> $ODIR/stdout
+$MDB -e "second::printf '%x' broken6491_t a" $tst_prog >> $ODIR/stdout
+$MDB -e "second::printf '%x' broken6491_t b" $tst_prog >> $ODIR/stdout
+$MDB -e "second::printf '%x' broken6491_t c" $tst_prog >> $ODIR/stdout
+$MDB -e "second::printf '%x' broken6491_t d" $tst_prog >> $ODIR/stdout
+$MDB -e "second::printf '%x' broken6491_t e" $tst_prog >> $ODIR/stdout
+$MDB -e "second::printf '%x' broken6491_t f" $tst_prog >> $ODIR/stdout
+
+#
+# If we pipe the output of ::print that is a different bitfield logic
+# path. So we take that all to a `::eval '.=K'` as a basic way to get it
+# out.
+#
+$MDB -e "first::print broken_t brk_a | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_b | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_c | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_d | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_e | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_f | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_g | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_h | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_i | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_j | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_k | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_l | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "first::print broken_t brk_m | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t a | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t b | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t c | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t d | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t e | ::eval '.=K'" $tst_prog >> $ODIR/stdout
+$MDB -e "second::print broken6491_t f | ::eval '.=K'" $tst_prog >> $ODIR/stdout
diff --git a/usr/src/test/util-tests/tests/mdb/numbers/tst.bitfields.ksh.out b/usr/src/test/util-tests/tests/mdb/numbers/tst.bitfields.ksh.out
index 347415a638..b71fd45c60 100644
--- a/usr/src/test/util-tests/tests/mdb/numbers/tst.bitfields.ksh.out
+++ b/usr/src/test/util-tests/tests/mdb/numbers/tst.bitfields.ksh.out
@@ -21,3 +21,60 @@ broken6491_t {
     unsigned short:1 e :1 = 0x1
     unsigned short:1 f :1 = 0
 }
+brk_a = 0x3
+brk_b = 0x3
+brk_c = 0
+brk_d = 0x1
+brk_e = 0x1
+brk_f = 0x1
+brk_g = 0x3
+brk_h = 0x5
+brk_i = 0x3
+brk_j = 0x9
+brk_k = 0x13
+brk_l = 0
+brk_m = 0x1
+a = 0x1
+b = 0x2
+c = 0
+d = 0
+e = 0x1
+f = 0
+3
+3
+0
+1
+1
+1
+3
+5
+3
+9
+13
+0
+1
+1
+2
+0
+0
+1
+0
+                3               
+                3               
+                0               
+                1               
+                1               
+                1               
+                3               
+                5               
+                3               
+                9               
+                13              
+                0               
+                1               
+                1               
+                2               
+                0               
+                0               
+                1               
+                0