[patch] Fix separate-debug with non-unique section names (PR 11409)

[patch] Fix separate-debug with non-unique section names (PR 11409)

[Jan Kratochvil]

Hi,

gdb-7.1 is now broken for example for debugging /usr/bin/emacs due to:
http://sourceware.org/bugzilla/show_bug.cgi?id=11409
  [22] .data   PROGBITS 00000000007fe8a0 1fe8a0 215068 00  WA  0   0 32
  [23] .data   PROGBITS 0000000000a13920 413920 68c6e0 00  WA  0   0 32

It is in fact a regression against gdb-7.0 by me due to:

commit 71d0069a9f238a11f7f455bf6ad2adfc25683521
Author: Jan Kratochvil <jan.kratochvil@redhat.com>
Date:   Tue Jan 5 15:51:01 2010 +0000

gdb/
        * symfile.c (syms_from_objfile): Remove the !MAINLINE conditional.

as while the code was broken even before the broken relocation was not applied
to mainline binary (before PIE+OSX patches went in).


No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.

OK to check-in also for gdb-7.1 (7.1.1)?


Thanks,
Jan


gdb/
2010-03-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* symfile.c (addr_info_make_relative): Move sect declaration to the
	outer block.  Initialize it to NULL.  Prefer SECT->next more than
	bfd_get_section_by_name.

gdb/testsuite/
2010-03-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/dup-sect.exp, gdb.base/dup-sect.S: New.

--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -529,6 +529,7 @@ addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
   asection *lower_sect;
   CORE_ADDR lower_offset;
   int i;
+  asection *sect;
 
   /* Find lowest loadable section to be used as starting point for
      continguous sections.  */
@@ -553,11 +554,23 @@ addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
      (the loadable section directly below it in memory).
      this_offset = lower_offset = lower_addr - lower_orig_addr */
 
