[PATCH v2] gdb: Fix numerical field extraction for target description "flags"

[PATCH v2] gdb: Fix numerical field extraction for target description "flags"

[Shahab Vahedi]

From: Shahab Vahedi <shahab@synopsys.com>

v2 (This section will be removed when checking the patch in):
1. There are no lines in the commit message starting with "---".
2. Joined 2 lines together that now fit under character limits.
3. Added the unit-test "test_print_flags" as proposed by Simon.

The "val_print_type_code_flags ()" function is responsible for
extraction of fields for "flags" data type.  These data types are
used when describing a custom register type in a target description
XML.  The logic used for the extraction though is not sound:

    unsigned field_len = TYPE_FIELD_BITSIZE (type, field);
    ULONGEST field_val
      = val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);

TYPE_FIELD_BITSIZE: The bit length of the field to be extracted.
TYPE_FIELD_BITPOS:  The starting position of the field; 0 is LSB.
val:                The register value.

Imagine you have a field that starts at position 1 and its length
is 4 bits.  According to the third line of the code snippet the
shifting right would become "val >> -2", or "val >> 0xfff...fe"
to be precise.  That will result in a "field_val" of 0.

The correct extraction should be:

    ULONGEST field_val = val >> TYPE_FIELD_BITPOS (type, field);

The rest of the algorithm that masks out the higher bits is OK.

Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
---
 gdb/valprint.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/gdb/valprint.c b/gdb/valprint.c
index fa2b64ef10a..324055da93f 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -43,6 +43,8 @@
 #include "c-lang.h"
 #include "cp-abi.h"
 #include "inferior.h"
+#include "gdbsupport/selftest.h"
+#include "selftest-arch.h"
 
 /* Maximum number of wchars returned from wchar_iterate.  */
 #define MAX_WCHARS 4
@@ -1221,8 +1223,7 @@ val_print_type_code_flags (struct type *type, struct value *original_value,
 	  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;
@@ -3137,10 +3138,41 @@ make_value_print_options_def_group (value_print_options *opts)
   return {{value_print_option_defs}, opts};
 }
 
+#if GDB_SELF_TEST
+
+/* Test printing of TYPE_CODE_FLAGS values.  */
+
+static void
+test_print_flags (gdbarch *arch)
+{
+  type *flags_type = arch_flags_type (arch, "test_type", 32);
+  type *field_type = builtin_type (arch)->builtin_uint32;
+
+  /* Value:  1010 1010
+     Fields: CCCB BAAA */
+  append_flags_type_field (flags_type, 0, 3, field_type, "A");
+  append_flags_type_field (flags_type, 3, 2, field_type, "B");
+  append_flags_type_field (flags_type, 5, 3, field_type, "C");
+
+  value *val = allocate_value (flags_type);
+  gdb_byte *contents = value_contents_writeable (val);
+  store_unsigned_integer (contents, 4, gdbarch_byte_order (arch), 0xaa);
+
+  string_file out;
+  val_print_type_code_flags (flags_type, val, 0, &out);
+  SELF_CHECK (out.string () == "[ A=2 B=1 C=5 ]");
+}
+
+#endif
+
 void _initialize_valprint ();
 void
 _initialize_valprint ()
 {
+#if GDB_SELF_TEST
+  selftests::register_test_foreach_arch ("print-flags", test_print_flags);
+#endif
+
   cmd_list_element *cmd;
 
   cmd_list_element *set_print_cmd
-- 
2.32.0

Re: [PATCH v2] gdb: Fix numerical field extraction for target description "flags"

[Simon Marchi]

On 2021-07-23 8:38 a.m., Shahab Vahedi via Gdb-patches wrote:
> From: Shahab Vahedi <shahab@synopsys.com>
> 
> v2 (This section will be removed when checking the patch in):
> 1. There are no lines in the commit message starting with "---".
> 2. Joined 2 lines together that now fit under character limits.
> 3. Added the unit-test "test_print_flags" as proposed by Simon.
> 
> The "val_print_type_code_flags ()" function is responsible for
> extraction of fields for "flags" data type.  These data types are
> used when describing a custom register type in a target description
> XML.  The logic used for the extraction though is not sound:
> 
>     unsigned field_len = TYPE_FIELD_BITSIZE (type, field);
>     ULONGEST field_val
>       = val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);
> 
> TYPE_FIELD_BITSIZE: The bit length of the field to be extracted.
> TYPE_FIELD_BITPOS:  The starting position of the field; 0 is LSB.
> val:                The register value.
> 
> Imagine you have a field that starts at position 1 and its length
> is 4 bits.  According to the third line of the code snippet the
> shifting right would become "val >> -2", or "val >> 0xfff...fe"
> to be precise.  That will result in a "field_val" of 0.
> 
> The correct extraction should be:
> 
>     ULONGEST field_val = val >> TYPE_FIELD_BITPOS (type, field);
> 
> The rest of the algorithm that masks out the higher bits is OK.
> 
> Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>

