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

Simon Marchi
2018-06-26T02:37:08Z

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