[PATCH] Defer breakpoint reset when cloning progspace for fork child

[PATCH] Defer breakpoint reset when cloning progspace for fork child

[Simon Marchi]

Using this simple test:

  static void
  break_here ()
  {
  }

  int
  main (int argc, char *argv[])
  {
    fork ();
    break_here();
    return 0;
  }

compiled as a PIE:

  $ gcc test.c -g3 -O0 -o test -pie

and running this:

  $ ./gdb -nx -q --data-directory=data-directory ./test -ex "b break_here" -ex "set detach-on-fork off" -ex r

gives:

  Warning:
  Cannot insert breakpoint 1.
  Cannot access memory at address 0x64a

Note that GDB might get stopped by SIGTTOU because of this issue:

  https://sourceware.org/bugzilla/show_bug.cgi?id=23020

In that case, just use "fg" to continue.

This issue happens only with position-independent executables.  Adding
the main objfile for the new inferior (the fork child) causes GDB to try
to reset the breakpoints.  However, that new objfile has not been
relocated yet.  So the breakpoint on "break_here" resolves to an
unrelocated address, from which we are trying to read/write to set a
breakpoint.  Passing SYMFILE_DEFER_BP_RESET avoids that problem.  The
executable is relocated just after, in the follow_fork_inferior
function.

The buildbot seems happy with this patch.  I don't think it's necessary
to add a new test.  Just changing this made many tests go from FAIL to
PASS on my machine, where gcc produces PIE executables by default.  If
anything, I think we would need to add a board file that produces
position-independent executables, so that we can run all the tests with
PIE, even on machines where that is not the default.

gdb/ChangeLog:

	* progspace.c (clone_program_space): Pass SYMFILE_DEFER_BP_RESET
	to symbol_file_add_main.
---
 gdb/progspace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/progspace.c b/gdb/progspace.c
index e0bcc5a18d68..ba400d47aa5c 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -166,7 +166,8 @@ clone_program_space (struct program_space *dest, struct program_space *src)
     exec_file_attach (src->pspace_exec_filename, 0);
 
   if (src->symfile_object_file != NULL)
-    symbol_file_add_main (objfile_name (src->symfile_object_file), 0);
+    symbol_file_add_main (objfile_name (src->symfile_object_file),
+			  SYMFILE_DEFER_BP_RESET);
 
   return dest;
 }
-- 
2.16.3

Re: [PATCH] Defer breakpoint reset when cloning progspace for fork child

[Simon Marchi]

On 2018-03-30 15:01, Simon Marchi wrote:
> Using this simple test:
> 
>   static void
>   break_here ()
>   {
>   }
> 
>   int
>   main (int argc, char *argv[])
>   {
>     fork ();
>     break_here();
>     return 0;
>   }
> 
> compiled as a PIE:
> 
>   $ gcc test.c -g3 -O0 -o test -pie
> 
> and running this:
> 
>   $ ./gdb -nx -q --data-directory=data-directory ./test -ex "b
> break_here" -ex "set detach-on-fork off" -ex r
> 
> gives:
> 
>   Warning:
>   Cannot insert breakpoint 1.
>   Cannot access memory at address 0x64a
> 
> Note that GDB might get stopped by SIGTTOU because of this issue:
> 
>   https://sourceware.org/bugzilla/show_bug.cgi?id=23020
> 
> In that case, just use "fg" to continue.
> 
> This issue happens only with position-independent executables.  Adding
> the main objfile for the new inferior (the fork child) causes GDB to 
> try
> to reset the breakpoints.  However, that new objfile has not been
> relocated yet.  So the breakpoint on "break_here" resolves to an
> unrelocated address, from which we are trying to read/write to set a
> breakpoint.  Passing SYMFILE_DEFER_BP_RESET avoids that problem.  The
> executable is relocated just after, in the follow_fork_inferior
> function.
> 
> The buildbot seems happy with this patch.  I don't think it's necessary
> to add a new test.  Just changing this made many tests go from FAIL to
> PASS on my machine, where gcc produces PIE executables by default.  If
> anything, I think we would need to add a board file that produces
> position-independent executables, so that we can run all the tests with
> PIE, even on machines where that is not the default.

I pushed this patch.

Simon

Re: [PATCH] Defer breakpoint reset when cloning progspace for fork child

