Message ID | 1456324014-17961-1-git-send-email-lgustavo@codesourcery.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 6293 invoked by alias); 24 Feb 2016 14:27:16 -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 6183 invoked by uid 89); 24 Feb 2016 14:27:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=tripped, 1768, 0x2b, 176, 8 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 24 Feb 2016 14:27:12 +0000 Received: from svr-orw-fem-06.mgc.mentorg.com ([147.34.97.120]) by relay1.mentorg.com with esmtp id 1aYaPF-00065B-7i from Luis_Gustavo@mentor.com ; Wed, 24 Feb 2016 06:27:09 -0800 Received: from opsys.world.mentorg.com (147.34.91.1) by SVR-ORW-FEM-06.mgc.mentorg.com (147.34.97.120) with Microsoft SMTP Server id 14.3.224.2; Wed, 24 Feb 2016 06:27:06 -0800 From: Luis Machado <lgustavo@codesourcery.com> To: <gdb-patches@sourceware.org> CC: <palves@redhat.com>, <gbenson@redhat.com> Subject: [PATCH 1/2] Debugging without a binary (regression) Date: Wed, 24 Feb 2016 11:26:53 -0300 Message-ID: <1456324014-17961-1-git-send-email-lgustavo@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
Commit Message
Luis Machado
Feb. 24, 2016, 2:26 p.m. UTC
When we attempt to debug a process using GDBserver in standard remote mode without a symbol file on GDB's end, we may run into an issue where GDB cuts the connection attempt short due to an error. The error is caused by not being able to open a symbol file, like so: -- (gdb) set sysroot (gdb) tar rem :2345 Remote debugging using :2345 /proc/23769/exe: Permission denied. (gdb) i r The program has no registers now. (gdb) It should've been like this: (gdb) set sysroot (gdb) tar rem :2345 Remote debugging using :2345 0xf7ddb2d0 in ?? () (gdb) i r eax 0x0 0 ecx 0x0 0 edx 0x0 0 ebx 0x0 0 esp 0xffffdfa0 0xffffdfa0 ebp 0x0 0x0 esi 0x0 0 edi 0x0 0 eip 0xf7ddb2d0 0xf7ddb2d0 eflags 0x200 [ IF ] cs 0x33 51 ss 0x2b 43 ds 0x0 0 es 0x0 0 fs 0x0 0 gs 0x0 0 (gdb) This is caused by a couple of function calls within exec_file_locate_attach that can potentially throw errors. The following patch guards both exec_file_attach and symbol_file_add_main to prevent the errors from disrupting the connection process. There was also a case where native GDB tripped on this problem, but it was mostly fixed by bf74e428bca61022bd5cdf6bf28789a184748b4d. Regression-tested on x86-64/Ubuntu. gdb/ChangeLog: 2016-02-24 Luis Machado <lgustavo@codesourcery.com> * exec.c (exec_file_locate_attach): Guard a couple functions that can throw errors. --- gdb/exec.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
Comments
On 02/24/2016 02:26 PM, Luis Machado wrote: > When we attempt to debug a process using GDBserver in standard remote mode > without a symbol file on GDB's end, we may run into an issue where GDB cuts > the connection attempt short due to an error. The error is caused by not > being able to open a symbol file, like so: > > -- > > (gdb) set sysroot > (gdb) tar rem :2345 > Remote debugging using :2345 > /proc/23769/exe: Permission denied. > (gdb) i r > The program has no registers now. > (gdb) > > It should've been like this: > > (gdb) set sysroot > (gdb) tar rem :2345 > Remote debugging using :2345 > 0xf7ddb2d0 in ?? () > (gdb) i r > eax 0x0 0 > ecx 0x0 0 > edx 0x0 0 > ebx 0x0 0 > esp 0xffffdfa0 0xffffdfa0 > ebp 0x0 0x0 > esi 0x0 0 > edi 0x0 0 > eip 0xf7ddb2d0 0xf7ddb2d0 > eflags 0x200 [ IF ] > cs 0x33 51 > ss 0x2b 43 > ds 0x0 0 > es 0x0 0 > fs 0x0 0 > gs 0x0 0 > (gdb) > > This is caused by a couple of function calls within exec_file_locate_attach > that can potentially throw errors. > > The following patch guards both exec_file_attach and symbol_file_add_main to > prevent the errors from disrupting the connection process. > > There was also a case where native GDB tripped on this problem, but it was > mostly fixed by bf74e428bca61022bd5cdf6bf28789a184748b4d. > > Regression-tested on x86-64/Ubuntu. > > gdb/ChangeLog: > > 2016-02-24 Luis Machado <lgustavo@codesourcery.com> > > * exec.c (exec_file_locate_attach): Guard a couple functions > that can throw errors. > --- > gdb/exec.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/gdb/exec.c b/gdb/exec.c > index 90811c0..3f77b3d 100644 > --- a/gdb/exec.c > +++ b/gdb/exec.c > @@ -176,8 +176,35 @@ exec_file_locate_attach (int pid, int from_tty) > > old_chain = make_cleanup (xfree, full_exec_path); > > - exec_file_attach (full_exec_path, from_tty); > - symbol_file_add_main (full_exec_path, from_tty); > + /* exec_file_attach and symbol_file_add_main may throw an error if the file > + cannot be opened either locally or remotely. This happens when, for > + example, GDB connects to a GDBserver that is running on a different > + filesystem and the sysroot is set to non-target-based (no "target:"). The second sentence no longer makes sense after Gary's change. I think it should read: This happens for example, when the file is first found in the local sysroot (above), and then disappears (a TOCTOU race), or when it doesn't exist in the target filesystem, or when the file does exist, but is not readable. > + > + Then GDB will neither load the binary from the target nor be able to > + load a binary from the local filesystem (it may not exist in the local > + filesystem in the same path as in the remote filesystem). No longer makes sense either. > + > + Even without a binary, the remote-based debugging session should > + continue normally instead of ending abruptly. Hence we catch thrown > + errors/exceptions in the following code. */ > + TRY > + { > + exec_file_attach (full_exec_path, from_tty); > + } > + CATCH (err, RETURN_MASK_ALL) This should be RETURN_MASK_ERROR instead. Silently swallowing ctrl-c is rarely the right thing to do, if ever. A ctrl-c during the initial remote connection is supposed to bring down the connection immediately: ~~~ /* Start the remote connection. If error() or QUIT, discard this target (we'd otherwise be in an inconsistent state) and then propogate the error on up the exception chain. This ensures that the caller doesn't stumble along blindly assuming that the function succeeded. ~~~ static void remote_start_remote (int from_tty, struct target_ops *target, int extended_p) { struct remote_state *rs = get_remote_state (); struct packet_config *noack_config; char *wait_status = NULL; immediate_quit++; /* Allow user to interrupt it. */ QUIT; ~~~ We should probably similarly completely tear down an incomplete attach interrupted by ctrl-c too, but that's a separate issue. > + { > + } > + END_CATCH > + > + TRY > + { > + symbol_file_add_main (full_exec_path, from_tty); > + } > + CATCH (err, RETURN_MASK_ALL) > + { > + } Throwing is bad, but, should we warn, including the exception message? > + END_CATCH > > do_cleanups (old_chain); > } > Thanks, Pedro Alves
On 02/24/2016 10:06 PM, Pedro Alves wrote: > On 02/24/2016 02:26 PM, Luis Machado wrote: >> When we attempt to debug a process using GDBserver in standard remote mode >> without a symbol file on GDB's end, we may run into an issue where GDB cuts >> the connection attempt short due to an error. The error is caused by not >> being able to open a symbol file, like so: >> >> -- >> >> (gdb) set sysroot >> (gdb) tar rem :2345 >> Remote debugging using :2345 >> /proc/23769/exe: Permission denied. >> (gdb) i r >> The program has no registers now. >> (gdb) >> >> It should've been like this: >> >> (gdb) set sysroot >> (gdb) tar rem :2345 >> Remote debugging using :2345 >> 0xf7ddb2d0 in ?? () >> (gdb) i r >> eax 0x0 0 >> ecx 0x0 0 >> edx 0x0 0 >> ebx 0x0 0 >> esp 0xffffdfa0 0xffffdfa0 >> ebp 0x0 0x0 >> esi 0x0 0 >> edi 0x0 0 >> eip 0xf7ddb2d0 0xf7ddb2d0 >> eflags 0x200 [ IF ] >> cs 0x33 51 >> ss 0x2b 43 >> ds 0x0 0 >> es 0x0 0 >> fs 0x0 0 >> gs 0x0 0 >> (gdb) >> >> This is caused by a couple of function calls within exec_file_locate_attach >> that can potentially throw errors. >> >> The following patch guards both exec_file_attach and symbol_file_add_main to >> prevent the errors from disrupting the connection process. >> >> There was also a case where native GDB tripped on this problem, but it was >> mostly fixed by bf74e428bca61022bd5cdf6bf28789a184748b4d. >> >> Regression-tested on x86-64/Ubuntu. >> >> gdb/ChangeLog: >> >> 2016-02-24 Luis Machado <lgustavo@codesourcery.com> >> >> * exec.c (exec_file_locate_attach): Guard a couple functions >> that can throw errors. >> --- >> gdb/exec.c | 31 +++++++++++++++++++++++++++++-- >> 1 file changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/gdb/exec.c b/gdb/exec.c >> index 90811c0..3f77b3d 100644 >> --- a/gdb/exec.c >> +++ b/gdb/exec.c >> @@ -176,8 +176,35 @@ exec_file_locate_attach (int pid, int from_tty) >> >> old_chain = make_cleanup (xfree, full_exec_path); >> >> - exec_file_attach (full_exec_path, from_tty); >> - symbol_file_add_main (full_exec_path, from_tty); >> + /* exec_file_attach and symbol_file_add_main may throw an error if the file >> + cannot be opened either locally or remotely. This happens when, for >> + example, GDB connects to a GDBserver that is running on a different >> + filesystem and the sysroot is set to non-target-based (no "target:"). > > The second sentence no longer makes sense after Gary's change. > > I think it should read: > > This happens for example, when the file is first found in the local > sysroot (above), and then disappears (a TOCTOU race), or when it doesn't > exist in the target filesystem, or when the file does exist, but > is not readable. > Sounds good. Updated in the next version. >> + >> + Then GDB will neither load the binary from the target nor be able to >> + load a binary from the local filesystem (it may not exist in the local >> + filesystem in the same path as in the remote filesystem). > > No longer makes sense either. > Updated as well. >> + >> + Even without a binary, the remote-based debugging session should >> + continue normally instead of ending abruptly. Hence we catch thrown >> + errors/exceptions in the following code. */ >> + TRY >> + { >> + exec_file_attach (full_exec_path, from_tty); >> + } >> + CATCH (err, RETURN_MASK_ALL) > > This should be RETURN_MASK_ERROR instead. Silently swallowing ctrl-c is > rarely the right thing to do, if ever. A ctrl-c during the > initial remote connection is supposed to bring down the connection > immediately: Fixed >> + { >> + } >> + END_CATCH >> + >> + TRY >> + { >> + symbol_file_add_main (full_exec_path, from_tty); >> + } >> + CATCH (err, RETURN_MASK_ALL) >> + { >> + } > > Throwing is bad, but, should we warn, including > the exception message? That's what i originally did with a single try/catch block, but i noticed with two of them we are prone to having two warnings of the same kind, one after the other. So i ended up leaving them empty. For example: warning: /home/test/build-binutils-gdb/gdb/testsuite/outputs/gdb.server/attach-with-no-symbol/attach-with-no-symbol: Permission denied.^M warning: /home/test/build-binutils-gdb/gdb/testsuite/outputs/gdb.server/attach-with-no-symbol/attach-with-no-symbol: Permission denied.^M Shouldn't we go back to a single block? Checking if both exceptions' messages match seems a bit too much.
On 02/25/2016 01:36 AM, Luis Machado wrote: > That's what i originally did with a single try/catch block, but i > noticed with two of them we are prone to having two warnings of the same > kind, one after the other. So i ended up leaving them empty. > > For example: > > warning: > /home/test/build-binutils-gdb/gdb/testsuite/outputs/gdb.server/attach-with-no-symbol/attach-with-no-symbol: > Permission denied.^M > warning: > /home/test/build-binutils-gdb/gdb/testsuite/outputs/gdb.server/attach-with-no-symbol/attach-with-no-symbol: > Permission denied.^M > > Shouldn't we go back to a single block? I'd think not. Pedantically, the file could fail to load as executable but load as symbol file. Also, if both fail, and the errors are different, having both may give a better hint to the user to fix whatever needs fixing. > Checking if both exceptions' messages match seems a bit too much. That doesn't sound so bad to me. static int exception_print_same (struct gdb_exception e1, struct gdb_exception e2) { const char *msg1 = e1.message; const char *msg2 = e2.message; if (msg1 == NULL) msg1 = ""; if (msg2 == NULL) msg2 = ""; return (e1.reason == e2.reason && e1.error == e2.error && strcmp (e1.message, e2.message) == 0); } ... CATCH (err, ...) { warn err; prev_err = err; } ... CATCH (err, ...) { if (!exception_print_same (prev_err, err)) warn err; } Thanks, Pedro Alves
diff --git a/gdb/exec.c b/gdb/exec.c index 90811c0..3f77b3d 100644 --- a/gdb/exec.c +++ b/gdb/exec.c @@ -176,8 +176,35 @@ exec_file_locate_attach (int pid, int from_tty) old_chain = make_cleanup (xfree, full_exec_path); - exec_file_attach (full_exec_path, from_tty); - symbol_file_add_main (full_exec_path, from_tty); + /* exec_file_attach and symbol_file_add_main may throw an error if the file + cannot be opened either locally or remotely. This happens when, for + example, GDB connects to a GDBserver that is running on a different + filesystem and the sysroot is set to non-target-based (no "target:"). + + Then GDB will neither load the binary from the target nor be able to + load a binary from the local filesystem (it may not exist in the local + filesystem in the same path as in the remote filesystem). + + Even without a binary, the remote-based debugging session should + continue normally instead of ending abruptly. Hence we catch thrown + errors/exceptions in the following code. */ + TRY + { + exec_file_attach (full_exec_path, from_tty); + } + CATCH (err, RETURN_MASK_ALL) + { + } + END_CATCH + + TRY + { + symbol_file_add_main (full_exec_path, from_tty); + } + CATCH (err, RETURN_MASK_ALL) + { + } + END_CATCH do_cleanups (old_chain); }