[v2,3/4] Make sure that sorting does not change section order

  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(-)
  

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.

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.

Simon
  

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
  

On 2018-06-26 01:09, Petr Tesarik wrote:
>> 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

I think you solution is fine, it just needs to be documented that it's 
intentional that we use some other value than the section index in 
sectindex.

Simon