+  sect = NULL;
   for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
     {
       const char *sect_name = addrs->other[i].name;
-      asection *sect = bfd_get_section_by_name (abfd, sect_name);
 
+      /* Prefer the next section of that we have found last.  The separate
+	 debug info files have either the same section layout or just a few
+	 sections are missing there.  On the other hand the section name is not
+	 unique and we could find an inappropraite section by its name.  */
+
+      if (sect)
+	sect = sect->next;
+      if (sect && strcmp (sect_name, bfd_get_section_name (abfd, sect)) != 0)
+	sect = NULL;
+
+      if (sect == NULL)
+	sect = bfd_get_section_by_name (abfd, sect_name);
       if (sect)
 	{
 	  /* This is the index used by BFD. */
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dup-sect.S
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.section	sect1, "a"
+var1:	.byte	1
+
+	.section	sect2, "a"
+var2:	.byte	2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dup-sect.exp
@@ -0,0 +1,79 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test inappropriate offseting of multiple sections with the same name.
+# When kept in object file (before final executable link) it still works.
+# When separate debug info file is not used it still works.
+# When the ELF symbol table is kept in the main binary it still works.
+# Used .S file as in .c file we would need __attriute__((section)) which is
+# a GCC extension.
+
+# This test can only be run on targets which support ELF and use gas.
+# For now pick a sampling of likely targets.
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0
+}
+
+set testfile dup-sect
+set srcfile ${testfile}.S
+set srcmainfile start.c
+set executable ${testfile}
+set binfile ${objdir}/${subdir}/${executable}
+
+if {[build_executable ${testfile}.exp $executable [list ${srcfile} ${srcmainfile}] {}] == -1} {
+    return -1
+}
+
+set test "rename section"
+set objcopy_program [transform objcopy]
+set result [catch "exec $objcopy_program --rename-section sect2=sect1 $binfile" output]
+verbose "result is $result"
+verbose "output is $output"
+if {$result != 0} {
+    fail $test
+    return
+}
+pass $test
+
+set test "split"
+if {[gdb_gnu_strip_debug $binfile] != 0} {
+    fail $test
+} else {
+    pass $test
+}
+
+# gdb_gnu_strip_debug uses only --strip-debug and keeps the ELF symbol table
+# in $binfile.
+set test "strip"
+set strip_program [transform strip]
+set result [catch "exec $strip_program $binfile" output]
+verbose "result is $result"
+verbose "output is $output"
+if {$result != 0} {
+    fail $test
+    return
+}
+pass $test
+
+clean_restart $executable
+
+gdb_test "p/d *(const char *) &var1" " = 1" "var1 after strip"
+gdb_test "p/d *(const char *) &var2" " = 2" "var2 after strip"

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> gdb-7.1 is now broken for example for debugging /usr/bin/emacs due to:
Jan> http://sourceware.org/bugzilla/show_bug.cgi?id=11409
Jan>   [22] .data   PROGBITS 00000000007fe8a0 1fe8a0 215068 00  WA  0   0 32
Jan>   [23] .data   PROGBITS 0000000000a13920 413920 68c6e0 00  WA  0   0 32

Jan> 2010-03-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	* symfile.c (addr_info_make_relative): Move sect declaration to the
Jan> 	outer block.  Initialize it to NULL.  Prefer SECT->next more than
Jan> 	bfd_get_section_by_name.

This patch seems to assume that all sections of the same name will be
next to each other in the section_addr_info.  Why is it ok to make this
assumption?

Tom

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Wed, 24 Mar 2010 13:02:07 -0600
> 
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> gdb-7.1 is now broken for example for debugging /usr/bin/emacs due to:
> Jan> http://sourceware.org/bugzilla/show_bug.cgi?id=11409
> Jan>   [22] .data   PROGBITS 00000000007fe8a0 1fe8a0 215068 00  WA  0   0 32
> Jan>   [23] .data   PROGBITS 0000000000a13920 413920 68c6e0 00  WA  0   0 32
> 
> Jan> 2010-03-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
> Jan> 	* symfile.c (addr_info_make_relative): Move sect declaration to the
> Jan> 	outer block.  Initialize it to NULL.  Prefer SECT->next more than
> Jan> 	bfd_get_section_by_name.
> 
> This patch seems to assume that all sections of the same name will be
> next to each other in the section_addr_info.  Why is it ok to make this
> assumption?

That assumption is false, at least in general.  I have an
OpenBSD/mips64el emacs binary that has two .data sections that aren't
adjacent.

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Jan Kratochvil]

On Wed, 24 Mar 2010 20:02:07 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> gdb-7.1 is now broken for example for debugging /usr/bin/emacs due to:
> Jan> http://sourceware.org/bugzilla/show_bug.cgi?id=11409
> Jan>   [22] .data   PROGBITS 00000000007fe8a0 1fe8a0 215068 00  WA  0   0 32
> Jan>   [23] .data   PROGBITS 0000000000a13920 413920 68c6e0 00  WA  0   0 32
> 
> Jan> 2010-03-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
> Jan> 	* symfile.c (addr_info_make_relative): Move sect declaration to the
> Jan> 	outer block.  Initialize it to NULL.  Prefer SECT->next more than
> Jan> 	bfd_get_section_by_name.
> 
> This patch seems to assume that all sections of the same name will be
> next to each other in the section_addr_info.  Why is it ok to make this
> assumption?

The code generally assumes the sections in .debug file are in the same order
as in the main file.  Only if there is some discrepancy it will resync by
bfd_get_section_by_name.  The resync should not normally happen.  Attached an
illustrative /bin/bash -> /usr/lib/debug/bin/bash.debug section diff.  The
only resynchronizations happen there on system part of sections - and these
have unique name.

The problem is I do not know how to match sections main<->.debug file when
their name is not unique.  One could check VMAs but - to find the difference
of VMAs is the goal of this function.

Assuming for executables + shared libraries there are never many sections
there.  This emacs .data section duplicity is more an exception.  The only
cases with many (thousands of) sections I am aware of are object files but
object files should get into addr_info_make_relative at all (I hope the Mach-O
support does not use it for the .o symbols loading).

ld --split-by-file=1 creates section names like .text.0 ... .text.1454 all
unique; by bfd_get_unique_section_name.

