Message ID | 20211110092042.GE2710@tucnak |
---|---|
State | Committed |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 158BA3857C40 for <patchwork@sourceware.org>; Wed, 10 Nov 2021 09:24:03 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 158BA3857C40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1636536243; bh=iUeUw2De3dcbSePIbrX/l77XagDbtC2ueKqoNIEIglw=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=QH7tMmAggVXP1I6m6ZTgxC+P+deoUoiSnGXmFB/eALoidvvX0kXGBGxfOTsCtsbnk 2e+jtPhZP72HjpSbOITINbQ87QI+EBz4YSCoZq69EqX0YqdnhB325DTu+O9gkr6kza Z3AOUIpsWu8tMHR82gWqXn7KGgmc13mFdtUXx7M8= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTPS id 2FDAC3857C69 for <gcc-patches@gcc.gnu.org>; Wed, 10 Nov 2021 09:20:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2FDAC3857C69 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-491-ucMyV2VDNGWzATHa-3pOQQ-1; Wed, 10 Nov 2021 04:20:48 -0500 X-MC-Unique: ucMyV2VDNGWzATHa-3pOQQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 16A81425C9; Wed, 10 Nov 2021 09:20:47 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.54]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8FE371007605; Wed, 10 Nov 2021 09:20:46 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 1AA9Kh5q803437 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 10 Nov 2021 10:20:43 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 1AA9KgVU803436; Wed, 10 Nov 2021 10:20:42 +0100 Date: Wed, 10 Nov 2021 10:20:42 +0100 To: Jason Merrill <jason@redhat.com>, Richard Biener <rguenther@suse.de>, Eric Botcazou <botcazou@adacore.com> Subject: [PATCH] dwarf2out: Fix up field_byte_offset [PR101378] Message-ID: <20211110092042.GE2710@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-5.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jakub Jelinek <jakub@redhat.com> Cc: gcc-patches@gcc.gnu.org Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
dwarf2out: Fix up field_byte_offset [PR101378]
|
|
Commit Message
Jakub Jelinek
Nov. 10, 2021, 9:20 a.m. UTC
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
Comments
> 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 > >
--- 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) { 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;