[rfc] Improve loading unlinked object files

[Daniel Jacobowitz <drow at false dot org>]

A user noticed that when he loaded a kernel module (for Linux kernel
debugging these are partially linked objects, from ld -r) a breakpoint
would be set at the wrong location.  I tracked this down to the use of
two text sections, .text and .init.text.

GDB has some support for relocating (both minimal and debug) symbols
based on their containing section.  But it's pretty limited, because
in a lot of cases there is no clear link from debug information back
to the section table.  As a result many places in GDB relocate
according to SECT_OFF_TEXT - the dwarf2 frame unwinder, the location
expression parser, the line table parser, and others during symbol
reading.  The line table parser was specifically to blame here.  The
line number tables for .init.text and .text had been overlayed.

My first idea was to apply objfile offsets during
symfile_relocate_debug_section.  But that doesn't work out, because
some shared libraries have relocations for their debug sections and
others don't - so for shared libraries, we may need to add section
offsets to relocated debug information anyway.  We don't want to end
up adding them twice.

The best I could come up with was this.  It's in code which only runs
when loading an unlinked object (either via file or add-symbol-file),
very early in the process.  It takes the user's specified section
addresses, and any default addresses we added to disambiguate
variables in different sections, and swaps them into the BFD as
section VMAs.  Then it clears the offsets.

It fixes the attached testcase, which uses C++ templates to provoke
the same scenario.

