[patch] svr4_exec_displacement success indicator [Re: PIE question]

[patch] svr4_exec_displacement success indicator [Re: PIE question]

[Jan Kratochvil]

[repost to <gdb-patches@sourceware.org>]

On Sun, 07 Mar 2010 01:53:29 +0100, Daniel Jacobowitz wrote:
> Jan, could you explain a little how the situation in this comment can
> happen?
> 
> static void
> svr4_relocate_main_executable (void)
> {
>   CORE_ADDR displacement = svr4_exec_displacement ();
> 
>   /* Even if DISPLACEMENT is 0 still try to relocate it as this is a new
>      difference of in-memory vs. in-file addresses and we could already
>      relocate the executable at this function to improper address before.  */

OK, no longer valid with current state of sources, going to post a separate
cleanup patch (later after some verifications) so that
svr4_relocate_main_executable gets called only once.

Some history:
------------------------------------------------------------------------------
svr4_exec_displacement behavior was considered a bit magic as it could
(a) depend on svr4_static_exec_displacement returning something I did not
    understand; to be removed by pending:
      Re: RFC: Verify AT_ENTRY before using it
      http://sourceware.org/ml/gdb-patches/2010-03/msg00030.html
(b) auxv read by ld_so_xfer_auxv resolving relocatable symbol "_dl_auxv" which
    brings some chicken-and-egg problems.
    But ld_so_xfer_auxv is used only if ATTACH_FLAG and in such case
    svr4_relocate_main_executable was called only once.
(c) I was trying to get valgrind-executed-PIE working and thought enough steps
    of investigating inferior would fix it.  There is currently no way to get
    valgrind-executed-PIE working (in general case), filed RFE for valgrind:
      valgrind: --db-command should support %{auxv address}
      http://bugs.kde.org/show_bug.cgi?id=223702
------------------------------------------------------------------------------


> I came across this because our local ARM uClinux incorrectly links in
> solib-svr4.c.  The remote target sends qOffsets, uses the result to
> relocate the objfile, and then this code overrides that.

I did not expect symfile_objfile can be already relocated before.

Attached these changes:

* svr4_exec_displacement calling convention should have success indicator.

* Preserving now section_offsets if they are already set, inspired by
  init_objfile_sect_indices.

I believe either of parts would be sufficient for this problem.


> I don't think this is related to our other discussion about executable
> relocation; I haven't forgotten, I'll get back to you as soon as I can.

I agree but technically these two new patches depend on those previous ones.
      Re: RFC: Verify AT_ENTRY before using it
      http://sourceware.org/ml/gdb-patches/2010-03/msg00030.html
      [patch-testcase] Re: RFC: Verify AT_ENTRY before using it
      http://sourceware.org/ml/gdb-patches/2010-03/msg00033.html


No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu although with
/-fPIE/-pie board it is not clear to me.  Debugging some more PIE
incompletenesses (such as unrelocated ei.entry_point in some cases).

OK to check-in?


Thanks,
Jan


2010-03-07  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* solib-svr4.c (svr4_exec_displacement): Return now success, new
	parameter displacementp.  Update comment.
	(svr4_relocate_main_executable): Return if non-zero SECTION_OFFSETS
	element exists.  Return if svr4_exec_displacement was not successful.
	Update comment.

--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1652,7 +1651,10 @@ read_program_headers_from_bfd (bfd *abfd, int *phdrs_size)
   return buf;
 }
 
-/* We relocate all of the sections by the same amount.  This
+/* Return 1 and fill *DISPLACEMENTP with detected PIE offset of inferior
+   exec_bfd.  Otherwise return 0.
+
+   We relocate all of the sections by the same amount.  This
    behavior is mandated by recent editions of the System V ABI. 
    According to the System V Application Binary Interface,
    Edition 4.1, page 5-5:
@@ -1692,8 +1694,8 @@ read_program_headers_from_bfd (bfd *abfd, int *phdrs_size)
      should either be removed or modified to accomodate the new file
      type.  - Kevin, Nov 2000. ]  */
 
-static CORE_ADDR
-svr4_exec_displacement (void)
+static int
+svr4_exec_displacement (CORE_ADDR *displacementp)
 {
   /* ENTRY_POINT is a possible function descriptor - before
      a call to gdbarch_convert_from_func_ptr_addr.  */
@@ -1785,7 +1787,8 @@ svr4_exec_displacement (void)
 	       bfd_get_filename (exec_bfd));
     }
 
-  return displacement;
+  *displacementp = displacement;
+  return 1;
 }
 
 /* Relocate the main executable.  This function should be called upon
@@ -1796,11 +1799,25 @@ svr4_exec_displacement (void)
 static void
 svr4_relocate_main_executable (void)
 {
-  CORE_ADDR displacement = svr4_exec_displacement ();
+  CORE_ADDR displacement;
+
+  if (symfile_objfile)
+    {
+      int i;
+
+      /* Remote target may have already set specific offsets by `qOffsets'
+	 which should be preferred.  */
+
+      for (i = 0; i < symfile_objfile->num_sections; i++)
+	if (ANOFFSET (symfile_objfile->section_offsets, i) != 0)
+	  return;
+    }
+
+  if (! svr4_exec_displacement (&displacement))
+    return;
 
