Re: [PATCH] Fix bit offset regression

[Andrew Burgess <andrew.burgess@embecosm.com>]

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