* [PATCH v2] gdb: Fix numerical field extraction for target description "flags"
@ 2021-07-23 12:38 Shahab Vahedi
2021-07-23 13:26 ` Simon Marchi
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Shahab Vahedi @ 2021-07-23 12:38 UTC (permalink / raw)
To: gdb-patches; +Cc: Shahab Vahedi, Simon Marchi
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] gdb: Fix numerical field extraction for target description "flags"
2021-07-23 12:38 [PATCH v2] gdb: Fix numerical field extraction for target description "flags" Shahab Vahedi
@ 2021-07-23 13:26 ` Simon Marchi
2021-07-24 11:16 ` Simon Marchi
2021-07-26 13:17 ` [PUSHED master] " Shahab Vahedi
2021-07-26 13:17 ` [PUSHED gdb-11-branch] " Shahab Vahedi
2 siblings, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2021-07-23 13:26 UTC (permalink / raw)
To: Shahab Vahedi, gdb-patches; +Cc: Shahab Vahedi, 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] gdb: Fix numerical field extraction for target description "flags"
2021-07-23 13:26 ` Simon Marchi
@ 2021-07-24 11:16 ` Simon Marchi
2021-07-24 13:50 ` Joel Brobecker
0 siblings, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2021-07-24 11:16 UTC (permalink / raw)
To: Shahab Vahedi, gdb-patches; +Cc: Shahab Vahedi, Simon Marchi, Joel Brobecker
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] gdb: Fix numerical field extraction for target description "flags"
2021-07-24 11:16 ` Simon Marchi
@ 2021-07-24 13:50 ` Joel Brobecker
2021-07-26 13:33 ` Shahab Vahedi
0 siblings, 1 reply; 8+ messages in thread
From: Joel Brobecker @ 2021-07-24 13:50 UTC (permalink / raw)
To: Simon Marchi
Cc: Shahab Vahedi, gdb-patches, Shahab Vahedi, Simon Marchi, 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PUSHED master] gdb: Fix numerical field extraction for target description "flags"
2021-07-23 12:38 [PATCH v2] gdb: Fix numerical field extraction for target description "flags" Shahab Vahedi
2021-07-23 13:26 ` Simon Marchi
@ 2021-07-26 13:17 ` Shahab Vahedi
2021-07-26 13:17 ` [PUSHED gdb-11-branch] " Shahab Vahedi
2 siblings, 0 replies; 8+ messages in thread
From: Shahab Vahedi @ 2021-07-26 13:17 UTC (permalink / raw)
To: gdb-patches; +Cc: Shahab Vahedi, Simon Marchi
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PUSHED gdb-11-branch] gdb: Fix numerical field extraction for target description "flags"
2021-07-23 12:38 [PATCH v2] gdb: Fix numerical field extraction for target description "flags" Shahab Vahedi
2021-07-23 13:26 ` Simon Marchi
2021-07-26 13:17 ` [PUSHED master] " Shahab Vahedi
@ 2021-07-26 13:17 ` Shahab Vahedi
2 siblings, 0 replies; 8+ messages in thread
From: Shahab Vahedi @ 2021-07-26 13:17 UTC (permalink / raw)
To: gdb-patches; +Cc: Shahab Vahedi, Simon Marchi
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] gdb: Fix numerical field extraction for target description "flags"
2021-07-24 13:50 ` Joel Brobecker
@ 2021-07-26 13:33 ` Shahab Vahedi
2021-07-26 13:51 ` Simon Marchi
0 siblings, 1 reply; 8+ messages in thread
From: Shahab Vahedi @ 2021-07-26 13:33 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi; +Cc: Shahab Vahedi, gdb-patches, Simon Marchi
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] gdb: Fix numerical field extraction for target description "flags"
2021-07-26 13:33 ` Shahab Vahedi
@ 2021-07-26 13:51 ` Simon Marchi
0 siblings, 0 replies; 8+ messages in thread
From: Simon Marchi @ 2021-07-26 13:51 UTC (permalink / raw)
To: Shahab Vahedi, Joel Brobecker; +Cc: Shahab Vahedi, gdb-patches, 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-26 13:51 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 12:38 [PATCH v2] gdb: Fix numerical field extraction for target description "flags" Shahab Vahedi
2021-07-23 13:26 ` Simon Marchi
2021-07-24 11:16 ` Simon Marchi
2021-07-24 13:50 ` Joel Brobecker
2021-07-26 13:33 ` Shahab Vahedi
2021-07-26 13:51 ` Simon Marchi
2021-07-26 13:17 ` [PUSHED master] " Shahab Vahedi
2021-07-26 13:17 ` [PUSHED gdb-11-branch] " Shahab Vahedi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).