dwarf2out: Fix up field_byte_offset [PR101378]

  Hi!

For PCC_BITFIELD_TYPE_MATTERS field_byte_offset has quite large code
to deal with it since many years ago (see it e.g. in GCC 3.2, although it
used to be on HOST_WIDE_INTs, then on double_ints, now on offset_ints).
But that code apparently isn't able to cope with members with empty class
types with [[no_unique_address]] attribute, because the empty classes have
non-zero type size but zero decl size and so one can end up from the
computation with negative offset or offset 1 byte smaller than it should be.
For !PCC_BITFIELD_TYPE_MATTERS, we just use
    tree_result = byte_position (decl);
which seems exactly right even for the empty classes or anything which is
not a bitfield (and for which we don't add DW_AT_bit_offset attribute).
So, instead of trying to handle those no_unique_address members in the
current already very complicated code, this limits it to bitfields.

stor-layout.c PCC_BITFIELD_TYPE_MATTERS handling also affects only
bitfields, twice it checks DECL_BIT_FIELD and once DECL_BIT_FIELD_TYPE.

The only thing I'm unsure about is whether the test should be
DECL_BIT_FIELD or DECL_BIT_FIELD_TYPE should be tested.  I thought it
doesn't matter, but it seems stor-layout.c in some cases clears
DECL_BIT_FIELD if their TYPE_MODE can express the type exactly, and
dwarf2out.c (gen_field_die) uses
  if (DECL_BIT_FIELD_TYPE (decl))
to decide if DW_AT_bit_offset etc. attributes should be added.
So maybe I should go with && DECL_BIT_FIELD_TYPE (decl) instead.
On
struct S { int e; int a : 1, b : 7, c : 8, d : 16; } s;
struct T { int a : 1, b : 7; long long c : 8; int d : 16; } t;
it doesn't make a difference though on x86_64, ppc64le nor ppc64...

I think Ada has bitfields of aggregate types, so CCing Eric, though
I'd hope it doesn't have bitfields where type size is smaller than
field decl size like C++ has.

Bootstrapped/regtested on x86_64-linux, i686-linux, powerpc64le-linux
and powerpc64-linux and Pedro has tested it on GDB testsuite.

I can bootstrap/regtest the
+      && DECL_BIT_FIELD_TYPE (decl)
version too.

2021-11-10  Jakub Jelinek  <jakub@redhat.com>

	PR debug/101378
	* dwarf2out.c (field_byte_offset): Do the PCC_BITFIELD_TYPE_MATTERS
	handling only for DECL_BIT_FIELD decls.

	* g++.dg/debug/dwarf2/pr101378.C: New test.


	Jakub
  

> I think Ada has bitfields of aggregate types, so CCing Eric, though
> I'd hope it doesn't have bitfields where type size is smaller than
> field decl size like C++ has.

Assuming your sentence is written in the right sense :-) then, no, in Ada bit-
fields always have DECL_SIZE (bf) <= TYPE_SIZE (TREE_TYPE (bf)).
  

On Wed, 10 Nov 2021, Jakub Jelinek wrote:

> Hi!
> 
> For PCC_BITFIELD_TYPE_MATTERS field_byte_offset has quite large code
> to deal with it since many years ago (see it e.g. in GCC 3.2, although it
> used to be on HOST_WIDE_INTs, then on double_ints, now on offset_ints).
> But that code apparently isn't able to cope with members with empty class
> types with [[no_unique_address]] attribute, because the empty classes have
> non-zero type size but zero decl size and so one can end up from the
> computation with negative offset or offset 1 byte smaller than it should be.
> For !PCC_BITFIELD_TYPE_MATTERS, we just use
>     tree_result = byte_position (decl);
> which seems exactly right even for the empty classes or anything which is
> not a bitfield (and for which we don't add DW_AT_bit_offset attribute).
> So, instead of trying to handle those no_unique_address members in the
> current already very complicated code, this limits it to bitfields.
> 
> stor-layout.c PCC_BITFIELD_TYPE_MATTERS handling also affects only
> bitfields, twice it checks DECL_BIT_FIELD and once DECL_BIT_FIELD_TYPE.
> 
> The only thing I'm unsure about is whether the test should be
> DECL_BIT_FIELD or DECL_BIT_FIELD_TYPE should be tested.  I thought it
> doesn't matter, but it seems stor-layout.c in some cases clears
> DECL_BIT_FIELD if their TYPE_MODE can express the type exactly, and
> dwarf2out.c (gen_field_die) uses
>   if (DECL_BIT_FIELD_TYPE (decl))
> to decide if DW_AT_bit_offset etc. attributes should be added.
> So maybe I should go with && DECL_BIT_FIELD_TYPE (decl) instead.

You need DECL_BIT_FIELD_TYPE if you want to know whether it is
a bitfield.  DECL_BIT_FIELD can only be used to test whether the
size is not a multiple of BITS_PER_UNIT.

So the question is whether the code makes a difference if
the bitfield is int a : 8; int b : 8; int c : 16; for example.
If so then DECL_BIT_FIELD_TYPE is needed, otherwise what you
test probably doesn't matter.

> On
> struct S { int e; int a : 1, b : 7, c : 8, d : 16; } s;
> struct T { int a : 1, b : 7; long long c : 8; int d : 16; } t;
> it doesn't make a difference though on x86_64, ppc64le nor ppc64...
> 
> I think Ada has bitfields of aggregate types, so CCing Eric, though
> I'd hope it doesn't have bitfields where type size is smaller than
> field decl size like C++ has.
> 
> Bootstrapped/regtested on x86_64-linux, i686-linux, powerpc64le-linux
> and powerpc64-linux and Pedro has tested it on GDB testsuite.
> 
> I can bootstrap/regtest the
> +      && DECL_BIT_FIELD_TYPE (decl)
> version too.
> 
> 2021-11-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/101378
> 	* dwarf2out.c (field_byte_offset): Do the PCC_BITFIELD_TYPE_MATTERS
> 	handling only for DECL_BIT_FIELD decls.
> 
> 	* g++.dg/debug/dwarf2/pr101378.C: New test.
> 
> --- gcc/dwarf2out.c.jj	2021-11-05 10:19:46.339457342 +0100
> +++ gcc/dwarf2out.c	2021-11-09 15:01:51.425437717 +0100
> @@ -19646,6 +19646,7 @@ field_byte_offset (const_tree decl, stru
>       properly dynamic byte offsets only when PCC bitfield type doesn't
>       matter.  */
>    if (PCC_BITFIELD_TYPE_MATTERS
> +      && DECL_BIT_FIELD (decl)
>        && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)

What's more interesting is the INTEGER_CST restriction - I'm sure
that Ada allows bitfields to follow variable position other fields.
Even C does:

void foo (int n)
{
  struct S { int a[n]; int b : 5;  int c : 3; } s;
}

runs into the code above and ends up not honoring 
PCC_BITFIELD_TYPE_MATTERS ...

Richard.

>      {
>        offset_int object_offset_in_bits;
> --- gcc/testsuite/g++.dg/debug/dwarf2/pr101378.C.jj	2021-11-09 15:17:39.504975396 +0100
> +++ gcc/testsuite/g++.dg/debug/dwarf2/pr101378.C	2021-11-09 15:17:28.067137556 +0100
> @@ -0,0 +1,13 @@
> +// PR debug/101378
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-gdwarf-5 -dA" }
> +// { dg-final { scan-assembler-times "0\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> +// { dg-final { scan-assembler-times "1\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> +// { dg-final { scan-assembler-times "2\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> +// { dg-final { scan-assembler-not "-1\[^0-9x\\r\\n\]* DW_AT_data_member_location" } }
> +
> +struct E {};
> +struct S
> +{
> +  [[no_unique_address]] E e, f, g;
> +} s;
> 
> 	Jakub
> 
>
  

On Wed, Nov 10, 2021 at 10:52:42AM +0100, Richard Biener wrote:
> > The only thing I'm unsure about is whether the test should be
> > DECL_BIT_FIELD or DECL_BIT_FIELD_TYPE should be tested.  I thought it
> > doesn't matter, but it seems stor-layout.c in some cases clears
> > DECL_BIT_FIELD if their TYPE_MODE can express the type exactly, and
> > dwarf2out.c (gen_field_die) uses
> >   if (DECL_BIT_FIELD_TYPE (decl))
> > to decide if DW_AT_bit_offset etc. attributes should be added.
> > So maybe I should go with && DECL_BIT_FIELD_TYPE (decl) instead.
> 
> You need DECL_BIT_FIELD_TYPE if you want to know whether it is
> a bitfield.  DECL_BIT_FIELD can only be used to test whether the
> size is not a multiple of BITS_PER_UNIT.

Yeah, so I think DECL_BIT_FIELD_TYPE is the right test and I'll retest with
that.

> So the question is whether the code makes a difference if
> the bitfield is int a : 8; int b : 8; int c : 16; for example.
> If so then DECL_BIT_FIELD_TYPE is needed, otherwise what you
> test probably doesn't matter.

Apparently I made a mistake of trying those tests with -g -dA.
The problem is that for bitfields we then emit DW_AT_data_bit_offset
(which is already in DWARF4, but we chose to emit it only for DWARF5
as many consumers didn't handle it).
So, on
struct S { int e; int a : 1, b : 7, c : 8, d : 16; } s;
struct T { int a : 1, b : 7; long long c : 8; int d : 16; } t;

int
main ()
{
  s.c = 0x55;
  s.d = 0xaaaa;
  t.c = 0x55;
  t.d = 0xaaaa;
  s.e++;
}
the difference with the patch as posted and -gdwarf-4 -dA is:
        .uleb128 0x4    # (DIE (0x5f) DW_TAG_member)
        .ascii "c\0"    # DW_AT_name
        .byte   0x1     # DW_AT_decl_file (hoho.C)
        .byte   0x1     # DW_AT_decl_line
        .byte   0x25    # DW_AT_decl_column
        .long   0x7c    # DW_AT_type
        .byte   0x4     # DW_AT_byte_size
        .byte   0x8     # DW_AT_bit_size
-       .byte   0x10    # DW_AT_bit_offset
-       .byte   0x4     # DW_AT_data_member_location
+       .byte   0x18    # DW_AT_bit_offset
+       .byte   0x5     # DW_AT_data_member_location
        .uleb128 0x4    # (DIE (0x6d) DW_TAG_member)
        .ascii "d\0"    # DW_AT_name
        .byte   0x1     # DW_AT_decl_file (hoho.C)
        .byte   0x1     # DW_AT_decl_line
        .byte   0x2c    # DW_AT_decl_column
        .long   0x7c    # DW_AT_type
        .byte   0x4     # DW_AT_byte_size
        .byte   0x10    # DW_AT_bit_size
-       .byte   0       # DW_AT_bit_offset
-       .byte   0x4     # DW_AT_data_member_location
+       .byte   0x10    # DW_AT_bit_offset
+       .byte   0x6     # DW_AT_data_member_location
        .byte   0       # end of children of DIE 0x2d
and
        .uleb128 0x4    # (DIE (0xbe) DW_TAG_member)
        .ascii "c\0"    # DW_AT_name
        .byte   0x1     # DW_AT_decl_file (hoho.C)
        .byte   0x2     # DW_AT_decl_line
        .byte   0x28    # DW_AT_decl_column
        .long   0xdb    # DW_AT_type
        .byte   0x8     # DW_AT_byte_size
        .byte   0x8     # DW_AT_bit_size
-       .byte   0x30    # DW_AT_bit_offset
-       .byte   0       # DW_AT_data_member_location
+       .byte   0x38    # DW_AT_bit_offset
+       .byte   0x1     # DW_AT_data_member_location
        .uleb128 0x4    # (DIE (0xcc) DW_TAG_member)
        .ascii "d\0"    # DW_AT_name
        .byte   0x1     # DW_AT_decl_file (hoho.C)
        .byte   0x2     # DW_AT_decl_line
        .byte   0x33    # DW_AT_decl_column
        .long   0x7c    # DW_AT_type
        .byte   0x4     # DW_AT_byte_size
        .byte   0x10    # DW_AT_bit_size
-       .byte   0       # DW_AT_bit_offset
-       .byte   0       # DW_AT_data_member_location
+       .byte   0x10    # DW_AT_bit_offset
+       .byte   0x2     # DW_AT_data_member_location
        .byte   0       # end of children of DIE 0x97
but GDB handles both fine.

> >    if (PCC_BITFIELD_TYPE_MATTERS
> > +      && DECL_BIT_FIELD (decl)
> >        && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
> 
> What's more interesting is the INTEGER_CST restriction - I'm sure
> that Ada allows bitfields to follow variable position other fields.
> Even C does:
> 
> void foo (int n)
> {
>   struct S { int a[n]; int b : 5;  int c : 3; } s;
> }
> 
> runs into the code above and ends up not honoring 
> PCC_BITFIELD_TYPE_MATTERS ...

True.  And, apparently we've been mishandling this forever.
The function actually doesn't and has never had any way to signal to caller
that it gave up and was unable to handle it correctly, before the addition
of dw_loc_descr_ref return the function was just returning HOST_WIDE_INT
and returned 0 both in cases where the field offset was really 0 and where
it gave up (e.g. because bit_position wasn't INTEGER_CST representable
in hwi).

I'm afraid I forgot the DECL_FIELD_OFFSET vs. DECL_FIELD_BIT_OFFSET stuff
enough that I'm not sure what is the right fix for that case, maybe it would
work if we dropped the && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST
check and instead used:
-      bitpos_int = wi::to_offset (DECL_FIELD_BIT_OFFSET (decl));
+      if (TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
+	bitpos_int = wi::to_offset (bit_position (decl));
+      else
+	bitpos_int = wi::to_offset (DECL_FIELD_BIT_OFFSET (decl));
at the start and
-      if (ctx->variant_part_offset == NULL_TREE)
+      if (ctx->variant_part_offset == NULL_TREE
+	  && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
         {
           *cst_offset = object_offset_in_bytes.to_shwi ();
           return NULL;
         }
       tree_result = wide_int_to_tree (sizetype, object_offset_in_bytes);
+      if (TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
+	tree_result = size_binop (PLUS_EXPR, DECL_FIELD_OFFSET (decl),
+				  tree_result);
or so.
But, is it ok to defer that to stage3 and incremental patch?

2021-11-10  Jakub Jelinek  <jakub@redhat.com>

	PR debug/101378
	* dwarf2out.c (field_byte_offset): Do the PCC_BITFIELD_TYPE_MATTERS
	handling only for DECL_BIT_FIELD_TYPE decls.

	* g++.dg/debug/dwarf2/pr101378.C: New test.

--- gcc/dwarf2out.c.jj	2021-11-05 10:19:46.339457342 +0100
+++ gcc/dwarf2out.c	2021-11-09 15:01:51.425437717 +0100
@@ -19646,6 +19646,7 @@ field_byte_offset (const_tree decl, stru
      properly dynamic byte offsets only when PCC bitfield type doesn't
      matter.  */
   if (PCC_BITFIELD_TYPE_MATTERS
+      && DECL_BIT_FIELD_TYPE (decl)
       && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
     {
       offset_int object_offset_in_bits;
--- gcc/testsuite/g++.dg/debug/dwarf2/pr101378.C.jj	2021-11-09 15:17:39.504975396 +0100
+++ gcc/testsuite/g++.dg/debug/dwarf2/pr101378.C	2021-11-09 15:17:28.067137556 +0100
@@ -0,0 +1,13 @@
+// PR debug/101378
+// { dg-do compile { target c++11 } }
+// { dg-options "-gdwarf-5 -dA" }
+// { dg-final { scan-assembler-times "0\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
+// { dg-final { scan-assembler-times "1\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
+// { dg-final { scan-assembler-times "2\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
+// { dg-final { scan-assembler-not "-1\[^0-9x\\r\\n\]* DW_AT_data_member_location" } }
+
+struct E {};
+struct S
+{
+  [[no_unique_address]] E e, f, g;
+} s;


	Jakub
  

On Wed, 10 Nov 2021, Jakub Jelinek wrote:

> On Wed, Nov 10, 2021 at 10:52:42AM +0100, Richard Biener wrote:
> > > The only thing I'm unsure about is whether the test should be
> > > DECL_BIT_FIELD or DECL_BIT_FIELD_TYPE should be tested.  I thought it
> > > doesn't matter, but it seems stor-layout.c in some cases clears
> > > DECL_BIT_FIELD if their TYPE_MODE can express the type exactly, and
> > > dwarf2out.c (gen_field_die) uses
> > >   if (DECL_BIT_FIELD_TYPE (decl))
> > > to decide if DW_AT_bit_offset etc. attributes should be added.
> > > So maybe I should go with && DECL_BIT_FIELD_TYPE (decl) instead.
> > 
> > You need DECL_BIT_FIELD_TYPE if you want to know whether it is
> > a bitfield.  DECL_BIT_FIELD can only be used to test whether the
> > size is not a multiple of BITS_PER_UNIT.
> 
> Yeah, so I think DECL_BIT_FIELD_TYPE is the right test and I'll retest with
> that.
> 
> > So the question is whether the code makes a difference if
> > the bitfield is int a : 8; int b : 8; int c : 16; for example.
> > If so then DECL_BIT_FIELD_TYPE is needed, otherwise what you
> > test probably doesn't matter.
> 
> Apparently I made a mistake of trying those tests with -g -dA.
> The problem is that for bitfields we then emit DW_AT_data_bit_offset
> (which is already in DWARF4, but we chose to emit it only for DWARF5
> as many consumers didn't handle it).
> So, on
> struct S { int e; int a : 1, b : 7, c : 8, d : 16; } s;
> struct T { int a : 1, b : 7; long long c : 8; int d : 16; } t;
> 
> int
> main ()
> {
>   s.c = 0x55;
>   s.d = 0xaaaa;
>   t.c = 0x55;
>   t.d = 0xaaaa;
>   s.e++;
> }
> the difference with the patch as posted and -gdwarf-4 -dA is:
>         .uleb128 0x4    # (DIE (0x5f) DW_TAG_member)
>         .ascii "c\0"    # DW_AT_name
>         .byte   0x1     # DW_AT_decl_file (hoho.C)
>         .byte   0x1     # DW_AT_decl_line
>         .byte   0x25    # DW_AT_decl_column
>         .long   0x7c    # DW_AT_type
>         .byte   0x4     # DW_AT_byte_size
>         .byte   0x8     # DW_AT_bit_size
> -       .byte   0x10    # DW_AT_bit_offset
> -       .byte   0x4     # DW_AT_data_member_location
> +       .byte   0x18    # DW_AT_bit_offset
> +       .byte   0x5     # DW_AT_data_member_location
>         .uleb128 0x4    # (DIE (0x6d) DW_TAG_member)
>         .ascii "d\0"    # DW_AT_name
>         .byte   0x1     # DW_AT_decl_file (hoho.C)
>         .byte   0x1     # DW_AT_decl_line
>         .byte   0x2c    # DW_AT_decl_column
>         .long   0x7c    # DW_AT_type
>         .byte   0x4     # DW_AT_byte_size
>         .byte   0x10    # DW_AT_bit_size
> -       .byte   0       # DW_AT_bit_offset
> -       .byte   0x4     # DW_AT_data_member_location
> +       .byte   0x10    # DW_AT_bit_offset
> +       .byte   0x6     # DW_AT_data_member_location
>         .byte   0       # end of children of DIE 0x2d
> and
>         .uleb128 0x4    # (DIE (0xbe) DW_TAG_member)
>         .ascii "c\0"    # DW_AT_name
>         .byte   0x1     # DW_AT_decl_file (hoho.C)
>         .byte   0x2     # DW_AT_decl_line
>         .byte   0x28    # DW_AT_decl_column
>         .long   0xdb    # DW_AT_type
>         .byte   0x8     # DW_AT_byte_size
>         .byte   0x8     # DW_AT_bit_size
> -       .byte   0x30    # DW_AT_bit_offset
> -       .byte   0       # DW_AT_data_member_location
> +       .byte   0x38    # DW_AT_bit_offset
> +       .byte   0x1     # DW_AT_data_member_location
>         .uleb128 0x4    # (DIE (0xcc) DW_TAG_member)
>         .ascii "d\0"    # DW_AT_name
>         .byte   0x1     # DW_AT_decl_file (hoho.C)
>         .byte   0x2     # DW_AT_decl_line
>         .byte   0x33    # DW_AT_decl_column
>         .long   0x7c    # DW_AT_type
>         .byte   0x4     # DW_AT_byte_size
>         .byte   0x10    # DW_AT_bit_size
> -       .byte   0       # DW_AT_bit_offset
> -       .byte   0       # DW_AT_data_member_location
> +       .byte   0x10    # DW_AT_bit_offset
> +       .byte   0x2     # DW_AT_data_member_location
>         .byte   0       # end of children of DIE 0x97
> but GDB handles both fine.
> 
> > >    if (PCC_BITFIELD_TYPE_MATTERS
> > > +      && DECL_BIT_FIELD (decl)
> > >        && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
> > 
> > What's more interesting is the INTEGER_CST restriction - I'm sure
> > that Ada allows bitfields to follow variable position other fields.
> > Even C does:
> > 
> > void foo (int n)
> > {
> >   struct S { int a[n]; int b : 5;  int c : 3; } s;
> > }
> > 
> > runs into the code above and ends up not honoring 
> > PCC_BITFIELD_TYPE_MATTERS ...
> 
> True.  And, apparently we've been mishandling this forever.
> The function actually doesn't and has never had any way to signal to caller
> that it gave up and was unable to handle it correctly, before the addition
> of dw_loc_descr_ref return the function was just returning HOST_WIDE_INT
> and returned 0 both in cases where the field offset was really 0 and where
> it gave up (e.g. because bit_position wasn't INTEGER_CST representable
> in hwi).
> 
> I'm afraid I forgot the DECL_FIELD_OFFSET vs. DECL_FIELD_BIT_OFFSET stuff
> enough that I'm not sure what is the right fix for that case, maybe it would
> work if we dropped the && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST
> check and instead used:
> -      bitpos_int = wi::to_offset (DECL_FIELD_BIT_OFFSET (decl));
> +      if (TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
> +	bitpos_int = wi::to_offset (bit_position (decl));
> +      else
> +	bitpos_int = wi::to_offset (DECL_FIELD_BIT_OFFSET (decl));
> at the start and
> -      if (ctx->variant_part_offset == NULL_TREE)
> +      if (ctx->variant_part_offset == NULL_TREE
> +	  && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
>          {
>            *cst_offset = object_offset_in_bytes.to_shwi ();
>            return NULL;
>          }
>        tree_result = wide_int_to_tree (sizetype, object_offset_in_bytes);
> +      if (TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
> +	tree_result = size_binop (PLUS_EXPR, DECL_FIELD_OFFSET (decl),
> +				  tree_result);

that needs multiplication by BITS_PER_UNIT.

> or so.

Not sure if it's worth special-casing constant DECL_FIELD_OFFSET
though, or whether the code can just work with DECL_FIELD_BIT_OFFSET
and DECL_FIELD_OFFSET be always added.


> But, is it ok to defer that to stage3 and incremental patch?

Sure.

Thanks,
Richard.

> 2021-11-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/101378
> 	* dwarf2out.c (field_byte_offset): Do the PCC_BITFIELD_TYPE_MATTERS
> 	handling only for DECL_BIT_FIELD_TYPE decls.
> 
> 	* g++.dg/debug/dwarf2/pr101378.C: New test.
> 
> --- gcc/dwarf2out.c.jj	2021-11-05 10:19:46.339457342 +0100
> +++ gcc/dwarf2out.c	2021-11-09 15:01:51.425437717 +0100
> @@ -19646,6 +19646,7 @@ field_byte_offset (const_tree decl, stru
>       properly dynamic byte offsets only when PCC bitfield type doesn't
>       matter.  */
>    if (PCC_BITFIELD_TYPE_MATTERS
> +      && DECL_BIT_FIELD_TYPE (decl)
>        && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
>      {
>        offset_int object_offset_in_bits;
> --- gcc/testsuite/g++.dg/debug/dwarf2/pr101378.C.jj	2021-11-09 15:17:39.504975396 +0100
> +++ gcc/testsuite/g++.dg/debug/dwarf2/pr101378.C	2021-11-09 15:17:28.067137556 +0100
> @@ -0,0 +1,13 @@
> +// PR debug/101378
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-gdwarf-5 -dA" }
> +// { dg-final { scan-assembler-times "0\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> +// { dg-final { scan-assembler-times "1\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> +// { dg-final { scan-assembler-times "2\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> +// { dg-final { scan-assembler-not "-1\[^0-9x\\r\\n\]* DW_AT_data_member_location" } }
> +
> +struct E {};
> +struct S
> +{
> +  [[no_unique_address]] E e, f, g;
> +} s;
> 
> 
> 	Jakub
> 
>
  

On Wed, Nov 10, 2021 at 12:36:04PM +0100, Richard Biener wrote:
> > I'm afraid I forgot the DECL_FIELD_OFFSET vs. DECL_FIELD_BIT_OFFSET stuff
> > enough that I'm not sure what is the right fix for that case, maybe it would
> > work if we dropped the && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST
> > check and instead used:
> > -      bitpos_int = wi::to_offset (DECL_FIELD_BIT_OFFSET (decl));
> > +      if (TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
> > +	bitpos_int = wi::to_offset (bit_position (decl));
> > +      else
> > +	bitpos_int = wi::to_offset (DECL_FIELD_BIT_OFFSET (decl));
> > at the start and
> > -      if (ctx->variant_part_offset == NULL_TREE)
> > +      if (ctx->variant_part_offset == NULL_TREE
> > +	  && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
> >          {
> >            *cst_offset = object_offset_in_bytes.to_shwi ();
> >            return NULL;
> >          }
> >        tree_result = wide_int_to_tree (sizetype, object_offset_in_bytes);
> > +      if (TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
> > +	tree_result = size_binop (PLUS_EXPR, DECL_FIELD_OFFSET (decl),
> > +				  tree_result);
> 
> that needs multiplication by BITS_PER_UNIT.

I don't think so.
From bit_from_pos and byte_from_pos functions it is clear that
DECL_FIELD_OFFSET is in bytes and DECL_FIELD_BIT_OFFSET in bits.
And object_offset_in_bytes is in bytes too.

> 
> > or so.
> 
> Not sure if it's worth special-casing constant DECL_FIELD_OFFSET
> though, or whether the code can just work with DECL_FIELD_BIT_OFFSET
> and DECL_FIELD_OFFSET be always added.

It has certainly the advantage that it doesn't change anything for the
most common case (where DECL_FIELD_OFFSET is INTEGER_CST).

	Jakub
  

Hi!

Bootstrapped/regtested now successfully on x86_64-linux and i686-linux,
verified the
struct S { int e; int a : 1, b : 7, c : 8, d : 16; } s;
struct T { int a : 1, b : 7; long long c : 8; int d : 16; } t;

int
main ()
{
  s.c = 0x55;
  s.d = 0xaaaa;
  t.c = 0x55;
  t.d = 0xaaaa;
  s.e++;
}
testcase is compiled the same way as before again, ok for trunk?

> 2021-11-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/101378
> 	* dwarf2out.c (field_byte_offset): Do the PCC_BITFIELD_TYPE_MATTERS
> 	handling only for DECL_BIT_FIELD_TYPE decls.
> 
> 	* g++.dg/debug/dwarf2/pr101378.C: New test.
> 
> --- gcc/dwarf2out.c.jj	2021-11-05 10:19:46.339457342 +0100
> +++ gcc/dwarf2out.c	2021-11-09 15:01:51.425437717 +0100
> @@ -19646,6 +19646,7 @@ field_byte_offset (const_tree decl, stru
>       properly dynamic byte offsets only when PCC bitfield type doesn't
>       matter.  */
>    if (PCC_BITFIELD_TYPE_MATTERS
> +      && DECL_BIT_FIELD_TYPE (decl)
>        && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
>      {
>        offset_int object_offset_in_bits;
> --- gcc/testsuite/g++.dg/debug/dwarf2/pr101378.C.jj	2021-11-09 15:17:39.504975396 +0100
> +++ gcc/testsuite/g++.dg/debug/dwarf2/pr101378.C	2021-11-09 15:17:28.067137556 +0100
> @@ -0,0 +1,13 @@
> +// PR debug/101378
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-gdwarf-5 -dA" }
> +// { dg-final { scan-assembler-times "0\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> +// { dg-final { scan-assembler-times "1\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> +// { dg-final { scan-assembler-times "2\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> +// { dg-final { scan-assembler-not "-1\[^0-9x\\r\\n\]* DW_AT_data_member_location" } }
> +
> +struct E {};
> +struct S
> +{
> +  [[no_unique_address]] E e, f, g;
> +} s;

	Jakub
  

On Thu, 11 Nov 2021, Jakub Jelinek wrote:

> Hi!
> 
> Bootstrapped/regtested now successfully on x86_64-linux and i686-linux,
> verified the
> struct S { int e; int a : 1, b : 7, c : 8, d : 16; } s;
> struct T { int a : 1, b : 7; long long c : 8; int d : 16; } t;
> 
> int
> main ()
> {
>   s.c = 0x55;
>   s.d = 0xaaaa;
>   t.c = 0x55;
>   t.d = 0xaaaa;
>   s.e++;
> }
> testcase is compiled the same way as before again, ok for trunk?

OK, also for affected branches.

Thanks,
Richard.

> > 2021-11-10  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR debug/101378
> > 	* dwarf2out.c (field_byte_offset): Do the PCC_BITFIELD_TYPE_MATTERS
> > 	handling only for DECL_BIT_FIELD_TYPE decls.
> > 
> > 	* g++.dg/debug/dwarf2/pr101378.C: New test.
> > 
> > --- gcc/dwarf2out.c.jj	2021-11-05 10:19:46.339457342 +0100
> > +++ gcc/dwarf2out.c	2021-11-09 15:01:51.425437717 +0100
> > @@ -19646,6 +19646,7 @@ field_byte_offset (const_tree decl, stru
> >       properly dynamic byte offsets only when PCC bitfield type doesn't
> >       matter.  */
> >    if (PCC_BITFIELD_TYPE_MATTERS
> > +      && DECL_BIT_FIELD_TYPE (decl)
> >        && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
> >      {
> >        offset_int object_offset_in_bits;
> > --- gcc/testsuite/g++.dg/debug/dwarf2/pr101378.C.jj	2021-11-09 15:17:39.504975396 +0100
> > +++ gcc/testsuite/g++.dg/debug/dwarf2/pr101378.C	2021-11-09 15:17:28.067137556 +0100
> > @@ -0,0 +1,13 @@
> > +// PR debug/101378
> > +// { dg-do compile { target c++11 } }
> > +// { dg-options "-gdwarf-5 -dA" }
> > +// { dg-final { scan-assembler-times "0\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> > +// { dg-final { scan-assembler-times "1\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> > +// { dg-final { scan-assembler-times "2\[^0-9x\\r\\n\]* DW_AT_data_member_location" 1 } }
> > +// { dg-final { scan-assembler-not "-1\[^0-9x\\r\\n\]* DW_AT_data_member_location" } }
> > +
> > +struct E {};
> > +struct S
> > +{
> > +  [[no_unique_address]] E e, f, g;
> > +} s;
> 
> 	Jakub
> 
>