While the algorithm could be much more clever - if we ever need to resync at
the point of non-unique section names the files are too different it makes no
sence to try to match them by addr_info_make_relative at all.

I may have forgot about this case, not sure, but I do not find it a problem.


Thanks,
Jan


-------------------
 .interp
 .note.ABI-tag
 .note.gnu.build-id
 .gnu.hash
 .dynsym
-.gnu.liblist
-.gnu.conflict
+.dynstr
 .gnu.version
 .gnu.version_r
 .rela.dyn
 .rela.plt
 .init
 .plt
 .text
 .fini
 .rodata
 .eh_frame_hdr
 .eh_frame
 .ctors
 .dtors
 .jcr
 .dynamic
 .got
 .got.plt
 .data
-.dynbss
 .bss
-.dynstr
-.gnu_debuglink
-.gnu.prelink_undo
+.comment
+.debug_aranges
+.debug_pubnames
+.debug_info
+.debug_abbrev
+.debug_line
+.debug_str
+.debug_loc
+.debug_pubtypes
+.debug_ranges
 .shstrtab
+.symtab
+.strtab

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Daniel Jacobowitz]

On Wed, Mar 24, 2010 at 09:09:43PM +0100, Jan Kratochvil wrote:
> While the algorithm could be much more clever - if we ever need to resync at
> the point of non-unique section names the files are too different it makes no
> sence to try to match them by addr_info_make_relative at all.

What if we assume that in a valid separate debug file, the only
sections that will be different will have been totally stripped from
either the before or after file?

