[RFA] Move copy_bitwise unittests to own unittest file (was: "Re: [PATCH] Fix buffer overflow in ada-lang.c:move_bits")

Message ID 20181115000158.GC4336@adacore.com
State New, archived
Headers

Commit Message

Joel Brobecker Nov. 15, 2018, 12:01 a.m. UTC
  > > Nit, since the function is now public, I'd consider moving the unit
> > tests to under gdb/unittests/ instead, like, to a new
> > copy_bitwise-selftests.c file.  (I'm mildly thinking that'd be a better
> > filename than utils-selftest.c because the function may well
> > move again in the future.  Notice how gdb_realpath's unit tests
> > were left behind in gdb/utils.c even though gdb_realpath moved to 
> > common/pathstuff.c.)
> > 
> > If you do that, you can drop the
> > '#if GDB_SELF_TEST' around the tests, since files in that
> > directory are not compiled if unit tests are disabled.
> 
> I can do that. Since you said you're file reguardless, it's a little
> easier for me to do it in two stages, so I'll go ahead and push this
> one. I'll start on moving the unit tests again right after, and
> will finish ASAP if it's not finished by end of today.

Here it is:

gdb/ChangeLog:

        * unittests/copy_bitwise-selftests.c: New file.
        * utils.c (selftests::bits_to_str, selftests::check_copy_bitwise)
        (selftests::copy_bitwise_tests): Delete, moving this code to
        unittests/copy_bitwise-selftests.c instead.
        (_initialize_utils): Do not register copy_bitwise tests.
        * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
        unittests/copy_bitwise-selftests.c.

Tested on x86_64-linux using the official testsuite, but also by
verifying that "maintenance selftests" still runs the copy_bitwise
tests.

OK to push to master?

Thank you!
  

Comments

Pedro Alves Nov. 15, 2018, 10:59 a.m. UTC | #1
On 11/15/2018 12:01 AM, Joel Brobecker wrote:

> Here it is:
> 
> gdb/ChangeLog:
> 
>         * unittests/copy_bitwise-selftests.c: New file.
>         * utils.c (selftests::bits_to_str, selftests::check_copy_bitwise)
>         (selftests::copy_bitwise_tests): Delete, moving this code to
>         unittests/copy_bitwise-selftests.c instead.
>         (_initialize_utils): Do not register copy_bitwise tests.
>         * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
>         unittests/copy_bitwise-selftests.c.
> 
> Tested on x86_64-linux using the official testsuite, but also by
> verifying that "maintenance selftests" still runs the copy_bitwise
> tests.
> 
> OK to push to master?
> 

OK.

> Thank you!
Thanks,
Pedro Alves
  
Joel Brobecker Nov. 15, 2018, 3:55 p.m. UTC | #2
> > gdb/ChangeLog:
> > 
> >         * unittests/copy_bitwise-selftests.c: New file.
> >         * utils.c (selftests::bits_to_str, selftests::check_copy_bitwise)
> >         (selftests::copy_bitwise_tests): Delete, moving this code to
> >         unittests/copy_bitwise-selftests.c instead.
> >         (_initialize_utils): Do not register copy_bitwise tests.
> >         * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
> >         unittests/copy_bitwise-selftests.c.
> > 
> > Tested on x86_64-linux using the official testsuite, but also by
> > verifying that "maintenance selftests" still runs the copy_bitwise
> > tests.
> > 
> > OK to push to master?
> > 
> 
> OK.

Thanks Pedro. Pushed to master. And thanks also to Tom, who pointed
out the issue and even tried to fix it; this started us towards
a nice little cleanup. Really nice :)
  

Patch

From b50e765dd1e317e09dee6082d6a9e6729c9f7542 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 14 Nov 2018 18:38:46 -0500
Subject: [PATCH] Move copy_bitwise unittests to own unittest file

Now that copy_bitwise has been made public, and considering that
its implementation could move to a different file again in the future,
this patch moves its unittest to its own file in gdb/unittests.

gdb/ChangeLog:

        * unittests/copy_bitwise-selftests.c: New file.
        * utils.c (selftests::bits_to_str, selftests::check_copy_bitwise)
        (selftests::copy_bitwise_tests): Delete, moving this code to
        unittests/copy_bitwise-selftests.c instead.
        (_initialize_utils): Do not register copy_bitwise tests.
        * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
        unittests/copy_bitwise-selftests.c.

