From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 4BQkLsDFGmA4WwAAWB0awg (envelope-from ) for ; Wed, 03 Feb 2021 10:48:16 -0500 Received: by simark.ca (Postfix, from userid 112) id AFF3B1EFCD; Wed, 3 Feb 2021 10:48:16 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 9AF871E939 for ; Wed, 3 Feb 2021 10:48:15 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 83CFD393BC00; Wed, 3 Feb 2021 15:48:14 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 83CFD393BC00 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1612367294; bh=QbecLvCLYsOWOxHtvmhAcR7E+Ar2Spg4yZspHMsxsec=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=EdRWPhraEhOBJPBtd4w1ab5AIq4h6H5pzFHjR3AX2VYrZzC4ISjw9c/rMZ4rErgwW Jhy4KfmdGpMPCpHvMEuPQHySkzYGQ9ND+Nz9kuub9ldAXBliZGBSSZaHGbU+syZflG iT977gZuwFlE3RJGzrkCDRqXXMRZef9Lc2hr1/YM= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 165D83857005 for ; Wed, 3 Feb 2021 15:48:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 165D83857005 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 113Fm589010408 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 3 Feb 2021 10:48:10 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 113Fm589010408 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 6FD6E1E939; Wed, 3 Feb 2021 10:48:05 -0500 (EST) Subject: Re: GDB 10.2 release (respin) -- 2021-01-31 Update To: Joel Brobecker References: <20210131064557.GA72834@adacore.com> <20210203055056.GA2717@adacore.com> Message-ID: Date: Wed, 3 Feb 2021 10:48:05 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: <20210203055056.GA2717@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 3 Feb 2021 15:48:05 +0000 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: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2021-02-03 12:50 a.m., Joel Brobecker wrote: > Generally speaking, anything that touches purely on rnglist > seems OK to backport because I understand this support is entirely > new. I'm a little less sure about the changes to liclist support, > though. Is there something I don't know that puts the changes to > loclist handling in the same category as the rnglist changes? I think the loc and rng changes fall in the same category, and kind of mirror of each other. Although there are loclist attributes in DWARF 4, the .debug_loclists section is new to DWARF 5 (DWARF 4 had .debug_loc). In the end, the format of one location list hasn't changed much, but the format of the section (the container of the location lists) changed quite a bit. My fixes have to do with parsing of the container, which is new stuff. And it's pretty much the same with ranges, in DWARF 4, you had .debug_ranges, in DWARF 5 you have .debug_rnglists. My fixes have to do with parsing the headers and finding the right list, not parsing the lists themselves. > Is it possible to skip some patches that are not strictly necessary, > and if yes, would that actually be a good idea? > > With that in mind, my best thoughts on the matter so far: > > [PATCH 01/13] gdb/dwarf: change read_loclist_index complaints into errors > > Although unnecessary, I think this one is fine, and perhaps > even desirable to avoid some weird behavior in GDB... Yes, and if I am not mistaken, this is new DWARF 5-only code to read the DW_FORM_loclistx form, so it shouldn't affect DWARF 4. > > [PATCH 02/13] gdb/dwarf: fix bound check in read_rnglist_index > > OK for gdb-10-branch Same, only affects new DWARF 5 stuff. > > [PATCH 03/13] gdb/dwarf: add missing bound check to read_loclist_index > > Seems straightforward and adds safety; OK for gdb-10-branch. Same. > > [PATCH 04/13] gdb/dwarf: remove unnecessary check in read_{rng,loc}list_index > > Maybe drop this patch, on the basis that this is just > a cleanup that should, in fine, be a no-op in practice. Again, it only affects new DWARF 5 stuff. But indeed it's really not necessary, I'll skip it. > > [PATCH 05/13] gdb/dwarf: few fixes for handling DW_FORM_{rng,loc}listx > > After verification in the DWARF 5 standard that those are > unsigned ULEBs, this one looks good to me for gdb-10-branch. Actually, this patch fixes a problem that was introduced by a refactor that happened after GDB 10, the one that added methods on struct attribute, 529908cbd0af ("Remove DW_UNSND"). So it's not relevant on the 10 branch. > > [PATCH 06/13] gdb/dwarf: read correct rnglist/loclist header in read_{rng, loc}list_index > > I will need to trust you on that one, as I think I would need > to delve more deeply into DWARF 5, and I'm lacking the time > (at least until this weekend). If that makes it feel safer, this code is only on the code path when reading a DW_FORM_rnglistx or DW_FORM_loclistx attribute, which is new DWARF 5 stuff. It should not be invoked when reading DWARF 4. And it is an important one that can't be skipped, it's the main fix of the series. Without it, let's say we have two compilation units, each with a contribution to .debug_rnglists, when reading a DW_FORM_rnglistx attribute from the second compilation unit, we read the header of the .debug_rnglists contribution of the first compilation unit. > [PATCH 07/13] gdb/dwarf: read DW_AT_ranges value as unsigned in partial_die_info::read > > A little scarier for me, but the justification that we are > already doing this in dwarf2_get_pc_bounds is convincing. > OK for gdb-10-branch. This one is also not relavant on the gdb-10-branch, because it fixes a regression also introduced by the "Remove DW_UNSND" commit. > [PATCH 08/13] gdb/testsuite: add .debug_rnglists tests > [PATCH 09/13] gdb/testsuite: DWARF assembler: add context parameters to _location > [PATCH 10/13] gdb/testsuite: add .debug_loclists tests > > No problem for me. Yeah, it's all tests, so it helps ensure that the backport works fine. > [PATCH 11/13] gdb/dwarf: split dwarf2_cu::ranges_base in two > > Do we need this? Looks like you are saying that this is > an enhancement for a case that is unlikely to happen > in practice? Indeed, probably not relevant for a backport since it fixes a corner case not likely to happen. > [PATCH 12/13] gdb/dwarf: make read_{loc, rng}list_index return sect_offset > > OK for gdb-10-branch, with perhaps a question regarding > the gains-vs-risks ratio? I'll skip it, it doesn't change anything in practice. > > [PATCH 13/13] gdb/testsuite: add test for .debug_{rng, loc}lists section without offset array > > No problem for me. > Ok. So this is what I'll push after regtesting a bit: - gdb/dwarf: change read_loclist_index complaints into errors - gdb/dwarf: fix bound check in read_rnglist_index - gdb/dwarf: add missing bound check to read_loclist_index - gdb/dwarf: read correct rnglist/loclist header in read_{rng,loc}list_index - gdb/testsuite: add .debug_rnglists tests - gdb/testsuite: DWARF assembler: add context parameters to _location - gdb/testsuite: add .debug_loclists tests - gdb/testsuite: add test for .debug_{rng,loc}lists section without offset array Thanks! Simon