IOW:

  i = 0 // index in main file
  j = 0 // index in separate file
  for each section in main file:
    if (section names match)
      good
    else if (section i's name is not present in separate file)
      i++, continue
    else if (section j's name is not present in separate file)
      j++, continue
    else
      complain that the two files do not match

?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Jan Kratochvil]

On Wed, 24 Mar 2010 21:18:31 +0100, Daniel Jacobowitz wrote:
> On Wed, Mar 24, 2010 at 09:09:43PM +0100, Jan Kratochvil wrote:
> > While the algorithm could be much more clever - if we ever need to resync at
> > the point of non-unique section names the files are too different it makes no
> > sence to try to match them by addr_info_make_relative at all.
> 
> What if we assume that in a valid separate debug file, the only
> sections that will be different will have been totally stripped from
> either the before or after file?
> 
> IOW:
> 
>   i = 0 // index in main file
>   j = 0 // index in separate file
>   for each section in main file:
>     if (section names match)
>       good
>     else if (section i's name is not present in separate file)
>       i++, continue
>     else if (section j's name is not present in separate file)
                                                  or main?
>       j++, continue
>     else
>       complain that the two files do not match
> 
> ?

.dynstr is present in both files and on different places from my real
prelinked /bin/bash example.


#! /usr/bin/perl
use strict; use warnings;
my @main=qw(.interp .note.ABI-tag .note.gnu.build-id .gnu.hash .dynsym .gnu.liblist .gnu.conflict .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame .ctors .dtors .jcr .dynamic .got .got.plt .data .dynbss .bss .dynstr .gnu_debuglink .gnu.prelink_undo .shstrtab);
my @debug=qw(.interp .note.ABI-tag .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame .ctors .dtors .jcr .dynamic .got .got.plt .data .bss .comment .debug_aranges .debug_pubnames .debug_info .debug_abbrev .debug_line .debug_str .debug_loc .debug_pubtypes .debug_ranges .shstrtab .symtab .strtab);
my $i = 0; # index in main file
my $j = 0; # index in separate file
while ($i < @main || $j < @debug) {
  $i++,$j++,next if $main[$i] eq $debug[$j];
  # section i's name is not present in separate file
  $i++,next if !grep /^\Q$main[$i]\E$/,@debug;
  # section j's name is not present in separate file
  $j++,next if !grep /^\Q$debug[$j]\E$/,@debug;
  die "complain that the two files do not match: $i=$main[$i] $j=$debug[$j]\n";
}
print "OK\n";
->
complain that the two files do not match: 7=.gnu.version 5=.dynstr


Not sure how this algorithm could be improved to cope with it.


Thanks,
Jan

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> complain that the two files do not match: 7=.gnu.version 5=.dynstr

This seems to undermine the assumption that sections in the .debug file
are in the same order as in the main file.

Jan> Not sure how this algorithm could be improved to cope with it.

Can we stably sort the sections by name, using the initial index as a
secondary key?

Tom

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Daniel Jacobowitz]

On Wed, Mar 24, 2010 at 09:32:52PM +0100, Jan Kratochvil wrote:
> >     else if (section j's name is not present in separate file)
>                                                   or main?
> >       j++, continue

You're right, I meant main.  Even with that in your perl script fixed,
it still won't work.

> .dynstr is present in both files and on different places from my real
> prelinked /bin/bash example.

Maybe this happened because prelink does special magic with dynamic
sections to create space.  I don't understand what else could have
done it.

I don't know how to correlate two lists of sections that are only mostly
the same.  I guess, you could add a fallback to the algorithm I
proposed: if each file only has one section with that name, use it,
and otherwise skip the section.  We're really guessing at this point though!

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Jan Kratochvil]

On Wed, 24 Mar 2010 21:37:34 +0100, Tom Tromey wrote:
> Can we stably sort the sections by name, using the initial index as a
> secondary key?

OK, attached.

build_section_addr_info_from_objfile could also be kept intact (just creating
new build_section_addr_info_from_bfd).

While there is some new code I have not found easier way.  Also the complexity
is now reduced (n^2 -> n*log(n)); not that it would matter.

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


gdb/
2010-03-25  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* symfile.c (build_section_addr_info_from_bfd): New.
	(build_section_addr_info_from_objfile): Base it on
	build_section_addr_info_from_bfd.
	(addrs_section_compar, addrs_section_sort): New.
	(addr_info_make_relative): New variables my_cleanup, abfd_addrs,
	addrs_sorted, abfd_addrs_sorted and addrs_to_abfd_addrs.  Build
	addrs_to_abfd_addrs.  Use it for recalculating ADDRS.

gdb/testsuite/
2010-03-25  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/dup-sect.exp, gdb.base/dup-sect.S: New.

--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -319,29 +319,47 @@ build_section_addr_info_from_section_table (const struct target_section *start,
   return sap;
 }
 
-/* Create a section_addr_info from section offsets in OBJFILE.  */
+/* Create a section_addr_info from section offsets in ABFD.  */
 
-struct section_addr_info *
-build_section_addr_info_from_objfile (const struct objfile *objfile)
+static struct section_addr_info *
+build_section_addr_info_from_bfd (bfd *abfd)
 {
   struct section_addr_info *sap;
   int i;
   struct bfd_section *sec;
 
-  sap = alloc_section_addr_info (objfile->num_sections);
-  for (i = 0, sec = objfile->obfd->sections; sec != NULL; sec = sec->next)
-    if (bfd_get_section_flags (objfile->obfd, sec) & (SEC_ALLOC | SEC_LOAD))
+  sap = alloc_section_addr_info (bfd_count_sections (abfd));
+  for (i = 0, sec = abfd->sections; sec != NULL; sec = sec->next)
+    if (bfd_get_section_flags (abfd, sec) & (SEC_ALLOC | SEC_LOAD))
       {
-	sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
-			      + objfile->section_offsets->offsets[i]);
-	sap->other[i].name = xstrdup (bfd_get_section_name (objfile->obfd,
-							    sec));
+	sap->other[i].addr = bfd_get_section_vma (abfd, sec);
+	sap->other[i].name = xstrdup (bfd_get_section_name (abfd, sec));
 	sap->other[i].sectindex = sec->index;
 	i++;
       }
   return sap;
 }
 
+/* Create a section_addr_info from section offsets in OBJFILE.  */
+
+struct section_addr_info *
+build_section_addr_info_from_objfile (const struct objfile *objfile)
+{
+  struct section_addr_info *sap;
+  int i;
+
+  /* Before reread_symbols gets rewritten it is not safe to call:
+     gdb_assert (objfile->num_sections == bfd_count_sections (objfile->obfd));
+     */
+  sap = build_section_addr_info_from_bfd (objfile->obfd);
+  for (i = 0; i < sap->num_sections && sap->other[i].name; i++)
+    {
+      int sectindex = sap->other[i].sectindex;
+
+      sap->other[i].addr += objfile->section_offsets->offsets[sectindex];
+    }
+  return sap;
+}
 
 /* Free all memory allocated by build_section_addr_info_from_section_table. */
 
@@ -519,6 +537,46 @@ relative_addr_info_to_section_offsets (struct section_offsets *section_offsets,
     }
 }
 
