* [PATCH] Fix bit offset regression
@ 2020-10-09 13:41 Tom Tromey
2020-10-09 14:27 ` Andrew Burgess
0 siblings, 1 reply; 3+ messages in thread
From: Tom Tromey @ 2020-10-09 13:41 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
The type-safe attribute patch introduced a regression that can occur
when the DW_AT_bit_offset value is negative. This can happen with
some Ada programs.
This patch fixes the problem. It also fixes a minor oddity in the
existing scalar storage test -- this test was intended to assign a
smaller number of bits to the field.
gdb/ChangeLog
2020-10-09 Tom Tromey <tromey@adacore.com>
* dwarf2/read.c (dwarf2_add_field): Handle signed offsets.
gdb/testsuite/ChangeLog
2020-10-09 Tom Tromey <tromey@adacore.com>
* gdb.ada/scalar_storage/storage.adb (Another_Range): New type.
(Rec): Add field. Fix range.
* gdb.ada/scalar_storage.exp: Update.
---
gdb/ChangeLog | 4 ++++
gdb/dwarf2/read.c | 10 +++++-----
gdb/testsuite/ChangeLog | 6 ++++++
gdb/testsuite/gdb.ada/scalar_storage.exp | 4 ++--
gdb/testsuite/gdb.ada/scalar_storage/storage.adb | 9 ++++++---
5 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index eedfea112d9..2ec3789135d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -15050,7 +15050,7 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
/* Get bit offset of field. */
handle_data_member_location (die, cu, fp);
attr = dwarf2_attr (die, DW_AT_bit_offset, cu);
- if (attr != nullptr && attr->form_is_unsigned ())
+ if (attr != nullptr && attr->form_is_constant ())
{
if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
{
@@ -15060,7 +15060,7 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
have to do anything special since we don't need to
know the size of the anonymous object. */
SET_FIELD_BITPOS (*fp, (FIELD_BITPOS (*fp)
- + attr->as_unsigned ()));
+ + attr->constant_value (0)));
}
else
{
@@ -15071,15 +15071,15 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
the field itself. The result is the bit offset of
the LSB of the field. */
int anonymous_size;
- int bit_offset = attr->as_unsigned ();
+ int bit_offset = attr->constant_value (0);
attr = dwarf2_attr (die, DW_AT_byte_size, cu);
- if (attr != nullptr && attr->form_is_unsigned ())
+ if (attr != nullptr && attr->form_is_constant ())
{
/* The size of the anonymous object containing
the bit field is explicit, so use the
indicated size (in bytes). */
- anonymous_size = attr->as_unsigned ();
+ anonymous_size = attr->constant_value (0);
}
else
{
diff --git a/gdb/testsuite/gdb.ada/scalar_storage.exp b/gdb/testsuite/gdb.ada/scalar_storage.exp
index b5e634c615b..952d7fd136e 100644
--- a/gdb/testsuite/gdb.ada/scalar_storage.exp
+++ b/gdb/testsuite/gdb.ada/scalar_storage.exp
@@ -34,5 +34,5 @@ if ![runto "storage.adb:$bp_location" ] then {
return
}
-gdb_test "print V_LE" "= \\(value => 126\\)"
-gdb_test "print V_BE" "= \\(value => 126\\)"
+gdb_test "print V_LE" "= \\(value => 126, another_value => 12\\)"
+gdb_test "print V_BE" "= \\(value => 126, another_value => 12\\)"
diff --git a/gdb/testsuite/gdb.ada/scalar_storage/storage.adb b/gdb/testsuite/gdb.ada/scalar_storage/storage.adb
index 608425d9dd1..741718e4e51 100644
--- a/gdb/testsuite/gdb.ada/scalar_storage/storage.adb
+++ b/gdb/testsuite/gdb.ada/scalar_storage/storage.adb
@@ -18,13 +18,16 @@ with System.Storage_Elements; use System.Storage_Elements;
procedure Storage is
subtype Some_Range is Natural range 0..127;
+ subtype Another_Range is Natural range 0..15;
type Rec is record
Value : Some_Range;
+ Another_Value : Another_Range;
end record;
for Rec use record
- Value at 0 range 0..127;
+ Value at 0 range 0..6;
+ Another_Value at 0 range 7..10;
end record;
type Rec_LE is new Rec;
@@ -39,8 +42,8 @@ procedure Storage is
V_BE : Rec_BE;
begin
- V_LE.Value := 126;
- V_BE.Value := 126;
+ V_LE := (126, 12);
+ V_BE := (126, 12);
Do_Nothing (V_LE'Address); -- START
Do_Nothing (V_BE'Address);
--
2.26.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fix bit offset regression
2020-10-09 13:41 [PATCH] Fix bit offset regression Tom Tromey
@ 2020-10-09 14:27 ` Andrew Burgess
2020-10-09 17:18 ` Tom Tromey
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Burgess @ 2020-10-09 14:27 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
* Tom Tromey <tromey@adacore.com> [2020-10-09 07:41:30 -0600]:
> The type-safe attribute patch introduced a regression that can occur
> when the DW_AT_bit_offset value is negative. This can happen with
> some Ada programs.
>
> This patch fixes the problem. It also fixes a minor oddity in the
> existing scalar storage test -- this test was intended to assign a
> smaller number of bits to the field.
>
> gdb/ChangeLog
> 2020-10-09 Tom Tromey <tromey@adacore.com>
>
> * dwarf2/read.c (dwarf2_add_field): Handle signed offsets.
>
> gdb/testsuite/ChangeLog
> 2020-10-09 Tom Tromey <tromey@adacore.com>
>
> * gdb.ada/scalar_storage/storage.adb (Another_Range): New type.
> (Rec): Add field. Fix range.
> * gdb.ada/scalar_storage.exp: Update.
> ---
> gdb/ChangeLog | 4 ++++
> gdb/dwarf2/read.c | 10 +++++-----
> gdb/testsuite/ChangeLog | 6 ++++++
> gdb/testsuite/gdb.ada/scalar_storage.exp | 4 ++--
> gdb/testsuite/gdb.ada/scalar_storage/storage.adb | 9 ++++++---
> 5 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index eedfea112d9..2ec3789135d 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -15050,7 +15050,7 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
> /* Get bit offset of field. */
> handle_data_member_location (die, cu, fp);
> attr = dwarf2_attr (die, DW_AT_bit_offset, cu);
> - if (attr != nullptr && attr->form_is_unsigned ())
> + if (attr != nullptr && attr->form_is_constant ())
> {
> if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> {
> @@ -15060,7 +15060,7 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
> have to do anything special since we don't need to
> know the size of the anonymous object. */
> SET_FIELD_BITPOS (*fp, (FIELD_BITPOS (*fp)
> - + attr->as_unsigned ()));
> + + attr->constant_value (0)));
I wondered what the `0` was all about here, so I took a look... In
attribute::constant_value it says:
/* For DW_FORM_data16 see attribute::form_is_constant. */
complaint (_("Attribute value is not a constant (%s)"),
dwarf_form_name (form));
return default_value;
Hmm, and the comment on form_is_constant says:
DW_FORM_data16 is not considered as constant_value cannot handle
that. */
I'm not sure I'm any more enlightened about the default value...
None of which has anything to do with your patch, which looks fine.
Thanks,
Andrew
> }
> else
> {
> @@ -15071,15 +15071,15 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
> the field itself. The result is the bit offset of
> the LSB of the field. */
> int anonymous_size;
> - int bit_offset = attr->as_unsigned ();
> + int bit_offset = attr->constant_value (0);
>
> attr = dwarf2_attr (die, DW_AT_byte_size, cu);
> - if (attr != nullptr && attr->form_is_unsigned ())
> + if (attr != nullptr && attr->form_is_constant ())
> {
> /* The size of the anonymous object containing
> the bit field is explicit, so use the
> indicated size (in bytes). */
> - anonymous_size = attr->as_unsigned ();
> + anonymous_size = attr->constant_value (0);
> }
> else
> {
> diff --git a/gdb/testsuite/gdb.ada/scalar_storage.exp b/gdb/testsuite/gdb.ada/scalar_storage.exp
> index b5e634c615b..952d7fd136e 100644
> --- a/gdb/testsuite/gdb.ada/scalar_storage.exp
> +++ b/gdb/testsuite/gdb.ada/scalar_storage.exp
> @@ -34,5 +34,5 @@ if ![runto "storage.adb:$bp_location" ] then {
> return
> }
>
> -gdb_test "print V_LE" "= \\(value => 126\\)"
> -gdb_test "print V_BE" "= \\(value => 126\\)"
> +gdb_test "print V_LE" "= \\(value => 126, another_value => 12\\)"
> +gdb_test "print V_BE" "= \\(value => 126, another_value => 12\\)"
> diff --git a/gdb/testsuite/gdb.ada/scalar_storage/storage.adb b/gdb/testsuite/gdb.ada/scalar_storage/storage.adb
> index 608425d9dd1..741718e4e51 100644
> --- a/gdb/testsuite/gdb.ada/scalar_storage/storage.adb
> +++ b/gdb/testsuite/gdb.ada/scalar_storage/storage.adb
> @@ -18,13 +18,16 @@ with System.Storage_Elements; use System.Storage_Elements;
>
> procedure Storage is
> subtype Some_Range is Natural range 0..127;
> + subtype Another_Range is Natural range 0..15;
>
> type Rec is record
> Value : Some_Range;
> + Another_Value : Another_Range;
> end record;
>
> for Rec use record
> - Value at 0 range 0..127;
> + Value at 0 range 0..6;
> + Another_Value at 0 range 7..10;
> end record;
>
> type Rec_LE is new Rec;
> @@ -39,8 +42,8 @@ procedure Storage is
> V_BE : Rec_BE;
>
> begin
> - V_LE.Value := 126;
> - V_BE.Value := 126;
> + V_LE := (126, 12);
> + V_BE := (126, 12);
>
> Do_Nothing (V_LE'Address); -- START
> Do_Nothing (V_BE'Address);
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fix bit offset regression
2020-10-09 14:27 ` Andrew Burgess
@ 2020-10-09 17:18 ` Tom Tromey
0 siblings, 0 replies; 3+ messages in thread
From: Tom Tromey @ 2020-10-09 17:18 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches
>> SET_FIELD_BITPOS (*fp, (FIELD_BITPOS (*fp)
>> - + attr->as_unsigned ()));
>> + + attr->constant_value (0)));
Andrew> I wondered what the `0` was all about here, so I took a look...
[...]
Andrew> I'm not sure I'm any more enlightened about the default value...
It's a bit of an API error, IMO.
Basically, constant_value lets you specify a default, so that if the
attribute does not have constant form, then the default is returned.
However, in this patch (and perhaps in other cases, I didn't look), the
attribute is already known to have constant form, so there's no need for
a default.
Perhaps I should add a default-less overload that asserts.
What do you think?
Andrew> None of which has anything to do with your patch, which looks
Andrew> fine.
I'm going to check it in.
Tom
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-09 17:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09 13:41 [PATCH] Fix bit offset regression Tom Tromey
2020-10-09 14:27 ` Andrew Burgess
2020-10-09 17:18 ` Tom Tromey
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).