From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 42AA83858435 for ; Tue, 28 Sep 2021 17:55:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 42AA83858435 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-482-ZaXDua0kPhCA40T7HUPJxg-1; Tue, 28 Sep 2021 13:55:13 -0400 X-MC-Unique: ZaXDua0kPhCA40T7HUPJxg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3C1281808319; Tue, 28 Sep 2021 17:55:12 +0000 (UTC) Received: from blarsen.remote.csb (ovpn-116-52.gru2.redhat.com [10.97.116.52]) by smtp.corp.redhat.com (Postfix) with ESMTPS id ED7215D6BA; Tue, 28 Sep 2021 17:55:10 +0000 (UTC) Subject: Re: [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs To: Simon Marchi , gdb-patches@sourceware.org References: <20210830201045.28139-1-blarsen@redhat.com> <4859cd91-257f-091e-1bc8-32a0da68d621@polymtl.ca> <9ef82bc8-db10-ccf4-e675-9efb004646a5@polymtl.ca> <3412b517-638c-a77d-467f-676ca50c2aee@polymtl.ca> From: Bruno Larsen Message-ID: <8bc117ec-1199-b4e9-3c5e-4d849d0a9e84@redhat.com> Date: Tue, 28 Sep 2021 14:55:07 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-7.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP, YOU_INHERIT 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: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Sep 2021 17:55:17 -0000 On 9/24/21 5:59 PM, Simon Marchi wrote: > On 2021-09-24 16:19, Bruno Larsen wrote: >> From what I read in the DWARF spec, if you have a constant >> DW_AT_data_member_location, your inheritance is non-virtual, I guess >> my intent was to turn that A => B into a A <=> B (making it so if you >> have a non-trivial expression, you must have virtual inheritance), but >> you're right, there's nothing on the DWARF specs that say it should be >> so. > > Indeed, all I see is this non-normative text: > > For a C++ virtual base, the data member location attribute will > usually consist of a non-trivial location description. > > The virtuality of base classes is tracked separately, so there doesn't > seem to be a need to tie inheritance virtuality and the location kind. > >> This means there is no way (that I can see at least) to fix the root >> cause or the core dump/assert, which is getting a bogus location for a >> member of the class, other than adding a validation step somewhere, >> that checks if the pointers are actually pointing to inside the base >> class. This patch is the next best thing I could think of, alerting >> the user in a non-destructive manner. > > Well, can't we teach gnuv3_baseclass_offset to evaluate offsets given as > location descriptions even if the inheritance is non-virtual? It seems > to be like that would be a good start. I've spent some time looking at this. This sounds like a trivial fix, because we can re-use what's already there, basically. > > But I think you are right, we probably need some validation / bailing > out somewhere to make sure that the offset makes sense (whether that is > in value_contents_copy_raw as you have done, or somehwere else). Another options for validation is adding a trivial validation on gnuv3_baseclass_offset (like the original one from this patch, checking if the offset is greater than the size of the class), which only work for non-virtual inheritance and returning a bitpos location. I am not familiar with virtual inheritance, much less how GDB deals with it internally, so I don't really know a good way to validate it. A quick google tells me that my validation on value_contents_copy_raw could assume that a problem has happened when we are just copying a virtual inheritance member. A better spot to put it could be cp_print_value, since we still have virtuality information at that point, and can validate differently at that point. The trivial validation fixes the second case, but not the first, so we still need the second validation. This still doesn't sound perfect to me, but I can start work on a v3 at least. Thoughts? -- Cheers! Bruno Larsen