+/* qsort comparator for addrs_section_sort.  Sort entries in ascending order by
+   their (name, sectindex) pair.  sectindex makes the sort by name stable.  */
+
+static int
+addrs_section_compar (const void *ap, const void *bp)
+{
+  const struct other_sections *a = *((struct other_sections **) ap);
+  const struct other_sections *b = *((struct other_sections **) bp);
+  int retval, a_idx, b_idx;
+
+  retval = strcmp (a->name, b->name);
+  if (retval)
+    return retval;
+
+  /* SECTINDEX is underfined iff ADDR is zero.  */
+  a_idx = a->addr == 0 ? 0 : a->sectindex;
+  b_idx = b->addr == 0 ? 0 : b->sectindex;
+  return a_idx - b_idx;
+}
+
+/* Provide sorted array of pointers to sections of ADDRS.  The array is
+   terminated by NULL.  Caller is responsible to call xfree for it.  */
+
+static struct other_sections **
+addrs_section_sort (struct section_addr_info *addrs)
+{
+  struct other_sections **array;
+  int i;
+
+  /* `+ 1' for the NULL terminator.  */
+  array = xmalloc (sizeof (*array) * (addrs->num_sections + 1));
+  for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
+    array[i] = &addrs->other[i];
+  array[i] = NULL;
+
+  qsort (array, i, sizeof (*array), addrs_section_compar);
+
+  return array;
+}
+
 /* Relativize absolute addresses in ADDRS into offsets based on ABFD.  Fill-in
    also SECTINDEXes specific to ABFD there.  This function can be used to
    rebase ADDRS to start referencing different BFD than before.  */
@@ -529,6 +587,10 @@ addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
   asection *lower_sect;
   CORE_ADDR lower_offset;
   int i;
+  struct cleanup *my_cleanup;
+  struct section_addr_info *abfd_addrs;
+  struct other_sections **addrs_sorted, **abfd_addrs_sorted;
+  struct other_sections **addrs_to_abfd_addrs;
 
   /* Find lowest loadable section to be used as starting point for
      continguous sections.  */
@@ -543,6 +605,55 @@ addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
   else
     lower_offset = bfd_section_vma (bfd_get_filename (abfd), lower_sect);
 