LGTM, thanks.

Simon

Re: [PATCH v2] gdb: Fix numerical field extraction for target description "flags"

[Simon Marchi]

Hi Joel,

Shahab expressed (on IRC) the desire to merge this patch in the GDB 11
branch.  That sounds reasonable to me, does it to you?

Simon

On 2021-07-23 9:26 a.m., Simon Marchi via Gdb-patches wrote:
> On 2021-07-23 8:38 a.m., Shahab Vahedi via Gdb-patches wrote:
>> From: Shahab Vahedi <shahab@synopsys.com>
>>
>> v2 (This section will be removed when checking the patch in):
>> 1. There are no lines in the commit message starting with "---".
>> 2. Joined 2 lines together that now fit under character limits.
>> 3. Added the unit-test "test_print_flags" as proposed by Simon.
>>
>> The "val_print_type_code_flags ()" function is responsible for
>> extraction of fields for "flags" data type.  These data types are
>> used when describing a custom register type in a target description
>> XML.  The logic used for the extraction though is not sound:
>>
>>     unsigned field_len = TYPE_FIELD_BITSIZE (type, field);
>>     ULONGEST field_val
>>       = val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);
>>
>> TYPE_FIELD_BITSIZE: The bit length of the field to be extracted.
>> TYPE_FIELD_BITPOS:  The starting position of the field; 0 is LSB.
>> val:                The register value.
>>
>> Imagine you have a field that starts at position 1 and its length
>> is 4 bits.  According to the third line of the code snippet the
>> shifting right would become "val >> -2", or "val >> 0xfff...fe"
>> to be precise.  That will result in a "field_val" of 0.
>>
>> The correct extraction should be:
>>
>>     ULONGEST field_val = val >> TYPE_FIELD_BITPOS (type, field);
>>
>> The rest of the algorithm that masks out the higher bits is OK.
>>
>> Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
> 
> LGTM, thanks.
> 
> Simon
> 

Re: [PATCH v2] gdb: Fix numerical field extraction for target description "flags"

[Joel Brobecker]

> Shahab expressed (on IRC) the desire to merge this patch in the GDB 11
> branch.  That sounds reasonable to me, does it to you?

It does to me too. Thanks Simon and Shahab.

> On 2021-07-23 9:26 a.m., Simon Marchi via Gdb-patches wrote:
> > On 2021-07-23 8:38 a.m., Shahab Vahedi via Gdb-patches wrote:
> >> From: Shahab Vahedi <shahab@synopsys.com>
> >>
> >> v2 (This section will be removed when checking the patch in):
> >> 1. There are no lines in the commit message starting with "---".
> >> 2. Joined 2 lines together that now fit under character limits.
> >> 3. Added the unit-test "test_print_flags" as proposed by Simon.
> >>
> >> The "val_print_type_code_flags ()" function is responsible for
> >> extraction of fields for "flags" data type.  These data types are
> >> used when describing a custom register type in a target description
> >> XML.  The logic used for the extraction though is not sound:
> >>
> >>     unsigned field_len = TYPE_FIELD_BITSIZE (type, field);
> >>     ULONGEST field_val
> >>       = val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);
> >>
> >> TYPE_FIELD_BITSIZE: The bit length of the field to be extracted.
> >> TYPE_FIELD_BITPOS:  The starting position of the field; 0 is LSB.
> >> val:                The register value.
> >>
> >> Imagine you have a field that starts at position 1 and its length
> >> is 4 bits.  According to the third line of the code snippet the
> >> shifting right would become "val >> -2", or "val >> 0xfff...fe"
> >> to be precise.  That will result in a "field_val" of 0.
> >>
> >> The correct extraction should be:
> >>
> >>     ULONGEST field_val = val >> TYPE_FIELD_BITPOS (type, field);
> >>
> >> The rest of the algorithm that masks out the higher bits is OK.
> >>
> >> Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
> > 
> > LGTM, thanks.
> > 
> > Simon
> > 

