Commit Message
As reported in PR gdb/22938, the field value in flags is not correct.
It is wrong in bit shift in val_print_type_code_flags, but the field
value is still correct if field_len is 1.
This patch also adds a self test to default_print_one_register_info, to
check that eflags on x86 and cpsr on aarch64 are correctly printed.
Regression tested on x86_64-linux and aarch64-linux.
gdb:
2018-03-09 Yao Qi <yao.qi@linaro.org>
PR gdb/22938
* infcmd.c (test_default_print_one_register_info): New function.
(_initialize_infcmd): Register the unit test.
* valprint.c (val_print_type_code_flags): Fix field value.
---
gdb/infcmd.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
gdb/valprint.c | 3 +--
2 files changed, 81 insertions(+), 2 deletions(-)
Comments
On 2018-03-09 11:58, Yao Qi wrote:
> As reported in PR gdb/22938, the field value in flags is not correct.
> It is wrong in bit shift in val_print_type_code_flags, but the field
> value is still correct if field_len is 1.
>
> This patch also adds a self test to default_print_one_register_info, to
> check that eflags on x86 and cpsr on aarch64 are correctly printed.
>
> Regression tested on x86_64-linux and aarch64-linux.
Hi Yao,
A question that comes to mind is if this works correctly with a
bits_big_endian architecture. The doc for field_location::bitpos says
that it's relative to the MSB on bits_big_endian arches. Therefore, I
would guess that the right shift would be wrong then (just an
assumption, I can't test right now).
Would it be possible to create a selftest that creates its own flags
types so we are not dependent on any arch type? We would have more
control on scenarios/edge cases we want to test, including testing with
bits_big_endian arches.
Simon
On 2018-03-11 08:46 PM, Simon Marchi wrote:
> On 2018-03-09 11:58, Yao Qi wrote:
>> As reported in PR gdb/22938, the field value in flags is not correct.
>> It is wrong in bit shift in val_print_type_code_flags, but the field
>> value is still correct if field_len is 1.
>>
>> This patch also adds a self test to default_print_one_register_info, to
>> check that eflags on x86 and cpsr on aarch64 are correctly printed.
>>
>> Regression tested on x86_64-linux and aarch64-linux.
>
> Hi Yao,
>
> A question that comes to mind is if this works correctly with a
> bits_big_endian architecture. The doc for field_location::bitpos says
> that it's relative to the MSB on bits_big_endian arches. Therefore, I
> would guess that the right shift would be wrong then (just an
> assumption, I can't test right now).
>
> Would it be possible to create a selftest that creates its own flags
> types so we are not dependent on any arch type? We would have more
> control on scenarios/edge cases we want to test, including testing with
> bits_big_endian arches.
>
> Simon
I'm trying to understand how this works, but I can't get a conclusive answer
about how a flag or field would be described on a big endian machine. In the
XML target descriptions, for example, are the start/end fields relative to the
MSB on bits big endian machines and LSB on bits little endian machines? That
would make it like the comment on bitpos:
/* * Position of this field, counting in bits from start of
containing structure. For gdbarch_bits_big_endian=1
targets, it is the bit offset to the MSB. For
gdbarch_bits_big_endian=0 targets, it is the bit offset to
the LSB. */
Reading target-descriptions.c, it's not clear to me. For bitfields in structs,
it seems like the target description always describes relative to the LSB, and
then converts it depending on the endianness (see make_gdb_type_struct).
However, for flag types, make_gdb_type_flags just passes the start as the bit
position. This means that bitpos will be set to whatever was written in the
target description. It would mean that one would have to put the position
relative to the MSB in the target desc. Either there's an inconsistency between
bitfields in structs and flags/fields in flag types, or there's something I
don't understand.
Simon
@@ -2371,6 +2371,81 @@ default_print_one_register_info (struct ui_file *file,
fprintf_filtered (file, "\n");
}
+#if GDB_SELF_TEST
+#include "selftest.h"
+#include "selftest-arch.h"
+
+namespace selftests {
+
+static void
+test_default_print_one_register_info (struct gdbarch *gdbarch)
+{
+ auto bfd_arch = gdbarch_bfd_arch_info (gdbarch)->arch;
+ const int num_regs = (gdbarch_num_regs (gdbarch)
+ + gdbarch_num_pseudo_regs (gdbarch));
+
+ for (auto regnum = 0; regnum < num_regs; regnum++)
+ {
+ if (register_size (gdbarch, regnum) == 0)
+ continue;
+
+ auto t = register_type (gdbarch, regnum);
+ auto name = gdbarch_register_name (gdbarch, regnum);
+ struct value *mark = value_mark ();
+ struct value *v = allocate_value (t);
+
+ VALUE_LVAL (v) = lval_register;
+ VALUE_REGNUM (v) = regnum;
+
+ if (TYPE_CODE (t) == TYPE_CODE_FLAGS)
+ {
+ if (bfd_arch == bfd_arch_i386)
+ {
+ /* Both eflags and mxcsr are 4-byte. */
+ SELF_CHECK (TYPE_LENGTH (t) == 4);
+
+ ULONGEST val = 0x246;
+ store_integer (value_contents_all_raw (v), TYPE_LENGTH (t),
+ gdbarch_byte_order (gdbarch), val);
+
+ string_file file;
+ default_print_one_register_info (&file, name, v);
+
+ if (strcmp (name, "elfags") == 0)
+ {
+ SELF_CHECK (file.string ().find (name) != std::string::npos);
+ SELF_CHECK (file.string ().find ("0x246")
+ != std::string::npos);
+ SELF_CHECK (file.string ().find ("[ PF ZF IF ]")
+ != std::string::npos);
+ }
+ }
+ else if (bfd_arch == bfd_arch_aarch64)
+ {
+ /* cpsr is 4-byte. */
+ SELF_CHECK (TYPE_LENGTH (t) == 4);
+
+ ULONGEST val = 0x800003c9;
+ store_integer (value_contents_all_raw (v), TYPE_LENGTH (t),
+ gdbarch_byte_order (gdbarch), val);
+
+ string_file file;
+ default_print_one_register_info (&file, name, v);
+
+ SELF_CHECK (file.string ().find (name) != std::string::npos);
+ SELF_CHECK (file.string ().find ("0x800003c9")
+ != std::string::npos);
+ SELF_CHECK (file.string ().find ("[ SP EL=2 F I A D N ]")
+ != std::string::npos);
+ }
+ }
+ value_free_to_mark (mark);
+ }
+}
+
+} // namespace selftests
+#endif /* GDB_SELF_TEST */
+
/* Print out the machine register regnum. If regnum is -1, print all
registers (print_all == 1) or all non-float and non-vector
registers (print_all == 0).
@@ -3537,4 +3612,9 @@ List absolute filename for executable of the process."),
add_cmd ("all", class_info, info_proc_cmd_all, _("\
List all available /proc info."),
&info_proc_cmdlist);
+
+#if GDB_SELF_TEST
+ selftests::register_test_foreach_arch ("test_default_print_one_register_info",
+ selftests::test_default_print_one_register_info);
+#endif
}
@@ -1209,8 +1209,7 @@ val_print_type_code_flags (struct type *type, const gdb_byte *valaddr,
else
{
unsigned field_len = TYPE_FIELD_BITSIZE (type, field);
- ULONGEST field_val
- = val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);
+ ULONGEST field_val = val >> TYPE_FIELD_BITPOS (type, field);
if (field_len < sizeof (ULONGEST) * TARGET_CHAR_BIT)
field_val &= ((ULONGEST) 1 << field_len) - 1;