+  /* Create ADDRS_TO_ABFD_ADDRS array to map the sections in ADDRS to sections
+     in ABFD.  Section names are not unique - there can be multiple sections of
+     the same name.  Also the sections of the same name do not have to be
+     adjacent to each other.  Some sections may be present only in one of the
+     files.  Even sections present in both files do not have to be in the same
+     order.
+
+     Use stable sort by name for the sections in both files.  Then linearly
+     scan both lists matching as most of the entries as possible.  */
+
+  addrs_sorted = addrs_section_sort (addrs);
+  my_cleanup = make_cleanup (xfree, addrs_sorted);
+
+  abfd_addrs = build_section_addr_info_from_bfd (abfd);
+  make_cleanup_free_section_addr_info (abfd_addrs);
+  abfd_addrs_sorted = addrs_section_sort (abfd_addrs);
+  make_cleanup (xfree, abfd_addrs_sorted);
+
+  /* Now create ADDRS_TO_ABFD_ADDRS from ADDRS_SORTED and ABFD_ADDRS_SORTED.  */
+
+  addrs_to_abfd_addrs = xzalloc (sizeof (*addrs_to_abfd_addrs)
+				 * addrs->num_sections);
+  make_cleanup (xfree, addrs_to_abfd_addrs);
+
+  while (*addrs_sorted)
+    {
+      const char *sect_name = (*addrs_sorted)->name;
+
+      while (*abfd_addrs_sorted
+	     && strcmp ((*abfd_addrs_sorted)->name, sect_name) < 0)
+	abfd_addrs_sorted++;
+
+      if (*abfd_addrs_sorted
+	  && strcmp ((*abfd_addrs_sorted)->name, sect_name) == 0)
+	{
+	  int index_in_addrs;
+
+	  /* Make the found item directly addressable from ADDRS.  */
+	  index_in_addrs = *addrs_sorted - addrs->other;
+	  gdb_assert (addrs_to_abfd_addrs[index_in_addrs] == NULL);
+	  addrs_to_abfd_addrs[index_in_addrs] = *abfd_addrs_sorted;
+
+	  /* Never use the same ABFD entry twice.  */
+	  abfd_addrs_sorted++;
+	}
+
+      addrs_sorted++;
+    }
+
   /* Calculate offsets for the loadable sections.
      FIXME! Sections must be in order of increasing loadable section
      so that contiguous sections can use the lower-offset!!!
@@ -556,16 +667,16 @@ addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
   for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
     {
       const char *sect_name = addrs->other[i].name;
-      asection *sect = bfd_get_section_by_name (abfd, sect_name);
+      struct other_sections *sect = addrs_to_abfd_addrs[i];
 
       if (sect)
 	{
 	  /* This is the index used by BFD. */
-	  addrs->other[i].sectindex = sect->index;
+	  addrs->other[i].sectindex = sect->sectindex;
 
 	  if (addrs->other[i].addr != 0)
 	    {
-	      addrs->other[i].addr -= bfd_section_vma (abfd, sect);
+	      addrs->other[i].addr -= sect->addr;
 	      lower_offset = addrs->other[i].addr;
 	    }
 	  else
@@ -597,6 +708,8 @@ addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
 	  /* SECTINDEX is invalid if ADDR is zero.  */
 	}
     }
+
+  do_cleanups (my_cleanup);
 }
 
 /* Parse the user's idea of an offset for dynamic linking, into our idea
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dup-sect.S
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.section	sect1, "a"
+var1:	.byte	1
+
+	.section	sect2, "a"
+var2:	.byte	2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dup-sect.exp
@@ -0,0 +1,79 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test inappropriate offseting of multiple sections with the same name.
+# When kept in object file (before final executable link) it still works.
+# When separate debug info file is not used it still works.
+# When the ELF symbol table is kept in the main binary it still works.
+# Used .S file as in .c file we would need __attriute__((section)) which is
+# a GCC extension.
+
+# This test can only be run on targets which support ELF and use gas.
+# For now pick a sampling of likely targets.
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0
+}
+
+set testfile dup-sect
+set srcfile ${testfile}.S
+set srcmainfile start.c
+set executable ${testfile}
+set binfile ${objdir}/${subdir}/${executable}
+
+if {[build_executable ${testfile}.exp $executable [list ${srcfile} ${srcmainfile}] {}] == -1} {
+    return -1
+}
+
+set test "rename section"
+set objcopy_program [transform objcopy]
+set result [catch "exec $objcopy_program --rename-section sect2=sect1 $binfile" output]
+verbose "result is $result"
+verbose "output is $output"
+if {$result != 0} {
+    fail $test
+    return
+}
+pass $test
+
+set test "split"
+if {[gdb_gnu_strip_debug $binfile] != 0} {
+    fail $test
+} else {
+    pass $test
+}
+
+# gdb_gnu_strip_debug uses only --strip-debug and keeps the ELF symbol table
+# in $binfile.
+set test "strip"
+set strip_program [transform strip]
+set result [catch "exec $strip_program $binfile" output]
+verbose "result is $result"
+verbose "output is $output"
+if {$result != 0} {
+    fail $test
+    return
+}
+pass $test
+
+clean_restart $executable
+
+gdb_test "p/d *(const char *) &var1" " = 1" "var1 after strip"
+gdb_test "p/d *(const char *) &var2" " = 2" "var2 after strip"

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Tom> Can we stably sort the sections by name, using the initial index as
Tom> a secondary key?

Jan> OK, attached.

Thanks... for future reference, and for anybody following along, it is
also ok to push back on questions like that if you think they are bad
ideas.  Like we discussed on irc, I couldn't really tell from this reply
what you thought of it.

Jan> 2010-03-25  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	* symfile.c (build_section_addr_info_from_bfd): New.
Jan> 	(build_section_addr_info_from_objfile): Base it on
Jan> 	build_section_addr_info_from_bfd.
Jan> 	(addrs_section_compar, addrs_section_sort): New.
Jan> 	(addr_info_make_relative): New variables my_cleanup, abfd_addrs,
Jan> 	addrs_sorted, abfd_addrs_sorted and addrs_to_abfd_addrs.  Build
Jan> 	addrs_to_abfd_addrs.  Use it for recalculating ADDRS.

Jan> +  /* SECTINDEX is underfined iff ADDR is zero.  */