Tested on x86_64-linux using the official testsuite, but also by
verifying that "maintenance selftests" still runs the copy_bitwise
tests.
---
 gdb/Makefile.in                        |   1 +
 gdb/unittests/copy_bitwise-selftests.c | 159 +++++++++++++++++++++++++++++++++
 gdb/utils.c                            | 136 ----------------------------
 3 files changed, 160 insertions(+), 136 deletions(-)
 create mode 100644 gdb/unittests/copy_bitwise-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 6f74911..8bd78a6 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -408,6 +408,7 @@  SUBDIR_UNITTESTS_SRCS = \
 	unittests/array-view-selftests.c \
 	unittests/cli-utils-selftests.c \
 	unittests/common-utils-selftests.c \
+	unittests/copy_bitwise-selftests.c \
 	unittests/environ-selftests.c \
 	unittests/format_pieces-selftests.c \
 	unittests/function-view-selftests.c \
diff --git a/gdb/unittests/copy_bitwise-selftests.c b/gdb/unittests/copy_bitwise-selftests.c
new file mode 100644
index 0000000..af6e5b9
--- /dev/null
+++ b/gdb/unittests/copy_bitwise-selftests.c
@@ -0,0 +1,159 @@ 
+/* Self tests of the copy_bitwise routine for GDB, the GNU debugger.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "selftest.h"
+#include "utils.h"
+
+namespace selftests {
+
+/* Helper function for the unit test of copy_bitwise.  Convert NBITS bits
+   out of BITS, starting at OFFS, to the respective '0'/'1'-string.  MSB0
+   specifies whether to assume big endian bit numbering.  Store the
+   resulting (not null-terminated) string at STR.  */
+
+static void
+bits_to_str (char *str, const gdb_byte *bits, ULONGEST offs,
+	     ULONGEST nbits, int msb0)
+{
+  unsigned int j;
+  size_t i;
+
+  for (i = offs / 8, j = offs % 8; nbits; i++, j = 0)
+    {
+      unsigned int ch = bits[i];
+      for (; j < 8 && nbits; j++, nbits--)
+	*str++ = (ch & (msb0 ? (1 << (7 - j)) : (1 << j))) ? '1' : '0';
+    }
+}
+
+/* Check one invocation of copy_bitwise with the given parameters.  */
+
+static void
+check_copy_bitwise (const gdb_byte *dest, unsigned int dest_offset,
+		    const gdb_byte *source, unsigned int source_offset,
+		    unsigned int nbits, int msb0)
+{
+  size_t len = align_up (dest_offset + nbits, 8);
+  char *expected = (char *) alloca (len + 1);
+  char *actual = (char *) alloca (len + 1);
+  gdb_byte *buf = (gdb_byte *) alloca (len / 8);
+
+  /* Compose a '0'/'1'-string that represents the expected result of
+     copy_bitwise below:
+      Bits from [0, DEST_OFFSET) are filled from DEST.
+      Bits from [DEST_OFFSET, DEST_OFFSET + NBITS) are filled from SOURCE.
+      Bits from [DEST_OFFSET + NBITS, LEN) are filled from DEST.
+
+     E.g., with:
+      dest_offset: 4
+      nbits:       2
+      len:         8
+      dest:        00000000
+      source:      11111111
+
+     We should end up with:
+      buf:         00001100
+                   DDDDSSDD (D=dest, S=source)
+  */
+  bits_to_str (expected, dest, 0, len, msb0);
+  bits_to_str (expected + dest_offset, source, source_offset, nbits, msb0);
+
+  /* Fill BUF with data from DEST, apply copy_bitwise, and convert the
+     result to a '0'/'1'-string.  */
+  memcpy (buf, dest, len / 8);
+  copy_bitwise (buf, dest_offset, source, source_offset, nbits, msb0);
+  bits_to_str (actual, buf, 0, len, msb0);
+
+  /* Compare the resulting strings.  */
+  expected[len] = actual[len] = '\0';
+  if (strcmp (expected, actual) != 0)
+    error (_("copy_bitwise %s != %s (%u+%u -> %u)"),
+	   expected, actual, source_offset, nbits, dest_offset);
+}
+
+/* Unit test for copy_bitwise.  */
+
+static void
+copy_bitwise_tests (void)
+{
+  /* Data to be used as both source and destination buffers.  The two
+     arrays below represent the lsb0- and msb0- encoded versions of the
+     following bit string, respectively:
+       00000000 00011111 11111111 01001000 10100101 11110010
+     This pattern is chosen such that it contains:
+     - constant 0- and 1- chunks of more than a full byte;
+     - 0/1- and 1/0 transitions on all bit positions within a byte;
+     - several sufficiently asymmetric bytes.
+  */
+  static const gdb_byte data_lsb0[] = {
+    0x00, 0xf8, 0xff, 0x12, 0xa5, 0x4f
+  };
+  static const gdb_byte data_msb0[] = {
+    0x00, 0x1f, 0xff, 0x48, 0xa5, 0xf2
+  };
+
+  constexpr size_t data_nbits = 8 * sizeof (data_lsb0);
+  constexpr unsigned max_nbits = 24;
+
+  /* Try all combinations of:
+      lsb0/msb0 bit order (using the respective data array)
+       X [0, MAX_NBITS] copy bit width
+       X feasible source offsets for the given copy bit width
+       X feasible destination offsets
+  */
+  for (int msb0 = 0; msb0 < 2; msb0++)
+    {
+      const gdb_byte *data = msb0 ? data_msb0 : data_lsb0;
+
+      for (unsigned int nbits = 1; nbits <= max_nbits; nbits++)
+	{
+	  const unsigned int max_offset = data_nbits - nbits;
+
+	  for (unsigned source_offset = 0;
+	       source_offset <= max_offset;
+	       source_offset++)
+	    {
+	      for (unsigned dest_offset = 0;
+		   dest_offset <= max_offset;
+		   dest_offset++)
+		{
+		  check_copy_bitwise (data + dest_offset / 8,
+				      dest_offset % 8,
+				      data + source_offset / 8,
+				      source_offset % 8,
+				      nbits, msb0);
+		}
+	    }
+	}
+
+      /* Special cases: copy all, copy nothing.  */
+      check_copy_bitwise (data_lsb0, 0, data_msb0, 0, data_nbits, msb0);
+      check_copy_bitwise (data_msb0, 0, data_lsb0, 0, data_nbits, msb0);
+      check_copy_bitwise (data, data_nbits - 7, data, 9, 0, msb0);
+    }
+}
+
+} /* namespace selftests */
+
+void
+_initialize_copy_bitwise_utils_selftests ()
+{
+  selftests::register_test ("copy_bitwise", selftests::copy_bitwise_tests);
+}
diff --git a/gdb/utils.c b/gdb/utils.c
index c088d8b..0577e64 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3312,141 +3312,6 @@  copy_bitwise (gdb_byte *dest, ULONGEST dest_offset,
     }
 }
 