-  /* Even if DISPLACEMENT is 0 still try to relocate it as this is a new
-     difference of in-memory vs. in-file addresses and we could already
-     relocate the executable at this function to improper address before.  */
+  /* Even DISPLACEMENT 0 is a valid new difference of in-memory vs. in-file
+     addresses.  */
 
   if (symfile_objfile)
     {

Re: [patch] svr4_exec_displacement success indicator [Re: PIE question]

[Daniel Jacobowitz]

On Mon, Mar 08, 2010 at 10:53:58PM +0100, Jan Kratochvil wrote:
> OK to check-in?

Yes, these are OK.  [Sorry, should have checked this folder first.]

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] svr4_exec_displacement success indicator [Re: PIE question]

[Jan Kratochvil]

On Mon, 08 Mar 2010 22:59:13 +0100, Daniel Jacobowitz wrote:
> Yes, these are OK.  [Sorry, should have checked this folder first.]

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

This is a duplicate mail to <gdb@sourceware.org> one:
	http://sourceware.org/ml/gdb/2010-03/msg00069.html


Thanks,
Jan

Re: [patch] svr4_exec_displacement success indicator [Re: PIE question]

[Jan Kratochvil]

On Mon, 08 Mar 2010 22:59:13 +0100, Daniel Jacobowitz wrote:
> On Mon, Mar 08, 2010 at 10:53:58PM +0100, Jan Kratochvil wrote:
> > OK to check-in?
> 
> Yes, these are OK.  [Sorry, should have checked this folder first.]

-> http://sourceware.org/ml/gdb-cvs/2010-03/msg00097.html
#  2010-03-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
# +
# +	* solib-svr4.c (svr4_exec_displacement): Return now success, new
# +	parameter displacementp.  Update comment.
# +	(svr4_relocate_main_executable): Return if non-zero SECTION_OFFSETS
# +	element exists.  Return if svr4_exec_displacement was not successful.
# +	Update comment.


Is it OK also for gdb_7_1-branch?

Otherwise the minimized attached patch also works for me.  Still I would
prefer the full patch from the master branch.

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


Thanks,
Jan


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

	* solib-svr4.c (svr4_relocate_main_executable): Delay the
	svr4_exec_displacement call.  Return on non-DYNAMIC exec_bfd.

