Re: ping: [patch 6/6] PIE: Fix back re-run

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Thu, 01 Jul 2010 21:10:41 +0200, Joel Brobecker wrote:
> BTW: I seem to be commenting a lot on your comments, but as said before,
> you're one of the few who makes an active effort at writing them, and
> I really think that this is extremely important and useful.  So a big
> Thank You.

I see it may be more simple to drop the comments before the final submit. :-)
Thanks for the reviewing patience, some problems of my comments I can
hopefully improve in the future.


> > +  /* 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.  */
> 
> I was having a hard time trying to figure out what you were trying to
> get at until I had a broader picture of the various scenarios that we
> are trying to deal with.  I would like to rephrase your comment a little
> differently:
> 
>   /* If we are re-running this executable, SYMFILE_OBJFILE->SECTION_OFFSETS
>      probably contains the offsets computed using the PIE displacement
>      from the previous run, which of course are irrelevant for this run.
>      So we need to determine the new PIE displacement and recompute the
>      section offsets accordingly, even if SYMFILE_OBJFILE->SECTION_OFFSETS
>      already contains pre-computed offsets.
> 
>      If, for some reason we cannot compute the PIE displacement (why???),
>      then we leave the section offsets untouched and use them as is for
>      this run.  Either:
>        
>        - These section offsets were properly reset earlier, and thus
>          already contain the correct values.  This can happen for instance
>          when reconnecting via the remote protocol to a target that supports
>          the `qOffsets' packet.
> 
>        - The section offsets were not reset earlier, and the best we can
>          hope is that the old offsets are still applicable to the new run.
>    */
> 
> But all this begs one important question: When does svr4_exec_displacement
> return zero (fail)??? Based on the code, it can happen in two situations,
> I think: (a) PIE is not in use; and (b) binary on disk is different from
> program being executed. Case (b) is hopeless as I understand it? But (a)
> shouldn't.

The (b) case is in use by Daniel Jacobowitz in:
	RFC: Verify AT_ENTRY before using it
	http://sourceware.org/ml/gdb-patches/2010-02/msg00612.html
	 - In one window, "gdbserver :PORT /path/to/this/loader /path/to/binary".
	 - In the other, "gdb /path/to/binary" and "target remote :PORT".

which is IMO invalid use of GDB it was the only working solution its time.
I broke it by the PIE patchset (fixed in that thread).  IMO the correct
solution is to have symbol file matching the target which was addressed by:
	[patch] Support gdb --args ld.so progname
	http://sourceware.org/ml/gdb-patches/2010-02/msg00647.html
	 - To be able to use "gdb /path/to/this/loader" on the client.
	 - To be pinged for gdb-7.3.

OTOH I understand GDB should not break backward compatibility even if/after
the latter patch gets accepted so we have to consider the (b) case as valid.


> So, expanding the logic in the comment I am suggesting, I think we need
> to account for the situation where svr4_exec_displacement fails because
> of non-PIE executable.
> 
> Either: (a1) there was no PIE in sight during the previous run either
>              => offsets should be already be correct;

Yes, if there is no PIE anywhere, the whole PIE patchset by me has no effect.


>         (a2) PIE was in use during the previous run => the offsets
>              may be incorrect.  But it should be trivial to reset them.

If PIE was in use during the previous run then svr4_exec_displacement should
not fail, it should succeed and report the current offset to reset to.
(If it fails it is an unexpected and not known so far PIE support bug.)

These offsets are bound to symfile_objfile so the offsets will be reset
/ different if PIE executable was run previously and non-PIE different
executable is loaded + executed now.


Therefore replaced there this part:
     If we cannot compute the PIE displacement, either:

       - The executable is not PIE.

       - SYMFILE_OBJFILE does not match the executable started in the target.
         This can happen for main executable symbols loaded at the host while
         `ld.so --ld-args main-executable' is loaded in the target.

     Then we leave the section offsets untouched and use them as is for
     this run.  Either:


> So, I'm wondering whether it would make sense to amend your patch to
> check for PIE after svr4_exec_displacement failed, and use a zero
> displacement if PIE is not detected.

It is based on
	PIE question
	http://sourceware.org/ml/gdb/2010-03/msg00040.html
	 - Remote target's qOffsets being reset.
and



> > +    # A bit better test coverage.
> > +    gdb_test "set disable-randomization off"
> 
> I think that this does more than just "better coverage". It is a
> critical part of the testcase that helps trying to obtain two runs
> with different displacement addresses.

True, without it FAILs only on testfile message check
	reach-dl_main: seen displacement message as NONZERO
while with it there is a real GDB failure:
	Cannot insert breakpoint -2.^M
	Error accessing memory address 0x7f9467321d00: Input/output error.^M
	(gdb) FAIL: gdb.base/break-interp.exp: LDprelinkNOdebugNO: reach-dl_main: reach


> > +    reach "dl_main" "run  segv" $displacement
>                            ^^ detail: two spaces?

It was a "hidden parameter" in some versions (not suitable for FSF GDB).  But
I can no longer find the code using it in repositories history.  Fixed.


Thanks,
Jan


gdb/
2010-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Joel Brobecker  <brobecker@adacore.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-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Joel Brobecker  <brobecker@adacore.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
@@ -1989,17 +1989,32 @@ svr4_relocate_main_executable (void)
 {
   CORE_ADDR displacement;
 
-  if (symfile_objfile)
-    {
-      int i;
+  /* If we are re-running this executable, SYMFILE_OBJFILE->SECTION_OFFSETS
+     probably contains the offsets computed using the PIE displacement
+     from the previous run, which of course are irrelevant for this run.
+     So we need to determine the new PIE displacement and recompute the
+     section offsets accordingly, even if SYMFILE_OBJFILE->SECTION_OFFSETS
+     already contains pre-computed offsets.
 
-      /* Remote target may have already set specific offsets by `qOffsets'
-	 which should be preferred.  */
+     If we cannot compute the PIE displacement, either:
 
-      for (i = 0; i < symfile_objfile->num_sections; i++)
-	if (ANOFFSET (symfile_objfile->section_offsets, i) != 0)
-	  return;
-    }
+       - The executable is not PIE.
+
+       - SYMFILE_OBJFILE does not match the executable started in the target.
+	 This can happen for main executable symbols loaded at the host while
+	 `ld.so --ld-args main-executable' is loaded in the target.
+
+     Then we leave the section offsets untouched and use them as is for
+     this run.  Either:
+
+       - These section offsets were properly reset earlier, and thus
+	 already contain the correct values.  This can happen for instance
+	 when reconnecting via the remote protocol to a target that supports
+	 the `qOffsets' packet.
+
+       - The section offsets were not reset earlier, and the best we can
+	 hope is that the old offsets are still applicable to the new run.
+   */
 
   if (! svr4_exec_displacement (&displacement))
     return;
--- a/gdb/testsuite/gdb.base/break-interp.exp
+++ b/gdb/testsuite/gdb.base/break-interp.exp
@@ -337,6 +337,11 @@ proc test_ld {file ifmain trynosym displacement} {
     # Print the "PIE (Position Independent Executable) displacement" message.
     gdb_test_no_output "set verbose on"
 
+    # We want to test the re-run of a PIE in the case where the executable
+    # is loaded with a different displacement, but disable-randomization
+    # prevents that from happening.  So turn it off.
+    gdb_test "set disable-randomization off"
+
     reach "dl_main" "run segv" $displacement
 
     gdb_test "bt" "#0 +\[^\r\n\]*\\mdl_main\\M.*" "dl bt"
@@ -347,7 +352,13 @@ proc test_ld {file ifmain trynosym 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