-- 
Joel

Re: [PATCH v2] gdb: Fix numerical field extraction for target description "flags"

[Shahab Vahedi]

On 7/24/21 3:50 PM, Joel Brobecker wrote:
>> Shahab expressed (on IRC) the desire to merge this patch in the GDB 11
>> branch.  That sounds reasonable to me, does it to you?
> 
> It does to me too. Thanks Simon and Shahab.

Could either of you set the "Target Milestone" [1] to gdb-11 then? Thanks!


[1] Incorrect extraction of integer fields in target description "flags" data type 
https://sourceware.org/bugzilla/show_bug.cgi?id=28103


-- 
Shahab

Re: [PATCH v2] gdb: Fix numerical field extraction for target description "flags"

[Simon Marchi]

On 2021-07-26 9:33 a.m., Shahab Vahedi wrote:
> On 7/24/21 3:50 PM, Joel Brobecker wrote:
>>> Shahab expressed (on IRC) the desire to merge this patch in the GDB 11
>>> branch.  That sounds reasonable to me, does it to you?
>>
>> It does to me too. Thanks Simon and Shahab.
> 
> Could either of you set the "Target Milestone" [1] to gdb-11 then? Thanks!
> 
> 
> [1] Incorrect extraction of integer fields in target description "flags" data type 
> https://sourceware.org/bugzilla/show_bug.cgi?id=28103
> 
> 

Done.

[PUSHED master] gdb: Fix numerical field extraction for target description "flags"

[Shahab Vahedi]

From: Shahab Vahedi <shahab@synopsys.com>

The "val_print_type_code_flags ()" function is responsible for
extraction of fields for "flags" data type.  These data types are
used when describing a custom register type in a target description
XML.  The logic used for the extraction though is not sound:

    unsigned field_len = TYPE_FIELD_BITSIZE (type, field);
    ULONGEST field_val
      = val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);

TYPE_FIELD_BITSIZE: The bit length of the field to be extracted.
TYPE_FIELD_BITPOS:  The starting position of the field; 0 is LSB.
val:                The register value.

Imagine you have a field that starts at position 1 and its length
is 4 bits.  According to the third line of the code snippet the
shifting right would become "val >> -2", or "val >> 0xfff...fe"
to be precise.  That will result in a "field_val" of 0.

The correct extraction should be:

    ULONGEST field_val = val >> TYPE_FIELD_BITPOS (type, field);

The rest of the algorithm that masks out the higher bits is OK.

Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
---
 gdb/valprint.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/gdb/valprint.c b/gdb/valprint.c
index fa2b64ef10a..324055da93f 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -43,6 +43,8 @@
 #include "c-lang.h"
 #include "cp-abi.h"
 #include "inferior.h"
+#include "gdbsupport/selftest.h"
+#include "selftest-arch.h"
 
 /* Maximum number of wchars returned from wchar_iterate.  */
 #define MAX_WCHARS 4
@@ -1221,8 +1223,7 @@ val_print_type_code_flags (struct type *type, struct value *original_value,
 	  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;
@@ -3137,10 +3138,41 @@ make_value_print_options_def_group (value_print_options *opts)
   return {{value_print_option_defs}, opts};
 }
 
+#if GDB_SELF_TEST
+
+/* Test printing of TYPE_CODE_FLAGS values.  */
+
+static void
+test_print_flags (gdbarch *arch)
+{
+  type *flags_type = arch_flags_type (arch, "test_type", 32);
+  type *field_type = builtin_type (arch)->builtin_uint32;
+
+  /* Value:  1010 1010
+     Fields: CCCB BAAA */
+  append_flags_type_field (flags_type, 0, 3, field_type, "A");
+  append_flags_type_field (flags_type, 3, 2, field_type, "B");
+  append_flags_type_field (flags_type, 5, 3, field_type, "C");
+
+  value *val = allocate_value (flags_type);
+  gdb_byte *contents = value_contents_writeable (val);
+  store_unsigned_integer (contents, 4, gdbarch_byte_order (arch), 0xaa);
+
+  string_file out;
+  val_print_type_code_flags (flags_type, val, 0, &out);
+  SELF_CHECK (out.string () == "[ A=2 B=1 C=5 ]");
+}
+
+#endif
+
 void _initialize_valprint ();
 void
 _initialize_valprint ()
 {
+#if GDB_SELF_TEST
+  selftests::register_test_foreach_arch ("print-flags", test_print_flags);
+#endif
+
   cmd_list_element *cmd;
 
   cmd_list_element *set_print_cmd
-- 
2.32.0

[PUSHED gdb-11-branch] gdb: Fix numerical field extraction for target description "flags"

[Shahab Vahedi]

From: Shahab Vahedi <shahab@synopsys.com>

The "val_print_type_code_flags ()" function is responsible for
extraction of fields for "flags" data type.  These data types are
used when describing a custom register type in a target description
XML.  The logic used for the extraction though is not sound:

    unsigned field_len = TYPE_FIELD_BITSIZE (type, field);
    ULONGEST field_val
      = val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);

