Patchwork Fix printing flags field

login
register
mail settings
Submitter Yao Qi
Date March 9, 2018, 4:58 p.m.
Message ID <1520614724-21448-1-git-send-email-yao.qi@linaro.org>
Download mbox | patch
Permalink /patch/26256/
State New
Headers show

Comments

Yao Qi - March 9, 2018, 4:58 p.m.
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(-)
Simon Marchi - March 12, 2018, 12:46 a.m.
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
Simon Marchi - March 13, 2018, 2:18 a.m.
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

Patch

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index c10a498..a156253 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -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
 }
diff --git a/gdb/valprint.c b/gdb/valprint.c
index 3104e0b..8201f7e 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -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;