[Pedro Alves]

On 04/07/2018 06:52 PM, Simon Marchi wrote:

>> This issue happens only with position-independent executables.  Adding
>> the main objfile for the new inferior (the fork child) causes GDB to try
>> to reset the breakpoints.  However, that new objfile has not been
>> relocated yet.  So the breakpoint on "break_here" resolves to an
>> unrelocated address, from which we are trying to read/write to set a
>> breakpoint.  Passing SYMFILE_DEFER_BP_RESET avoids that problem.  The
>> executable is relocated just after, in the follow_fork_inferior
>> function.
>>

>> The buildbot seems happy with this patch.  I don't think it's necessary
>> to add a new test.  Just changing this made many tests go from FAIL to
>> PASS on my machine, where gcc produces PIE executables by default.  If
>> anything, I think we would need to add a board file that produces
>> position-independent executables, so that we can run all the tests with
>> PIE, even on machines where that is not the default.
> 
> I pushed this patch.

Thanks for the fix.

I'm not sure I agree with no test, though.  I think a simple test
that tries both pie and not-pie would be useful, because
defaults vary depending on distro/system and they change over
time -- having a smoke test like the one described above covering both
pie and non-pie ensures no developer or testing environment where pie
might be relevant ever misses the problem in the future.

Thanks,
Pedro Alves

Re: [PATCH] Defer breakpoint reset when cloning progspace for fork child

[Simon Marchi]

On 2018-04-07 14:43, Pedro Alves wrote:
> I'm not sure I agree with no test, though.  I think a simple test
> that tries both pie and not-pie would be useful, because
> defaults vary depending on distro/system and they change over
> time -- having a smoke test like the one described above covering both
> pie and non-pie ensures no developer or testing environment where pie
> might be relevant ever misses the problem in the future.

Doing this means we'll need to update tests on an individual basis to 
test pie and non-pie.  In order to be able to run all tests with pie 
executables (even on machines where it isn't the default), wouldn't it 
be better to have a target board for it, like we have 
dwarf4-gdb-index.exp, fission.exp and fission-dwp.exp?

Simon

Re: [PATCH] Defer breakpoint reset when cloning progspace for fork child

[Pedro Alves]

On 04/09/2018 08:09 PM, Simon Marchi wrote:
> On 2018-04-07 14:43, Pedro Alves wrote:
>> I'm not sure I agree with no test, though.  I think a simple test
>> that tries both pie and not-pie would be useful, because
>> defaults vary depending on distro/system and they change over
>> time -- having a smoke test like the one described above covering both
>> pie and non-pie ensures no developer or testing environment where pie
>> might be relevant ever misses the problem in the future.
> 
> Doing this means we'll need to update tests on an individual basis to test pie and non-pie.  In order to be able to run all tests with pie executables (even on machines where it isn't the default), wouldn't it be better to have a target board for it, like we have dwarf4-gdb-index.exp, fission.exp and fission-dwp.exp?

While a board file as a new test more may be useful, I think there's
value in having smoke tests that run with the default board too.  Just
like we have other tests in the tree that exercise basic PIE, gdb index,
fission, etc. already.  E.g., we test PIE+exec in gdb.base/pie-execl.exp.

I was not saying to update the existing fork tests, but instead to
add the small test that you described in the commit log as a testcase.

Let me put it another way -- I suspect that if you had discovered
this issue on a system with a compiler that does _not_ emit PIE
by default, you'd have likely considered adding a small test.

Thanks,
Pedro Alves

Re: [PATCH] Defer breakpoint reset when cloning progspace for fork child

[Simon Marchi]

On 2018-04-09 15:21, Pedro Alves wrote:
> While a board file as a new test more may be useful, I think there's
> value in having smoke tests that run with the default board too.  Just
> like we have other tests in the tree that exercise basic PIE, gdb 
> index,
> fission, etc. already.  E.g., we test PIE+exec in 
> gdb.base/pie-execl.exp.
> 
> I was not saying to update the existing fork tests, but instead to
> add the small test that you described in the commit log as a testcase.
> 
> Let me put it another way -- I suspect that if you had discovered
> this issue on a system with a compiler that does _not_ emit PIE
> by default, you'd have likely considered adding a small test.

Ok, I agree.

Simon