Does anyone have any comments on this patch, besides the obvious ones
("it's ugly" and "we have too many confusingly different ways to
represent section addresses", see included comment)?

-- 
Daniel Jacobowitz
CodeSourcery

2007-02-15  Daniel Jacobowitz  <dan@codesourcery.com>

	* symfile.c (place_section): Check SEC_ALLOC.  Do not check VMA.
	(default_symfile_offsets): Check VMA here.  Update section VMAs.

2007-02-15  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.cp/cp-relocate.cc, gdb.cp/cp-relocate.exp: New.

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.181
diff -u -p -r1.181 symfile.c
--- symfile.c	13 Feb 2007 08:15:49 -0000	1.181
+++ symfile.c	15 Feb 2007 18:48:45 -0000
@@ -477,8 +477,8 @@ place_section (bfd *abfd, asection *sect
   int done;
   ULONGEST align = ((ULONGEST) 1) << bfd_get_section_alignment (abfd, sect);
 
-  /* We are only interested in loadable sections.  */
-  if ((bfd_get_section_flags (abfd, sect) & SEC_LOAD) == 0)
+  /* We are only interested in allocated sections.  */
+  if ((bfd_get_section_flags (abfd, sect) & SEC_ALLOC) == 0)
     return;
 
   /* If the user specified an offset, honor it.  */
@@ -502,13 +502,8 @@ place_section (bfd *abfd, asection *sect
 	if (cur_sec == sect)
 	  continue;
 
-	/* We can only conflict with loadable sections.  */
-	if ((bfd_get_section_flags (abfd, cur_sec) & SEC_LOAD) == 0)
-	  continue;
-
-	/* We do not expect this to happen; just ignore sections in a
-	   relocatable file with an assigned VMA.  */
-	if (bfd_section_vma (abfd, cur_sec) != 0)
+	/* We can only conflict with allocated sections.  */
+	if ((bfd_get_section_flags (abfd, cur_sec) & SEC_ALLOC) == 0)
 	  continue;
 
 	/* If the section offset is 0, either the section has not been placed
@@ -581,9 +576,62 @@ default_symfile_offsets (struct objfile 
   if ((bfd_get_file_flags (objfile->obfd) & (EXEC_P | DYNAMIC)) == 0)
     {
       struct place_section_arg arg;
-      arg.offsets = objfile->section_offsets;
-      arg.lowest = 0;
-      bfd_map_over_sections (objfile->obfd, place_section, &arg);
+      bfd *abfd = objfile->obfd;
+      asection *cur_sec;
+      CORE_ADDR lowest = 0;
+
+      for (cur_sec = abfd->sections; cur_sec != NULL; cur_sec = cur_sec->next)
+	/* We do not expect this to happen; just skip this step if the
+	   relocatable file has a section with an assigned VMA.  */
+	if (bfd_section_vma (abfd, cur_sec) != 0)
+	  break;
+
+      if (cur_sec == NULL)
+	{
+	  CORE_ADDR *offsets = objfile->section_offsets->offsets;
+
+	  /* Pick non-overlapping offsets for sections the user did not
+	     place explicitly.  */
+	  arg.offsets = objfile->section_offsets;
+	  arg.lowest = 0;
+	  bfd_map_over_sections (objfile->obfd, place_section, &arg);
+
+	  /* Correctly filling in the section offsets is not quite
+	     enough.  Relocatable files have two properties that
+	     (most) shared objects do not:
+
+	     - Their debug information will contain relocations.  Some
+	     shared libraries do also, but many do not, so this can not
+	     be assumed.
+
+	     - If there are multiple code sections they will be loaded
+	     at different relative addresses in memory than they are
+	     in the objfile, since all sections in the file will start
+	     at address zero.
+
+	     Because GDB has very limited ability to map from an
+	     address in debug info to the correct code section,
+	     it relies on adding SECT_OFF_TEXT to things which might be
+	     code.  If we clear all the section offsets, and set the
+	     section VMAs instead, then symfile_relocate_debug_section
+	     will return meaningful debug information pointing at the
+	     correct sections.
+
+	     GDB has too many different data structures for section
+	     addresses - a bfd, objfile, and so_list all have section
+	     tables, as does exec_ops.  Some of these could probably
+	     be eliminated.  */
+
+	  for (cur_sec = abfd->sections; cur_sec != NULL;
+	       cur_sec = cur_sec->next)
+	    {
+	      if ((bfd_get_section_flags (abfd, cur_sec) & SEC_ALLOC) == 0)
+		continue;
+
+	      bfd_set_section_vma (abfd, cur_sec, offsets[cur_sec->index]);
+	      offsets[cur_sec->index] = 0;
+	    }
+	}
     }
 
   /* Remember the bfd indexes for the .text, .data, .bss and
Index: testsuite/gdb.cp/cp-relocate.cc
===================================================================
RCS file: testsuite/gdb.cp/cp-relocate.cc
diff -N testsuite/gdb.cp/cp-relocate.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/cp-relocate.cc	15 Feb 2007 18:48:45 -0000
@@ -0,0 +1,29 @@
+/* This test file is part of GDB, the GNU debugger.
+
+   Copyright 2007
+   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 2 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, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
+   02111-1307, USA.  */
+
+template<int X> int func(int)
+{
+  return X;
+}
+
+int caller()
+{
+  return func<1>(1) + func<2>(2);
+}
Index: testsuite/gdb.cp/cp-relocate.exp
===================================================================
RCS file: testsuite/gdb.cp/cp-relocate.exp
diff -N testsuite/gdb.cp/cp-relocate.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/cp-relocate.exp	15 Feb 2007 18:48:45 -0000
@@ -0,0 +1,137 @@
+# Copyright 2007 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 2 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, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  */
+
+# Test loading symbols from unrelocated C++ object files.
+
+set testfile cp-relocate
+set srcfile ${testfile}.cc
+set binfile ${objdir}/${subdir}/${testfile}.o
+
+if { [skip_cplus_tests] } { continue }
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" object {c++ debug}] != "" } {
+     untested cp-relocate.exp
+     return -1
+}
+
+proc get_func_address { func } {
+    global gdb_prompt hex
+
+    set rfunc [string_to_regexp $func]
+    gdb_test_multiple "print '${func}'" "get address of ${func}" {
+	-re "\\\$\[0-9\]+ = \\{.*\\} (0|($hex) <${rfunc}>)\[\r\n\]+${gdb_prompt} $" {
+	    # $1 = {int ()} 0x24 <function_bar>
+	    # But if the function is at zero, the name may be omitted.
+	    pass "get address of ${func}"
+	    if { $expect_out(1,string) == "0" } {
+		return "0x0"
+	    } else {
+		return $expect_out(2,string)
+	    }
+	}
+    }
+  return ""
+}
+
+# Load the file as an executable; GDB should assign non-overlapping
+# section offsets.
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_file_cmd ${binfile}
+
+# Find the interesting functions.  We go to a little effort to find
+# the right function names here, to work around PR c++/40.
+set func1_name ""
+set func2_name ""
+gdb_test_multiple "info functions func<.>" "info functions" {
+    -re "\r\nint (\[^\r\]*func<1>\[^\r]*);" {
+	set func1_name $expect_out(1,string)
+	exp_continue
+    }
+    -re "\r\nint (\[^\r\]*func<2>\[^\r]*);" {
+	set func2_name $expect_out(1,string)
+	exp_continue
+    }
+    -re "$gdb_prompt $" {
+	if { ${func1_name} != "" && ${func2_name} != "" } {
+	    pass "info functions"
+	} else {
+	    fail "info functions"
+	    return -1
+	}
+    }
+}
+
+# Check that all the functions have different addresses.
+set func1_addr [get_func_address "$func1_name"]
+set func2_addr [get_func_address "$func2_name"]
+set caller_addr [get_func_address "caller"]
+
+if { "${func1_addr}" == "${func2_addr}"
+     || "${func1_addr}" == "${func2_addr}"
+     || "${func2_addr}" == "${caller_addr}" } {
+  fail "C++ functions have different addresses"
+} else {
+  pass "C++ functions have different addresses"
+}
+
+# Figure out the names of the sections containing the template
+# functions.
+set func1_sec ""
+set func2_sec ""
+gdb_test_multiple "info file" "info file" {
+    -re "($hex) - ($hex) is (\[^\r\]*)\r" {
+	if { $expect_out(1,string) <= $func1_addr
+	     && $expect_out(2,string) > $func1_addr } {
+	    set func1_sec $expect_out(3,string)
+	} elseif { $expect_out(1,string) <= $func2_addr
+	    && $expect_out(2,string) > $func2_addr } {
+	    set func2_sec $expect_out(3,string)
+	}
+	exp_continue
+    }
+    -re "$gdb_prompt $" {
+	if { ${func1_sec} != "" && ${func2_sec} != "" } {
+	    pass "info file"
+	} else {
+	    fail "info file"
+	    return -1
+	}
+    }
+}
+
+if { $func1_sec == $func2_sec } {
+    untested "cp-relocate.exp - template functions in same sections"
+    return -1
+}
+
+# Now start a clean GDB, for add-symbol-file tests.
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_test "add-symbol-file ${binfile} 0 -s ${func1_sec} 0x40000 -s ${func2_sec} 0x80000" \
+	"Reading symbols from .*${testfile}\\.o\\.\\.\\.done\\.(|\r\nUsing host libthread_db library .*libthread_db.so.*\\.)" \
+	"add-symbol-file ${binfile}" \
+	"add symbol table from file \".*${testfile}\\.o\" at.*\\(y or n\\) " \
+	"y"
+
+# Make sure the function addresses were updated.
+gdb_test "break *'$func1_name'" \
+    "Breakpoint $decimal at 0x4....: file .*"
+gdb_test "break *'$func2_name'" \
+    "Breakpoint $decimal at 0x8....: file .*"