gdb: Fix numerical field extraction for target description "flags"

Message ID 20210719162945.23261-1-shahab.vahedi@gmail.com
State New
Headers show
Series
  • gdb: Fix numerical field extraction for target description "flags"
Related show

Commit Message

Simon Marchi via Gdb-patches July 19, 2021, 4:29 p.m.
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:

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

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:
------------------ 8< ------------------
  ULONGEST field_val = val >> TYPE_FIELD_BITPOS (type, field);
------------------ >8 ------------------

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

gdb/ChangeLog:

	PR gdb/28103
	* valprint.c (val_print_type_code_flags): Merely shift the
	VAL to the right to get rid of the lower bits.
---
 gdb/valprint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.32.0

Comments

Simon Marchi via Gdb-patches July 22, 2021, 7:21 p.m. | #1
On 2021-07-19 12:29 p.m., Shahab Vahedi via Gdb-patches wrote:
> 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:

> 

> ------------------ 8< ------------------

>   unsigned field_len = TYPE_FIELD_BITSIZE (type, field);

>   ULONGEST field_val

>     = val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);

> ------------------ >8 ------------------

> 

> 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:

> ------------------ 8< ------------------

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

> ------------------ >8 ------------------


Please avoid lines starting with "---".  You can just remove those
"8<" lines.  Indent the code blocks with four spaces, and it will be
clear (and even formatted with a monospace font on some platforms like
Github that render commit messages as markdown).

The reason is that when git-am-ing your patch, git drops everything
above that line from the commit message (you can try it for yourself if
you want to see).

> 

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


We will want a test for this.  I tried to write a selftest, it seems to
work nicely.  I will put it at the end of this message, you can
integrate it in your patch or modify it as you see fit.  If you do use
it, you can add this git trailer, as per our new procedures:

Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>

> 

> gdb/ChangeLog:

> 

> 	PR gdb/28103

> 	* valprint.c (val_print_type_code_flags): Merely shift the

> 	VAL to the right to get rid of the lower bits.

> ---

>  gdb/valprint.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/gdb/valprint.c b/gdb/valprint.c

> index fa2b64ef10a..7f96e44a1dc 100644

> --- a/gdb/valprint.c

> +++ b/gdb/valprint.c

> @@ -1222,7 +1222,7 @@ val_print_type_code_flags (struct type *type, struct value *original_value,

>  	    {

>  	      unsigned field_len = TYPE_FIELD_BITSIZE (type, field);

>  	      ULONGEST field_val

> -		= val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);

> +		= val >> TYPE_FIELD_BITPOS (type, field);


This now fits on a single line.

Simon


From 569e6a95cb4f0ec13c3be317f07ad035c144d26d Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>

Date: Thu, 22 Jul 2021 15:15:29 -0400
Subject: [PATCH] Add test

Change-Id: Id4d7f89396aab931a02a1bfdaf9c6fd80d097c65
---
 gdb/valprint.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/gdb/valprint.c b/gdb/valprint.c
index 7f96e44a1dc9..92f7e7be4877 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
@@ -3137,10 +3139,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_flag (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);
+  contents[0] = 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-flag", test_print_flag);
+#endif
+
   cmd_list_element *cmd;
 
   cmd_list_element *set_print_cmd
-- 
2.32.0
Simon Marchi via Gdb-patches July 23, 2021, 1:42 a.m. | #2
> +static void

> +test_print_flag (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);

> +  contents[0] = 0xaa;


I noticed that this doesn't, it doesn't account for big endian arches.
You can replace it with:

    store_unsigned_integer (contents, 4, gdbarch_byte_order (arch), 0xaa);

I didn't see the failure because I was doing "maintenance selftest
flags", which didn't match the name of the name of the test,
"print-flag"... good job Simon.

Since there are multiple flags, and it's already pluralized everywhere,
the test should probably be called "print-flags", and the function
test_print_flags.

Simon
Simon Marchi via Gdb-patches July 23, 2021, 10:04 a.m. | #3
On 7/22/21 9:21 PM, Simon Marchi wrote:
> On 2021-07-19 12:29 p.m., Shahab Vahedi via Gdb-patches wrote:

>> From: Shahab Vahedi <shahab@synopsys.com>

>>

>> The correct extraction should be:

>> ------------------ 8< ------------------

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

>> ------------------ >8 ------------------

> 

> Please avoid lines starting with "---".  You can just remove those

> "8<" lines.  Indent the code blocks with four spaces, and it will be

> clear (and even formatted with a monospace font on some platforms like

> Github that render commit messages as markdown).

> 

> The reason is that when git-am-ing your patch, git drops everything

> above that line from the commit message (you can try it for yourself if

> you want to see).


I wasn't aware of this because "git am ..." just works for me.  I am using
git v2.32.0.  Nevertheless, that input is valuable and I will adjust the
commit message.

Rest of your inputs, in both emails, will be considered as well.  Thank you
for improving it!


-- 
Shahab
Simon Marchi via Gdb-patches July 23, 2021, 1:22 p.m. | #4
On 2021-07-22 3:21 p.m., Simon Marchi via Gdb-patches wrote:
> Please avoid lines starting with "---".  You can just remove those

> "8<" lines.  Indent the code blocks with four spaces, and it will be

> clear (and even formatted with a monospace font on some platforms like

> Github that render commit messages as markdown).

> 

> The reason is that when git-am-ing your patch, git drops everything

> above that line from the commit message (you can try it for yourself if

> you want to see).


For anybody reading this, I was mistaken.  It's because I have the
option mailinfo.scissors=1 in my git config.

Simon

Patch

diff --git a/gdb/valprint.c b/gdb/valprint.c
index fa2b64ef10a..7f96e44a1dc 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -1222,7 +1222,7 @@  val_print_type_code_flags (struct type *type, struct value *original_value,
 	    {
 	      unsigned field_len = TYPE_FIELD_BITSIZE (type, field);
 	      ULONGEST field_val
-		= val >> (TYPE_FIELD_BITPOS (type, field) - field_len + 1);
+		= val >> TYPE_FIELD_BITPOS (type, field);
 
 	      if (field_len < sizeof (ULONGEST) * TARGET_CHAR_BIT)
 		field_val &= ((ULONGEST) 1 << field_len) - 1;