diff mbox

Defer breakpoint reset when cloning progspace for fork child

Message ID 20180330190132.20823-1-simon.marchi@polymtl.ca
State New
Headers show

Commit Message

Simon Marchi March 30, 2018, 7:01 p.m. UTC
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(-)

Comments

Simon Marchi April 7, 2018, 5:52 p.m. UTC | #1
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
Pedro Alves April 7, 2018, 6:43 p.m. UTC | #2
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
Simon Marchi April 9, 2018, 7:09 p.m. UTC | #3
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
Pedro Alves April 9, 2018, 7:21 p.m. UTC | #4
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
Simon Marchi April 9, 2018, 7:35 p.m. UTC | #5
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
diff mbox

Patch

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;
 }