--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1719,7 +1719,15 @@ svr4_exec_displacement (void)
 static void
 svr4_relocate_main_executable (void)
 {
-  CORE_ADDR displacement = svr4_exec_displacement ();
+  CORE_ADDR displacement;
+
+  /* Therefore for ELF it is ET_EXEC and not ET_DYN.  Both shared libraries
+     being executed themselves and PIE (Position Independent Executable)
+     executables are ET_DYN.  */
+  if (exec_bfd && (bfd_get_file_flags (exec_bfd) & DYNAMIC) == 0)
+    return;
+
+  displacement = svr4_exec_displacement ();
 
   /* Even if DISPLACEMENT is 0 still try to relocate it as this is a new
      difference of in-memory vs. in-file addresses and we could already

Re: [patch] svr4_exec_displacement success indicator [Re: PIE question]

[Daniel Jacobowitz]

On Fri, Mar 12, 2010 at 04:31:38PM +0100, Jan Kratochvil wrote:
> Is it OK also for gdb_7_1-branch?

I think so.  Joel, is the branch open?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] svr4_exec_displacement success indicator [Re: PIE question]

[Joel Brobecker]

> > Is it OK also for gdb_7_1-branch?
> 
> I think so.  Joel, is the branch open?

Yep - I will send an email when I close the branch (which is around
the time I create the release, usually a window of 1 to 2 hours).

-- 
Joel

Re: [patch] svr4_exec_displacement success indicator [Re: PIE question]

[Jan Kratochvil]

On Fri, 12 Mar 2010 16:39:41 +0100, Daniel Jacobowitz wrote:
> On Fri, Mar 12, 2010 at 04:31:38PM +0100, Jan Kratochvil wrote:
> > Is it OK also for gdb_7_1-branch?
> 
> I think so.

Checked-in for gdb_7_1-branch:
	http://sourceware.org/ml/gdb-cvs/2010-03/msg00124.html

2010-03-14  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* solib-svr4.c (svr4_exec_displacement): Return now success, new
	parameter displacementp.  Update comment.
	(svr4_relocate_main_executable): Return if non-zero SECTION_OFFSETS
	element exists.  Return if svr4_exec_displacement was not successful.
	Update comment.


Thanks,
Jan

[patch] PIE: Fix back re-run [Re: [patch] svr4_exec_displacement success indicator]

[Jan Kratochvil]

Hi,

currently:

$ echo 'main(){}'|gcc -o 1 -fPIE -pie -x c -; ./gdb -nx -ex 'set disable-randomization off' -ex 'b main' -ex r -ex c -ex r ./1
Breakpoint 1 at 0x6b0
Starting program: /home/jkratoch/redhat/gdb-clean/gdb/testsuite/1 
Breakpoint 1, 0x00007fbf73e8c6b0 in main ()
Continuing.
Program exited with code 0140.
Starting program: /home/jkratoch/redhat/gdb-clean/gdb/testsuite/1 
Error in re-setting breakpoint 1: Cannot access memory at address 0x7fbf73e8c6ac

It is since:
	[patch] svr4_exec_displacement success indicator [Re: PIE question]
	http://sourceware.org/ml/gdb-patches/2010-03/msg00336.html


On Mon, 08 Mar 2010 22:53:58 +0100, Jan Kratochvil wrote:
> Attached these changes:
> 
> * svr4_exec_displacement calling convention should have success indicator.
> 
> * Preserving now section_offsets if they are already set, inspired by
>   init_objfile_sect_indices.
> 
> I believe either of parts would be sufficient for this problem.

The first part has caused the regression for PIE on native x86* GNU/Linux host.

As I believe for Daniel J.'s seen regression of `qOffsets' the second already
checked-in part is sufficient - I would like to remove the first part.

OK to check it in?

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

(I do not think one needs to think about 7.1-branch as it is not a regression
against any FSF GDB release.)


Thanks,
Jan


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

	Fix re-run of PIE executable.
	* solib-svr4.c (svr4_relocate_main_executable) <symfile_objfile>: Remove
	the part of pre-set SYMFILE_OBJFILE->SECTION_OFFSETS.

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

	Fix re-run of PIE executable.
	* gdb.base/break-interp.exp (test_ld): Turn off "disable-randomization".
	Remove $displacement_main to match the solib-svr4.c change.  New "kill"
	and re-"run" of the inferior.

--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1791,17 +1791,10 @@ svr4_relocate_main_executable (void)
 {
   CORE_ADDR displacement;
 
-  if (symfile_objfile)
-    {
-      int i;
-
-      /* Remote target may have already set specific offsets by `qOffsets'
-	 which should be preferred.  */
-
-      for (i = 0; i < symfile_objfile->num_sections; i++)
-	if (ANOFFSET (symfile_objfile->section_offsets, i) != 0)
-	  return;
-    }
+  /* SYMFILE_OBJFILE->SECTION_OFFSETS may now contain displacement from the
+     previous run of the inferior.  Re-set it according to the current value,
+     if we can find it out.  But otherwise keep it as for remote target it may
+     have been pre-set by the `qOffsets' packet.  */
 
   if (! svr4_exec_displacement (&displacement))
     return;
--- a/gdb/testsuite/gdb.base/break-interp.exp
+++ b/gdb/testsuite/gdb.base/break-interp.exp
@@ -416,25 +416,28 @@ proc test_ld {file ifmain trynosym displacement} {
     # Print the "PIE (Position Independent Executable) displacement" message.
     gdb_test "set verbose on"
 
+    # A bit better test coverage.
+    gdb_test "set disable-randomization off"
+
     reach "dl_main" "run segv" $displacement
 
     gdb_test "bt" "#0 +\[^\r\n\]*\\mdl_main\\M.*" "dl bt"
 
     if $ifmain {
 	# Displacement message will be printed the second time on initializing
-	# the linker from svr4_special_symbol_handling.  If any ANOFFSET has
-	# been already set as non-zero the detection will no longer be run.
-	if {$displacement == "NONZERO"} {
-	    set displacement_main "NONE"
-	} else {
-	    set displacement_main $displacement
-	}
-	reach "main" continue $displacement_main
+	# the linker from svr4_special_symbol_handling.
+	reach "main" continue $displacement
 
 	reach "libfunc" continue "NONE"
 
 	gdb_test "bt" "#0 +\[^\r\n\]*\\mlibfunc\\M\[^\r\n\]*\r\n#1 +\[^\r\n\]*\\mmain\\M.*" "main bt"
+    }
 
+    # Try re-run if the new PIE displacement takes effect.
+    gdb_test "kill" "" "kill" {Kill the program being debugged\? \(y or n\) } "y"
+    reach "dl_main" "run  segv" $displacement
+
+    if $ifmain {
 	test_core $file $displacement
 
 	test_attach $file $displacement

[cancel] Re: [patch] PIE: Fix back re-run [Re: [patch] svr4_exec_displacement success indicator]

[Jan Kratochvil]

Cancelled as it is being reposted rediffed in a larger series.