TYPE_FIELD_BITSIZE: The bit length of the field to be extracted.
TYPE_FIELD_BITPOS:  The starting position of the field; 0 is LSB.
val:                The register value.

Imagine you have a field that starts at position 1 and its length
is 4 bits.  According to the third line of the code snippet the
shifting right would become "val >> -2", or "val >> 0xfff...fe"
to be precise.  That will result in a "field_val" of 0.

The correct extraction should be:

    ULONGEST field_val = val >> TYPE_FIELD_BITPOS (type, field);

The rest of the algorithm that masks out the higher bits is OK.

gdb/ChangeLog:
2021-07-26  Shahab Vahedi  <shahab@synopsys.com>
            Simon Marchi  <simon.marchi@efficios.com>

	PR gdb/28103
	* valprint.c (val_print_type_code_flags): Merely shift the VAL
	to the right to get rid of the lower bits.
	(test_print_flags): New.
	(_initialize_valprint): Invoke the "test_print_flags" as a unit-test.

Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
---
 gdb/ChangeLog  |  9 +++++++++
 gdb/valprint.c | 36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2923c592a2f..7b6136574ef 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2021-07-26  Shahab Vahedi  <shahab@synopsys.com>
+	    Simon Marchi  <simon.marchi@efficios.com>
+
+	PR gdb/28103
+	* valprint.c (val_print_type_code_flags): Merely shift the VAL
+	to the right to get rid of the lower bits.
+	(test_print_flags): New.
+	(_initialize_valprint): Invoke the "test_print_flags" as a unit-test.
+
 2021-07-26  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
 
 	PR gdb/28076
diff --git a/gdb/valprint.c b/gdb/valprint.c
index fa2b64ef10a..324055da93f 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -43,6 +43,8 @@
 #include "c-lang.h"
 #include "cp-abi.h"
 #include "inferior.h"
+#include "gdbsupport/selftest.h"
+#include "selftest-arch.h"
 
 /* Maximum number of wchars returned from wchar_iterate.  */
 #define MAX_WCHARS 4
@@ -1221,8 +1223,7 @@ val_print_type_code_flags (struct type *type, struct value *original_value,
 	  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;
@@ -3137,10 +3138,41 @@ make_value_print_options_def_group (value_print_options *opts)
   return {{value_print_option_defs}, opts};
 }
 
+#if GDB_SELF_TEST
+
+/* Test printing of TYPE_CODE_FLAGS values.  */
+
+static void
+test_print_flags (gdbarch *arch)
+{
+  type *flags_type = arch_flags_type (arch, "test_type", 32);
+  type *field_type = builtin_type (arch)->builtin_uint32;
+
+  /* Value:  1010 1010
+     Fields: CCCB BAAA */
+  append_flags_type_field (flags_type, 0, 3, field_type, "A");
+  append_flags_type_field (flags_type, 3, 2, field_type, "B");
+  append_flags_type_field (flags_type, 5, 3, field_type, "C");
+
+  value *val = allocate_value (flags_type);
+  gdb_byte *contents = value_contents_writeable (val);
+  store_unsigned_integer (contents, 4, gdbarch_byte_order (arch), 0xaa);
+
+  string_file out;
+  val_print_type_code_flags (flags_type, val, 0, &out);
+  SELF_CHECK (out.string () == "[ A=2 B=1 C=5 ]");
+}
+
+#endif
+
 void _initialize_valprint ();
 void
 _initialize_valprint ()
 {
+#if GDB_SELF_TEST
+  selftests::register_test_foreach_arch ("print-flags", test_print_flags);
+#endif
+
   cmd_list_element *cmd;
 
   cmd_list_element *set_print_cmd
-- 
2.32.0