[RFA,v2,1/4] Sign-extend non-bit-fields in unpack_bits_as_long
Commit Message
unpack_bits_as_long is documented as sign-extending its result when
the type is signed. However, it was only doing sign-extension in the
case where the field was a bitfield -- that is, not when the "bitsize"
parameter was 0, indicating the size should be taken from the type.
Also, unpack_bits_as_long was incorrectly computing the shift for
big-endian architectures for the non-bitfield case.
This patch fixes these bugs in a straightforward way. A new selftest
is included.
2018-02-22 Tom Tromey <tom@tromey.com>
* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
unittests/unpack-selftests.c.
* unittests/unpack-selftests.c: New file.
* value.c (unpack_bits_as_long): Fix bugs in non-bitfield cases.
---
gdb/ChangeLog | 7 +++++
gdb/Makefile.in | 1 +
gdb/unittests/unpack-selftests.c | 61 ++++++++++++++++++++++++++++++++++++++++
gdb/value.c | 7 +++--
4 files changed, 74 insertions(+), 2 deletions(-)
create mode 100644 gdb/unittests/unpack-selftests.c
Comments
Hi Tom,
On Thu, Feb 22, 2018 at 01:30:15PM -0700, Tom Tromey wrote:
> unpack_bits_as_long is documented as sign-extending its result when
> the type is signed. However, it was only doing sign-extension in the
> case where the field was a bitfield -- that is, not when the "bitsize"
> parameter was 0, indicating the size should be taken from the type.
>
> Also, unpack_bits_as_long was incorrectly computing the shift for
> big-endian architectures for the non-bitfield case.
>
> This patch fixes these bugs in a straightforward way. A new selftest
> is included.
>
> 2018-02-22 Tom Tromey <tom@tromey.com>
>
> * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
> unittests/unpack-selftests.c.
> * unittests/unpack-selftests.c: New file.
> * value.c (unpack_bits_as_long): Fix bugs in non-bitfield cases.
Looks good to me. Just one thing: I think we need to adjust
unpack_bits_as_long's documentation a bit to match the reality of
how this function is called:
/* Unpack a bitfield of the specified FIELD_TYPE, from the object at
VALADDR, and store the result in *RESULT.
The bitfield starts at BITPOS bits and contains BITSIZE bits.
I checked the callers, and none of them really make a distinction between
bitsize = 0 and bitsize != 0. And since it's fairly straightforward to
handlet this case directly in unpack_bits_as_long, I agree this is best.
Pre-approved with this change.
Thanks for adding a self-test!
@@ -1,3 +1,10 @@
+2018-02-22 Tom Tromey <tom@tromey.com>
+
+ * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
+ unittests/unpack-selftests.c.
+ * unittests/unpack-selftests.c: New file.
+ * value.c (unpack_bits_as_long): Fix bugs in non-bitfield cases.
+
2018-02-21 John Baldwin <jhb@FreeBSD.org>
* arch/aarch64.c: Include "common-defs.h".
@@ -427,6 +427,7 @@ SUBDIR_UNITTESTS_SRCS = \
unittests/scoped_fd-selftests.c \
unittests/scoped_mmap-selftests.c \
unittests/scoped_restore-selftests.c \
+ unittests/unpack-selftests.c \
unittests/xml-utils-selftests.c
SUBDIR_UNITTESTS_OBS = $(patsubst %.c,%.o,$(SUBDIR_UNITTESTS_SRCS))
new file mode 100644
@@ -0,0 +1,61 @@
+/* Self tests for unpack_field_as_long
+
+ 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 "selftest-arch.h"
+#include "value.h"
+#include "gdbtypes.h"
+#include "arch-utils.h"
+
+namespace selftests {
+namespace unpack {
+
+static void
+unpack_field_as_long_tests (struct gdbarch *arch)
+{
+ gdb_byte buffer[8];
+ const struct builtin_type *bt = builtin_type (arch);
+ struct type *struct_type = arch_composite_type (arch, "<<selftest>>",
+ TYPE_CODE_STRUCT);
+
+ append_composite_type_field (struct_type, "field0", bt->builtin_int8);
+ append_composite_type_field_aligned (struct_type, "field1",
+ bt->builtin_uint32, 4);
+
+ memset (buffer, 0, sizeof (buffer));
+ buffer[0] = 255;
+ if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG)
+ buffer[7] = 23;
+ else
+ buffer[4] = 23;
+
+ SELF_CHECK (unpack_field_as_long (struct_type, buffer, 0) == -1);
+ SELF_CHECK (unpack_field_as_long (struct_type, buffer, 1) == 23);
+}
+
+}
+}
+
+void
+_initialize_unpack_selftests ()
+{
+ selftests::register_test_foreach_arch
+ ("unpack_field_as_long", selftests::unpack::unpack_field_as_long_tests);
+}
@@ -3244,7 +3244,10 @@ unpack_bits_as_long (struct type *field_type, const gdb_byte *valaddr,
if (bitsize)
bytes_read = ((bitpos % 8) + bitsize + 7) / 8;
else
- bytes_read = TYPE_LENGTH (field_type);
+ {
+ bytes_read = TYPE_LENGTH (field_type);
+ bitsize = 8 * bytes_read;
+ }
read_offset = bitpos / 8;
@@ -3262,7 +3265,7 @@ unpack_bits_as_long (struct type *field_type, const gdb_byte *valaddr,
/* If the field does not entirely fill a LONGEST, then zero the sign bits.
If the field is signed, and is negative, then sign extend. */
- if ((bitsize > 0) && (bitsize < 8 * (int) sizeof (val)))
+ if (bitsize < 8 * (int) sizeof (val))
{
valmask = (((ULONGEST) 1) << bitsize) - 1;
val &= valmask;