From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 79713 invoked by alias); 19 Feb 2020 03:42:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 79691 invoked by uid 89); 19 Feb 2020 03:42:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=meat, mid X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Feb 2020 03:42:27 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D96A21E47D; Tue, 18 Feb 2020 22:42:23 -0500 (EST) Subject: Re: [PATCH 01/14] Fix latent bug in dwarf2_find_containing_comp_unit To: Tom Tromey , gdb-patches@sourceware.org References: <20200215165444.32653-1-tom@tromey.com> <20200215165444.32653-2-tom@tromey.com> From: Simon Marchi Message-ID: <01167589-fae4-39a0-1ebe-0c7d4cbf52b5@simark.ca> Date: Wed, 19 Feb 2020 03:42:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.2 MIME-Version: 1.0 In-Reply-To: <20200215165444.32653-2-tom@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2020-02/txt/msg00757.txt.bz2 On 2020-02-15 11:54 a.m., Tom Tromey wrote: > dwarf2_find_containing_comp_unit has this in its binary search: > > if (mid_cu->is_dwz > offset_in_dwz > || (mid_cu->is_dwz == offset_in_dwz > && mid_cu->sect_off + mid_cu->length >= sect_off)) > high = mid; > > The intent here is to determine whether SECT_OFF appears in or before > MID_CU. > > I believe this has an off-by-one error, and that the check should use > ">" rather than ">=". If the two side are equal, then SECT_OFF > actually appears at the start of the next CU. > > I've had this patch kicking around for ages but I forget how I found > the problem. > > gdb/ChangeLog > 2020-02-15 Tom Tromey > > * dwarf2/read.c (dwarf2_find_containing_comp_unit): Use ">", not > ">=", in binary search. > --- > gdb/ChangeLog | 5 +++++ > gdb/dwarf2/read.c | 2 +- > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index e74383e01dc..e373cc0af2c 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -24171,7 +24171,7 @@ dwarf2_find_containing_comp_unit (sect_offset sect_off, > mid_cu = dwarf2_per_objfile->all_comp_units[mid]; > if (mid_cu->is_dwz > offset_in_dwz > || (mid_cu->is_dwz == offset_in_dwz > - && mid_cu->sect_off + mid_cu->length >= sect_off)) > + && mid_cu->sect_off + mid_cu->length > sect_off)) > high = mid; > else > low = mid + 1; I can only convince myself of these things by plugging in real numbers. So let's say mid_cu's range is [100,150[, so 150 bytes long, and we are looking for sect_off == 150. 150 is outside (just after) mid_cu's range, so the right answer will be the cu just after this one. Without your change, we would have gone in the "if", and therefore excluded the right answer. With your change, we would have gone in the "else", and therefore chosen the range with the right answer. So that looks correct to me. I'm going to ask if you thought about a way to test this. I don't think writing a typical test case for this is feasible. By refactoring the code a bit, we could maybe factor out the meat of this function into one that operates on an std::vector instead of on a dwarf2_per_objfile. It should then be feasible to create an std::vector with dwarf2_per_cu_data elements out of thin air to unit-test the function, including edge cases like this. Also, do you understand what's the logic with this dwz stuff? Is it that all the CUs coming from a dwz file are at the end of the list, or something like that? Simon