A small typo: s/underfined/undefined/

This patch looks good to me.  It is ok.

Tom

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Jan Kratochvil]

On Thu, 25 Mar 2010 21:00:16 +0100, Tom Tromey wrote:
> Thanks... for future reference, and for anybody following along, it is
> also ok to push back on questions like that if you think they are bad
> ideas.  Like we discussed on irc, I couldn't really tell from this reply
> what you thought of it.

I have to admit I do not believe there would ever be seen any real world
functionality difference between the first 14-liner and this 126-liner.

But I agree this second/checked-in patch is a more complete/safe solution for
the problem.  So if there have popped up concerns about the first patch and
GDB maintainers agree to accept the larger codebase of the more complete/safe
solution the technical cost of coding such implementation I find more
appropriate than arguing about it.


> A small typo: s/underfined/undefined/

Fixed.

> This patch looks good to me.  It is ok.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-03/msg00241.html


Now the question remains what to use for gdb_7_1-branch as it fixes
a regression (by me) from gdb-7.0 as described in the first mail:
	http://sourceware.org/ml/gdb-patches/2010-03/msg00799.html

I would even suggest the first simple variant of the patch from that mail.


Thanks,
Jan

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> Now the question remains what to use for gdb_7_1-branch as it fixes
Jan> a regression (by me) from gdb-7.0 as described in the first mail:
Jan> 	http://sourceware.org/ml/gdb-patches/2010-03/msg00799.html

Jan> I would even suggest the first simple variant of the patch from that mail.

Was there ever an answer to this?
I think the simple variant is probably safe enough.  In any case it is
an improvement.

Tom

Re: [patch] Fix separate-debug with non-unique section names (PR 11409)

[Jan Kratochvil]

On Wed, 21 Apr 2010 22:03:11 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> Now the question remains what to use for gdb_7_1-branch as it fixes
> Jan> a regression (by me) from gdb-7.0 as described in the first mail:
> Jan> 	http://sourceware.org/ml/gdb-patches/2010-03/msg00799.html
> 
> Jan> I would even suggest the first simple variant of the patch from that mail.
> 
> Was there ever an answer to this?

No.


> I think the simple variant is probably safe enough.  In any case it is
> an improvement.

Checked-in the branch gdb_7_1-branch.

Had to make a slight port for "sect_name".

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2010-04/msg00229.html

--- src/gdb/ChangeLog	2010/04/08 17:15:08	1.11378.2.36
+++ src/gdb/ChangeLog	2010/04/22 23:20:13	1.11378.2.37
@@ -1,3 +1,9 @@
+2010-04-22  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* symfile.c (addr_info_make_relative): Move sect declaration to the
+	outer block.  Initialize it to NULL.  Prefer SECT->next more than
+	bfd_get_section_by_name.
+
 2010-04-08  Sami Wagiaalla  <swagiaal@redhat.com>
 
 	PR Breakpoints/11408:
