Re: [PATCH v2 3/4] Make sure that sorting does not change section order

[Petr Tesarik <ptesarik@suse.cz>]

On Mon, 25 Jun 2018 22:36:58 -0400
Simon Marchi <simon.marchi@polymtl.ca> 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  <ptesarik@suse.com>
> > 
> > 	* 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  <ptesarik@suse.com>
> > 
> > +	* 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  <ptesarik@suse.com>
> > +
> >  	* 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