From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 62656 invoked by alias); 26 Jun 2018 05:10:28 -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 59363 invoked by uid 89); 26 Jun 2018 05:10:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 spammy=dozen X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 26 Jun 2018 05:10:26 +0000 Received: from relay1.suse.de (charybdis-ext-too.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 272BDABDD; Tue, 26 Jun 2018 05:10:24 +0000 (UTC) Date: Tue, 26 Jun 2018 05:10:00 -0000 From: Petr Tesarik To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2 3/4] Make sure that sorting does not change section order Message-ID: <20180626070943.7331c1d2@ezekiel.suse.cz> In-Reply-To: <2a43d96e7d7c435bb9344dc1825e5be8@polymtl.ca> References: <20180611120835.27343-1-ptesarik@suse.cz> <20180611120835.27343-4-ptesarik@suse.cz> <2a43d96e7d7c435bb9344dc1825e5be8@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-06/txt/msg00598.txt.bz2 On Mon, 25 Jun 2018 22:36:58 -0400 Simon Marchi wrote: > On 2018-06-11 08:08, Petr Tesarik wrote: > > Symbol files may contain multiple sections with the same name. > > Section addresses specified add-symbol-file are assigned to the > > corresponding BFD sections in addr_info_make_relative using sorted > > indexes of both vectors. Since the sort algorithm is not inherently > > stable, the comparison function uses sectindex to maintain the > > original order. However, add_symbol_file_command uses zero for all > > sections, so if the user specifies multiple sections with the same > > name, they will be assigned randomly to symbol file sections with > > the same name. > > > > gdb/ChangeLog: > > 2018-06-11 Petr Tesarik > > > > * symfile.c (add_symbol_file_command): Make sure that sections > > with the same name are sorted in the same order. > > --- > > gdb/ChangeLog | 5 +++++ > > gdb/symfile.c | 2 +- > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > > index 1c5a1f6bfb..0f75992d4c 100644 > > --- a/gdb/ChangeLog > > +++ b/gdb/ChangeLog > > @@ -1,5 +1,10 @@ > > 2018-06-11 Petr Tesarik > > > > + * symfile.c (add_symbol_file_command): Make sure that sections > > + with the same name are sorted in the same order. > > + > > +2018-06-11 Petr Tesarik > > + > > * symfile.c (add_symbol_file_command, _initialize_symfile): Do not > > require the second argument. If omitted, load sections at the > > addresses specified in the file. > > diff --git a/gdb/symfile.c b/gdb/symfile.c > > index 3e3ab20412..8b8b194334 100644 > > --- a/gdb/symfile.c > > +++ b/gdb/symfile.c > > @@ -2185,7 +2185,7 @@ add_symbol_file_command (const char *args, int > > from_tty) > > > > /* Here we store the section offsets in the order they were > > entered on the command line. */ > > - section_addrs.emplace_back (addr, sec, 0); > > + section_addrs.emplace_back (addr, sec, section_addrs.size ()); > > printf_unfiltered ("\t%s_addr = %s\n", sec, > > paddress (gdbarch, addr)); > > It took me a while to acknowledge that this was correct, because > other_sections::sectindex usually refers to the section index in the > BFD. After digging I understood that this field was actually unused > until filled by addr_info_make_relative, and that you kind of > re-purposed it. It sounds like there should be some comment at > other_sections::sectindex and probably in add_symbol_file_command to > explain how it's used. Agreed. As a matter of fact, it also took me some while to understand why add_symbol_file_command could get away with setting the index to zero for all sections... > Another option would be to use std::stable_sort instead of std::sort. > But it's more resource-hungry and not needed for all paths that lead to > addrs_section_sort, so it would be a bit wasteful. Yes, I tried to avoid that solution. OTOH it's unlikely that there are any object files with more than a few dozen sections, and to my best knowledge this code is never in the GDB hot path, so if you prefer std::stable_sort for clarity, I'm not against. Please, advise. Petr T