Message ID | 20180330190132.20823-1-simon.marchi@polymtl.ca |
---|---|
State | New, archived |
Headers |
Received: (qmail 112687 invoked by alias); 30 Mar 2018 19:01:50 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 112677 invoked by uid 89); 30 Mar 2018 19:01:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_SOFTFAIL, UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy= X-HELO: barracuda.ebox.ca Received: from barracuda.ebox.ca (HELO barracuda.ebox.ca) (96.127.255.19) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 30 Mar 2018 19:01:48 +0000 X-ASG-Debug-ID: 1522436493-0c856e618971d2b0001-fS2M51 Received: from smtp.ebox.ca (smtp.electronicbox.net [96.127.255.82]) by barracuda.ebox.ca with ESMTP id C1nai2aSzG51iD9S (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 30 Mar 2018 15:01:33 -0400 (EDT) X-Barracuda-Envelope-From: simon.marchi@polymtl.ca X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from simark.lan (unknown [192.222.164.54]) by smtp.ebox.ca (Postfix) with ESMTP id C84DE441B21; Fri, 30 Mar 2018 15:01:33 -0400 (EDT) From: Simon Marchi <simon.marchi@polymtl.ca> X-Barracuda-Effective-Source-IP: 192-222-164-54.qc.cable.ebox.net[192.222.164.54] X-Barracuda-Apparent-Source-IP: 192.222.164.54 X-Barracuda-RBL-IP: 192.222.164.54 To: gdb-patches@sourceware.org Cc: Simon Marchi <simon.marchi@polymtl.ca> Subject: [PATCH] Defer breakpoint reset when cloning progspace for fork child Date: Fri, 30 Mar 2018 15:01:32 -0400 X-ASG-Orig-Subj: [PATCH] Defer breakpoint reset when cloning progspace for fork child Message-Id: <20180330190132.20823-1-simon.marchi@polymtl.ca> X-Barracuda-Connect: smtp.electronicbox.net[96.127.255.82] X-Barracuda-Start-Time: 1522436493 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Barracuda-Scan-Msg-Size: 2284 X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.00 X-Barracuda-Spam-Status: No, SCORE=0.00 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests= X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.49442 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- X-IsSubscribed: yes |
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
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
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
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
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
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 --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; }