From patchwork Thu Feb 20 00:12:13 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom Tromey X-Patchwork-Id: 38249 Received: (qmail 93029 invoked by alias); 20 Feb 2020 00:12:19 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 92934 invoked by uid 89); 20 Feb 2020 00:12:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=locate X-HELO: gateway34.websitewelcome.com Received: from gateway34.websitewelcome.com (HELO gateway34.websitewelcome.com) (192.185.148.212) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 20 Feb 2020 00:12:16 +0000 Received: from cm10.websitewelcome.com (cm10.websitewelcome.com [100.42.49.4]) by gateway34.websitewelcome.com (Postfix) with ESMTP id 4FBD6735B9F for ; Wed, 19 Feb 2020 18:12:14 -0600 (CST) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id 4ZRujEnieEfyq4ZRujMWOk; Wed, 19 Feb 2020 18:12:14 -0600 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=y9PndNDdQdVKnM8RL/qL4XdKwEVoTtkeBiP9P/LGb78=; b=iSl4/BkXscu/nmxr9UpqwtiO2o l/+T5iO1sBSMDCqYepa2lN9id2MGZ+awLKahUcgt0wplXaM5VwqIdTx9iYh5nQFG3+kjeSf4wCU+i 2KU7l26OfQyuOyBGb2mTgxLsO; Received: from 75-166-123-50.hlrn.qwest.net ([75.166.123.50]:46740 helo=bapiya) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92) (envelope-from ) id 1j4ZRu-003Vdn-2X; Wed, 19 Feb 2020 17:12:14 -0700 From: Tom Tromey To: Tom Tromey Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 01/14] Fix latent bug in dwarf2_find_containing_comp_unit References: <20200215165444.32653-1-tom@tromey.com> <20200215165444.32653-2-tom@tromey.com> <01167589-fae4-39a0-1ebe-0c7d4cbf52b5@simark.ca> <87o8tu3c1g.fsf@tromey.com> Date: Wed, 19 Feb 2020 17:12:13 -0700 In-Reply-To: <87o8tu3c1g.fsf@tromey.com> (Tom Tromey's message of "Wed, 19 Feb 2020 07:08:11 -0700") Message-ID: <87o8tu86ci.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) MIME-Version: 1.0 Simon> By refactoring the code a bit, we could maybe factor out the Simon> meat of this function into one that operates on an std::vector instead Simon> of on a dwarf2_per_objfile. It should then be feasible to create an std::vector with Simon> dwarf2_per_cu_data elements out of thin air to unit-test the function, including edge Simon> cases like this. Oops, I meant to attach the patch in that last email. Let me know what you think. Tom commit 5d02b8ad013451b03e004ef1e71c4636b43252c2 Author: Tom Tromey Date: Sat Feb 15 09:08:09 2020 -0700 Fix latent bug in dwarf2_find_containing_comp_unit 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-19 Tom Tromey * dwarf2/read.c (dwarf2_find_containing_comp_unit): Use ">", not ">=", in binary search. (dwarf2_find_containing_comp_unit): New overload. (run_test): New self-test. (_initialize_dwarf2_read): Register new test. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index b4a586c333a..b07c9156e28 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,11 @@ +2020-02-19 Tom Tromey + + * dwarf2/read.c (dwarf2_find_containing_comp_unit): Use ">", not + ">=", in binary search. + (dwarf2_find_containing_comp_unit): New overload. + (run_test): New self-test. + (_initialize_dwarf2_read): Register new test. + 2020-02-19 Simon Marchi * dwarf2/read.c (allocate_signatured_type_table, diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 4d767a59af7..f998fe6b8d0 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -24136,34 +24136,53 @@ dwarf2_per_cu_data::addr_type () const return addr_type; } -/* Locate the .debug_info compilation unit from CU's objfile which contains - the DIE at OFFSET. Raises an error on failure. */ +/* A helper function for dwarf2_find_containing_comp_unit that returns + the index of the result, and that searches a vector. It will + return a result even if the offset in question does not actually + occur in any CU. This is separate so that it can be unit + tested. */ -static struct dwarf2_per_cu_data * -dwarf2_find_containing_comp_unit (sect_offset sect_off, - unsigned int offset_in_dwz, - struct dwarf2_per_objfile *dwarf2_per_objfile) +static int +dwarf2_find_containing_comp_unit + (sect_offset sect_off, + unsigned int offset_in_dwz, + const std::vector &all_comp_units) { - struct dwarf2_per_cu_data *this_cu; int low, high; low = 0; - high = dwarf2_per_objfile->all_comp_units.size () - 1; + high = all_comp_units.size () - 1; while (high > low) { struct dwarf2_per_cu_data *mid_cu; int mid = low + (high - low) / 2; - mid_cu = dwarf2_per_objfile->all_comp_units[mid]; + mid_cu = 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; } gdb_assert (low == high); - this_cu = dwarf2_per_objfile->all_comp_units[low]; + return low; +} + +/* Locate the .debug_info compilation unit from CU's objfile which contains + the DIE at OFFSET. Raises an error on failure. */ + +static struct dwarf2_per_cu_data * +dwarf2_find_containing_comp_unit (sect_offset sect_off, + unsigned int offset_in_dwz, + struct dwarf2_per_objfile *dwarf2_per_objfile) +{ + int low + = dwarf2_find_containing_comp_unit (sect_off, offset_in_dwz, + dwarf2_per_objfile->all_comp_units); + struct dwarf2_per_cu_data *this_cu + = dwarf2_per_objfile->all_comp_units[low]; + if (this_cu->is_dwz != offset_in_dwz || this_cu->sect_off > sect_off) { if (low == 0 || this_cu->is_dwz != offset_in_dwz) @@ -24186,6 +24205,57 @@ dwarf2_find_containing_comp_unit (sect_offset sect_off, } } +#if GDB_SELF_TEST + +namespace selftests { +namespace find_containing_comp_unit { + +static void +run_test () +{ + struct dwarf2_per_cu_data one {}; + struct dwarf2_per_cu_data two {}; + struct dwarf2_per_cu_data three {}; + struct dwarf2_per_cu_data four {}; + + one.length = 5; + two.sect_off = sect_offset (one.length); + two.length = 7; + + three.length = 5; + three.is_dwz = 1; + four.sect_off = sect_offset (three.length); + four.length = 7; + four.is_dwz = 1; + + std::vector units; + units.push_back (&one); + units.push_back (&two); + units.push_back (&three); + units.push_back (&four); + + int result; + + result = dwarf2_find_containing_comp_unit (sect_offset (0), 0, units); + SELF_CHECK (units[result] == &one); + result = dwarf2_find_containing_comp_unit (sect_offset (3), 0, units); + SELF_CHECK (units[result] == &one); + result = dwarf2_find_containing_comp_unit (sect_offset (5), 0, units); + SELF_CHECK (units[result] == &two); + + result = dwarf2_find_containing_comp_unit (sect_offset (0), 1, units); + SELF_CHECK (units[result] == &three); + result = dwarf2_find_containing_comp_unit (sect_offset (3), 1, units); + SELF_CHECK (units[result] == &three); + result = dwarf2_find_containing_comp_unit (sect_offset (5), 1, units); + SELF_CHECK (units[result] == &four); +} + +} +} + +#endif /* GDB_SELF_TEST */ + /* Initialize dwarf2_cu CU, owned by PER_CU. */ dwarf2_cu::dwarf2_cu (struct dwarf2_per_cu_data *per_cu_) @@ -24690,5 +24760,7 @@ Warning: This option must be enabled before gdb reads the file."), #if GDB_SELF_TEST selftests::register_test ("dw2_expand_symtabs_matching", selftests::dw2_expand_symtabs_matching::run_test); + selftests::register_test ("dwarf2_find_containing_comp_unit", + selftests::find_containing_comp_unit::run_test); #endif }