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 <ja...@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
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