--- src/gdb/symfile.c	2010/03/17 00:05:23	1.272.2.3
+++ src/gdb/symfile.c	2010/04/22 23:20:14	1.272.2.4
@@ -566,6 +566,7 @@
   asection *lower_sect;
   CORE_ADDR lower_offset;
   int i;
+  asection *sect;
 
   /* Find lowest loadable section to be used as starting point for
      continguous sections.  */
@@ -590,10 +591,23 @@
      (the loadable section directly below it in memory).
      this_offset = lower_offset = lower_addr - lower_orig_addr */
 
+  sect = NULL;
   for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
     {
-      asection *sect = bfd_get_section_by_name (abfd, addrs->other[i].name);
+      const char *sect_name = addrs->other[i].name;
+
+      /* Prefer the next section of that we have found last.  The separate
+	 debug info files have either the same section layout or just a few
+	 sections are missing there.  On the other hand the section name is not
+	 unique and we could find an inappropraite section by its name.  */
+
+      if (sect)
+	sect = sect->next;
+      if (sect && strcmp (sect_name, bfd_get_section_name (abfd, sect)) != 0)
+	sect = NULL;
 
+      if (sect == NULL)
+	sect = bfd_get_section_by_name (abfd, sect_name);
       if (sect)
 	{
 	  /* This is the index used by BFD. */
--- src/gdb/testsuite/ChangeLog	2010/04/08 17:15:11	1.2147.2.5
+++ src/gdb/testsuite/ChangeLog	2010/04/22 23:20:14	1.2147.2.6
@@ -1,3 +1,7 @@
+2010-04-22  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* gdb.base/dup-sect.exp, gdb.base/dup-sect.S: New.
+
 2010-04-08  Sami Wagiaalla  <swagiaal@redhat.com>
 
 	* gdb.cp/gdb2384-base.h: Created 'namespace B'.
--- src/gdb/testsuite/gdb.base/dup-sect.S
+++ src/gdb/testsuite/gdb.base/dup-sect.S	2010-04-22 23:20:26.349536000 +0000
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.section	sect1, "a"
+var1:	.byte	1
+
+	.section	sect2, "a"
+var2:	.byte	2
--- src/gdb/testsuite/gdb.base/dup-sect.exp
+++ src/gdb/testsuite/gdb.base/dup-sect.exp	2010-04-22 23:20:26.676628000 +0000
@@ -0,0 +1,79 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test inappropriate offseting of multiple sections with the same name.
+# When kept in object file (before final executable link) it still works.
+# When separate debug info file is not used it still works.
+# When the ELF symbol table is kept in the main binary it still works.
+# Used .S file as in .c file we would need __attriute__((section)) which is
+# a GCC extension.
+
+# This test can only be run on targets which support ELF and use gas.
+# For now pick a sampling of likely targets.
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0
+}
+
+set testfile dup-sect
+set srcfile ${testfile}.S
+set srcmainfile start.c
+set executable ${testfile}
+set binfile ${objdir}/${subdir}/${executable}
+
+if {[build_executable ${testfile}.exp $executable [list ${srcfile} ${srcmainfile}] {}] == -1} {
+    return -1
+}
+
+set test "rename section"
+set objcopy_program [transform objcopy]
+set result [catch "exec $objcopy_program --rename-section sect2=sect1 $binfile" output]
+verbose "result is $result"
+verbose "output is $output"
+if {$result != 0} {
+    fail $test
+    return
+}
+pass $test
+
+set test "split"
+if {[gdb_gnu_strip_debug $binfile] != 0} {
+    fail $test
+} else {
+    pass $test
+}
+
+# gdb_gnu_strip_debug uses only --strip-debug and keeps the ELF symbol table
+# in $binfile.
+set test "strip"
+set strip_program [transform strip]
+set result [catch "exec $strip_program $binfile" output]
+verbose "result is $result"
+verbose "output is $output"
+if {$result != 0} {
+    fail $test
+    return
+}
+pass $test
+
+clean_restart $executable
+
+gdb_test "p/d *(const char *) &var1" " = 1" "var1 after strip"
+gdb_test "p/d *(const char *) &var2" " = 2" "var2 after strip"