-#if GDB_SELF_TEST
-
-namespace selftests {
-
-/* Helper function for the unit test of copy_bitwise.  Convert NBITS bits
-   out of BITS, starting at OFFS, to the respective '0'/'1'-string.  MSB0
-   specifies whether to assume big endian bit numbering.  Store the
-   resulting (not null-terminated) string at STR.  */
-
-static void
-bits_to_str (char *str, const gdb_byte *bits, ULONGEST offs,
-	     ULONGEST nbits, int msb0)
-{
-  unsigned int j;
-  size_t i;
-
-  for (i = offs / 8, j = offs % 8; nbits; i++, j = 0)
-    {
-      unsigned int ch = bits[i];
-      for (; j < 8 && nbits; j++, nbits--)
-	*str++ = (ch & (msb0 ? (1 << (7 - j)) : (1 << j))) ? '1' : '0';
-    }
-}
-
-/* Check one invocation of copy_bitwise with the given parameters.  */
-
-static void
-check_copy_bitwise (const gdb_byte *dest, unsigned int dest_offset,
-		    const gdb_byte *source, unsigned int source_offset,
-		    unsigned int nbits, int msb0)
-{
-  size_t len = align_up (dest_offset + nbits, 8);
-  char *expected = (char *) alloca (len + 1);
-  char *actual = (char *) alloca (len + 1);
-  gdb_byte *buf = (gdb_byte *) alloca (len / 8);
-
-  /* Compose a '0'/'1'-string that represents the expected result of
-     copy_bitwise below:
-      Bits from [0, DEST_OFFSET) are filled from DEST.
-      Bits from [DEST_OFFSET, DEST_OFFSET + NBITS) are filled from SOURCE.
-      Bits from [DEST_OFFSET + NBITS, LEN) are filled from DEST.
-
-     E.g., with:
-      dest_offset: 4
-      nbits:       2
-      len:         8
-      dest:        00000000
-      source:      11111111
-
-     We should end up with:
-      buf:         00001100
-                   DDDDSSDD (D=dest, S=source)
-  */
-  bits_to_str (expected, dest, 0, len, msb0);
-  bits_to_str (expected + dest_offset, source, source_offset, nbits, msb0);
-
-  /* Fill BUF with data from DEST, apply copy_bitwise, and convert the
-     result to a '0'/'1'-string.  */
-  memcpy (buf, dest, len / 8);
-  copy_bitwise (buf, dest_offset, source, source_offset, nbits, msb0);
-  bits_to_str (actual, buf, 0, len, msb0);
-
-  /* Compare the resulting strings.  */
-  expected[len] = actual[len] = '\0';
-  if (strcmp (expected, actual) != 0)
-    error (_("copy_bitwise %s != %s (%u+%u -> %u)"),
-	   expected, actual, source_offset, nbits, dest_offset);
-}
-
-/* Unit test for copy_bitwise.  */
-
-static void
-copy_bitwise_tests (void)
-{
-  /* Data to be used as both source and destination buffers.  The two
-     arrays below represent the lsb0- and msb0- encoded versions of the
-     following bit string, respectively:
-       00000000 00011111 11111111 01001000 10100101 11110010
-     This pattern is chosen such that it contains:
-     - constant 0- and 1- chunks of more than a full byte;
-     - 0/1- and 1/0 transitions on all bit positions within a byte;
-     - several sufficiently asymmetric bytes.
-  */
-  static const gdb_byte data_lsb0[] = {
-    0x00, 0xf8, 0xff, 0x12, 0xa5, 0x4f
-  };
-  static const gdb_byte data_msb0[] = {
-    0x00, 0x1f, 0xff, 0x48, 0xa5, 0xf2
-  };
-
-  constexpr size_t data_nbits = 8 * sizeof (data_lsb0);
-  constexpr unsigned max_nbits = 24;
-
-  /* Try all combinations of:
-      lsb0/msb0 bit order (using the respective data array)
-       X [0, MAX_NBITS] copy bit width
-       X feasible source offsets for the given copy bit width
-       X feasible destination offsets
-  */
-  for (int msb0 = 0; msb0 < 2; msb0++)
-    {
-      const gdb_byte *data = msb0 ? data_msb0 : data_lsb0;
-
-      for (unsigned int nbits = 1; nbits <= max_nbits; nbits++)
-	{
-	  const unsigned int max_offset = data_nbits - nbits;
-
-	  for (unsigned source_offset = 0;
-	       source_offset <= max_offset;
-	       source_offset++)
-	    {
-	      for (unsigned dest_offset = 0;
-		   dest_offset <= max_offset;
-		   dest_offset++)
-		{
-		  check_copy_bitwise (data + dest_offset / 8,
-				      dest_offset % 8,
-				      data + source_offset / 8,
-				      source_offset % 8,
-				      nbits, msb0);
-		}
-	    }
-	}
-
-      /* Special cases: copy all, copy nothing.  */
-      check_copy_bitwise (data_lsb0, 0, data_msb0, 0, data_nbits, msb0);
-      check_copy_bitwise (data_msb0, 0, data_lsb0, 0, data_nbits, msb0);
-      check_copy_bitwise (data, data_nbits - 7, data, 9, 0, msb0);
-    }
-}
-
-} /* namespace selftests */
-
-#endif /* GDB_SELF_TEST */
-
 void
 _initialize_utils (void)
 {
@@ -3456,6 +3321,5 @@  _initialize_utils (void)
 
 #if GDB_SELF_TEST
   selftests::register_test ("gdb_realpath", gdb_realpath_tests);
-  selftests::register_test ("copy_bitwise", selftests::copy_bitwise_tests);
 #endif
 }
-- 
2.1.4