* [PATCH 0/3] "run" and "attach" failure handling problems @ 2024-02-12 20:01 Pedro Alves 2024-02-12 20:01 ` [PATCH 1/3] Fix "run" failure with GDBserver Pedro Alves ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Pedro Alves @ 2024-02-12 20:01 UTC (permalink / raw) To: gdb-patches While looking at gdb.base/attach.exp testsuite results on native Cygwin, I noticed that "attach PID" or "run" would hang if the previous command was an "attach" or "run" command that failed. After fixing that on the Windows backend, I wrote a testcase for the "run" scenario, and that revealed that GDBserver (all ports) also did not handle that well. This series thus fixes GDBserver, adds said testcase, and then finally fixes native Cygwin. Pedro Alves (3): Fix "run" failure with GDBserver Improve vRun error reporting Windows: Fix run/attach hang after bad run/attach gdb/remote.c | 23 +++++++- gdb/testsuite/gdb.base/run-fail-twice.exp | 67 +++++++++++++++++++++++ gdb/windows-nat.c | 15 +++-- gdbserver/server.cc | 10 +++- 4 files changed, 106 insertions(+), 9 deletions(-) create mode 100644 gdb/testsuite/gdb.base/run-fail-twice.exp base-commit: cda750802aef3beea582f0f3cad824be491abb4d -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] Fix "run" failure with GDBserver 2024-02-12 20:01 [PATCH 0/3] "run" and "attach" failure handling problems Pedro Alves @ 2024-02-12 20:01 ` Pedro Alves 2024-02-13 15:19 ` Lancelot SIX 2024-02-12 20:01 ` [PATCH 2/3] Improve vRun error reporting Pedro Alves 2024-02-12 20:01 ` [PATCH 3/3] Windows: Fix run/attach hang after bad run/attach Pedro Alves 2 siblings, 1 reply; 11+ messages in thread From: Pedro Alves @ 2024-02-12 20:01 UTC (permalink / raw) To: gdb-patches If starting the inferior process with "run" (vRun packet) fails, GDBserver throws an error that escapes all the way to the top level. When an error escapes all the way like that, GDBserver interprets it as a disconnection, and either goes back to waiting for a new GDB connection, or exits, if --once was specified. E.g., with the testcase program added by this commit, we see: On GDB side: ... (gdb) tar extended-remote :999 ... Remote debugging using :9999 (gdb) r Starting program: Running ".../gdb.base/run-fail-twice/run-fail-twice.nox" on the remote target failed (gdb) On GDBserver side: $ gdbserver --once --multi :9999 Remote debugging from host 127.0.0.1, port 34344 bash: line 1: .../gdb.base/run-fail-twice/run-fail-twice.nox: Permission denied bash: line 1: exec: .../gdb.base/run-fail-twice/run-fail-twice.nox: cannot execute: Permission denied gdbserver: During startup program exited with code 126. $ # gdbserver exited This is wrong, as we've connected with extended-remote/--multi. GDBserver should just report an error to vCont, and continue connected to GDB, waiting for other commands. This commit fixes GDBserver by catching the error locally in handle_v_run. Change-Id: Ib386f267522603f554b52a885b15229c9639e870 --- gdb/testsuite/gdb.base/run-fail-twice.exp | 67 +++++++++++++++++++++++ gdbserver/server.cc | 10 +++- 2 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 gdb/testsuite/gdb.base/run-fail-twice.exp diff --git a/gdb/testsuite/gdb.base/run-fail-twice.exp b/gdb/testsuite/gdb.base/run-fail-twice.exp new file mode 100644 index 00000000000..2fda5c9fde5 --- /dev/null +++ b/gdb/testsuite/gdb.base/run-fail-twice.exp @@ -0,0 +1,67 @@ +# Copyright 2024 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Test doing a "run" that fails, and then another "run". + +# The purpose of this testcase is to test the "run" command. If we +# cannot use it, then there is no point in running this testcase. +require !use_gdb_stub + +standard_testfile + +if {[build_executable "failed to build" $testfile $srcfile {debug}]} { + return -1 +} + +proc test_run {testname} { + gdb_run_cmd + gdb_test_multiple "" $testname { + -re -wrap "During startup program exited with code 126\\." { + # What we get on GNU/Linux. + pass $gdb_test_name + } + -re -wrap "Error creating process.*" { + # What we get on Windows. + pass $gdb_test_name + } + -re -wrap "Running .* on the remote target failed" { + # What we get with older GDBserver and other remote + # targets. + pass $gdb_test_name + } + } +} + +proc_with_prefix test {} { + global gdb_prompt binfile + + clean_restart $binfile + + gdb_test_no_output "set confirm off" + + gdb_remote_download host $binfile $binfile.nox + remote_exec target "chmod \"a-x\" $binfile.nox" + gdb_test "exec-file $binfile.nox" \ + "" \ + "exec-file \$binfile.nox" + gdb_test "set remote exec-file $binfile.nox" \ + "" \ + "set remote exec-file \$binfile.nox" + + test_run "bad run 1" + test_run "bad run 2" +} + +test diff --git a/gdbserver/server.cc b/gdbserver/server.cc index e02cdb83b51..0967b194376 100644 --- a/gdbserver/server.cc +++ b/gdbserver/server.cc @@ -3025,7 +3025,15 @@ handle_v_run (char *own_buf) free_vector_argv (program_args); program_args = new_argv; - target_create_inferior (program_path.get (), program_args); + try + { + target_create_inferior (program_path.get (), program_args); + } + catch (const gdb_exception_error &exception) + { + sprintf (own_buf, "E.%s", exception.what ()); + return; + } if (cs.last_status.kind () == TARGET_WAITKIND_STOPPED) { -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] Fix "run" failure with GDBserver 2024-02-12 20:01 ` [PATCH 1/3] Fix "run" failure with GDBserver Pedro Alves @ 2024-02-13 15:19 ` Lancelot SIX 2024-02-13 21:11 ` Pedro Alves 0 siblings, 1 reply; 11+ messages in thread From: Lancelot SIX @ 2024-02-13 15:19 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Hi Pedro, On Mon, Feb 12, 2024 at 08:01:51PM +0000, Pedro Alves wrote: > If starting the inferior process with "run" (vRun packet) fails, > GDBserver throws an error that escapes all the way to the top level. > When an error escapes all the way like that, GDBserver interprets it > as a disconnection, and either goes back to waiting for a new GDB > connection, or exits, if --once was specified. > > E.g., with the testcase program added by this commit, we see: > > On GDB side: > > ... > (gdb) tar extended-remote :999 > ... > Remote debugging using :9999 > (gdb) r > Starting program: > Running ".../gdb.base/run-fail-twice/run-fail-twice.nox" on the remote target failed > (gdb) > > On GDBserver side: > > $ gdbserver --once --multi :9999 > Remote debugging from host 127.0.0.1, port 34344 > bash: line 1: .../gdb.base/run-fail-twice/run-fail-twice.nox: Permission denied > bash: line 1: exec: .../gdb.base/run-fail-twice/run-fail-twice.nox: cannot execute: Permission denied > gdbserver: During startup program exited with code 126. > $ # gdbserver exited > > This is wrong, as we've connected with extended-remote/--multi. > GDBserver should just report an error to vCont, and continue connected > to GDB, waiting for other commands. > > This commit fixes GDBserver by catching the error locally in > handle_v_run. > > Change-Id: Ib386f267522603f554b52a885b15229c9639e870 > --- > gdb/testsuite/gdb.base/run-fail-twice.exp | 67 +++++++++++++++++++++++ > gdbserver/server.cc | 10 +++- > 2 files changed, 76 insertions(+), 1 deletion(-) > create mode 100644 gdb/testsuite/gdb.base/run-fail-twice.exp > Looks like you forgot the gdb/testsuite/gdb.base/run-fail-twice.c file? > diff --git a/gdb/testsuite/gdb.base/run-fail-twice.exp b/gdb/testsuite/gdb.base/run-fail-twice.exp > new file mode 100644 > index 00000000000..2fda5c9fde5 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/run-fail-twice.exp > @@ -0,0 +1,67 @@ > +# Copyright 2024 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > + > +# Test doing a "run" that fails, and then another "run". > + > +# The purpose of this testcase is to test the "run" command. If we > +# cannot use it, then there is no point in running this testcase. > +require !use_gdb_stub > + > +standard_testfile > + > +if {[build_executable "failed to build" $testfile $srcfile {debug}]} { > + return -1 > +} > + > +proc test_run {testname} { > + gdb_run_cmd > + gdb_test_multiple "" $testname { > + -re -wrap "During startup program exited with code 126\\." { > + # What we get on GNU/Linux. > + pass $gdb_test_name > + } > + -re -wrap "Error creating process.*" { > + # What we get on Windows. > + pass $gdb_test_name > + } > + -re -wrap "Running .* on the remote target failed" { > + # What we get with older GDBserver and other remote > + # targets. > + pass $gdb_test_name > + } > + } > +} > + > +proc_with_prefix test {} { > + global gdb_prompt binfile > + > + clean_restart $binfile > + > + gdb_test_no_output "set confirm off" > + > + gdb_remote_download host $binfile $binfile.nox > + remote_exec target "chmod \"a-x\" $binfile.nox" > + gdb_test "exec-file $binfile.nox" \ Couldn't you use gdb_test_no_output and remove the 2nd argument? > + "" \ > + "exec-file \$binfile.nox" > + gdb_test "set remote exec-file $binfile.nox" \ > + "" \ Same here. > + "set remote exec-file \$binfile.nox" > + > + test_run "bad run 1" > + test_run "bad run 2" > +} > + > +test > diff --git a/gdbserver/server.cc b/gdbserver/server.cc > index e02cdb83b51..0967b194376 100644 > --- a/gdbserver/server.cc > +++ b/gdbserver/server.cc > @@ -3025,7 +3025,15 @@ handle_v_run (char *own_buf) > free_vector_argv (program_args); > program_args = new_argv; > > - target_create_inferior (program_path.get (), program_args); > + try > + { > + target_create_inferior (program_path.get (), program_args); > + } > + catch (const gdb_exception_error &exception) > + { > + sprintf (own_buf, "E.%s", exception.what ()); > + return; > + } > > if (cs.last_status.kind () == TARGET_WAITKIND_STOPPED) > { > > -- > 2.43.0 > Best, Lancelot ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] Fix "run" failure with GDBserver 2024-02-13 15:19 ` Lancelot SIX @ 2024-02-13 21:11 ` Pedro Alves 0 siblings, 0 replies; 11+ messages in thread From: Pedro Alves @ 2024-02-13 21:11 UTC (permalink / raw) To: Lancelot SIX; +Cc: gdb-patches On 2024-02-13 15:19, Lancelot SIX wrote: > Hi Pedro, Hi! > > On Mon, Feb 12, 2024 at 08:01:51PM +0000, Pedro Alves wrote: >> --- >> gdb/testsuite/gdb.base/run-fail-twice.exp | 67 +++++++++++++++++++++++ >> gdbserver/server.cc | 10 +++- >> 2 files changed, 76 insertions(+), 1 deletion(-) >> create mode 100644 gdb/testsuite/gdb.base/run-fail-twice.exp >> > > Looks like you forgot the gdb/testsuite/gdb.base/run-fail-twice.c file? Indeed... Added now. > >> diff --git a/gdb/testsuite/gdb.base/run-fail-twice.exp b/gdb/testsuite/gdb.base/run-fail-twice.exp >> + >> +# Test doing a "run" that fails, and then another "run". >> + >> +# The purpose of this testcase is to test the "run" command. If we >> +# cannot use it, then there is no point in running this testcase. >> +require !use_gdb_stub I've switched this to: require target_can_use_run_cmd >> + >> +standard_testfile >> + >> +if {[build_executable "failed to build" $testfile $srcfile {debug}]} { >> + return -1 >> +} >> + >> +proc test_run {testname} { >> + gdb_run_cmd I switched to calling "run" directly, as that's what we're testing anyhow. >> + gdb_test_multiple "" $testname { >> + -re -wrap "During startup program exited with code 126\\." { >> + # What we get on GNU/Linux. >> + pass $gdb_test_name >> + } >> + -re -wrap "Error creating process.*" { >> + # What we get on Windows. >> + pass $gdb_test_name >> + } >> + -re -wrap "Running .* on the remote target failed" { >> + # What we get with older GDBserver and other remote >> + # targets. >> + pass $gdb_test_name >> + } >> + } >> +} >> + >> +proc_with_prefix test {} { >> + global gdb_prompt binfile >> + >> + clean_restart $binfile >> + >> + gdb_test_no_output "set confirm off" >> + >> + gdb_remote_download host $binfile $binfile.nox >> + remote_exec target "chmod \"a-x\" $binfile.nox" >> + gdb_test "exec-file $binfile.nox" \ > > Couldn't you use gdb_test_no_output and remove the 2nd argument? I can! And I did. > >> + "" \ >> + "exec-file \$binfile.nox" >> + gdb_test "set remote exec-file $binfile.nox" \ >> + "" \ > > Same here. > Ditto. Here's the updated patch. ---- 8< ---- From 04b71816555898fa804a76aa0412b1bad1dc9692 Mon Sep 17 00:00:00 2001 From: Pedro Alves <pedro@palves.net> Subject: [PATCH] Fix "run" failure with GDBserver If starting the inferior process with "run" (vRun packet) fails, GDBserver throws an error that escapes all the way to the top level. When an error escapes all the way like that, GDBserver interprets it as a disconnection, and either goes back to waiting for a new GDB connection, or exits, if --once was specified. E.g., with the testcase program added by this commit, we see: On GDB side: ... (gdb) tar extended-remote :999 ... Remote debugging using :9999 (gdb) r Starting program: Running ".../gdb.base/run-fail-twice/run-fail-twice.nox" on the remote target failed (gdb) On GDBserver side: $ gdbserver --once --multi :9999 Remote debugging from host 127.0.0.1, port 34344 bash: line 1: .../gdb.base/run-fail-twice/run-fail-twice.nox: Permission denied bash: line 1: exec: .../gdb.base/run-fail-twice/run-fail-twice.nox: cannot execute: Permission denied gdbserver: During startup program exited with code 126. $ # gdbserver exited This is wrong, as we've connected with extended-remote/--multi. GDBserver should just report an error to vCont, and continue connected to GDB, waiting for other commands. This commit fixes GDBserver by catching the error locally in handle_v_run. Change-Id: Ib386f267522603f554b52a885b15229c9639e870 --- gdb/testsuite/gdb.base/run-fail-twice.c | 20 +++++++ gdb/testsuite/gdb.base/run-fail-twice.exp | 63 +++++++++++++++++++++++ gdbserver/server.cc | 10 +++- 3 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 gdb/testsuite/gdb.base/run-fail-twice.c create mode 100644 gdb/testsuite/gdb.base/run-fail-twice.exp diff --git a/gdb/testsuite/gdb.base/run-fail-twice.c b/gdb/testsuite/gdb.base/run-fail-twice.c new file mode 100644 index 00000000000..fddf841eb3e --- /dev/null +++ b/gdb/testsuite/gdb.base/run-fail-twice.c @@ -0,0 +1,20 @@ +/* Copyright 2024 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int +main (int argc, char **argv) +{ + return 0; +} diff --git a/gdb/testsuite/gdb.base/run-fail-twice.exp b/gdb/testsuite/gdb.base/run-fail-twice.exp new file mode 100644 index 00000000000..676fc486fbf --- /dev/null +++ b/gdb/testsuite/gdb.base/run-fail-twice.exp @@ -0,0 +1,63 @@ +# Copyright 2024 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Test doing a "run" that fails, and then another "run". + +require target_can_use_run_cmd + +standard_testfile + +if {[build_executable "failed to build" $testfile $srcfile {debug}]} { + return -1 +} + +proc test_run {testname} { + gdb_test_multiple "run" $testname { + -re -wrap "During startup program exited with code 126\\." { + # What we get on GNU/Linux. + pass $gdb_test_name + } + -re -wrap "Error creating process.*" { + # What we get on Windows. + pass $gdb_test_name + } + -re -wrap "Running .* on the remote target failed" { + # What we get with remote targets. + pass $gdb_test_name + } + } +} + +proc_with_prefix test {} { + global gdb_prompt binfile + + clean_restart $binfile + + gdb_test_no_output "set confirm off" + + gdb_remote_download host $binfile $binfile.nox + remote_exec target "chmod \"a-x\" $binfile.nox" + gdb_test_no_output \ + "exec-file $binfile.nox" \ + "exec-file \$binfile.nox" + gdb_test_no_output \ + "set remote exec-file $binfile.nox" \ + "set remote exec-file \$binfile.nox" + + test_run "bad run 1" + test_run "bad run 2" +} + +test diff --git a/gdbserver/server.cc b/gdbserver/server.cc index 74c7763d777..14a19bc1882 100644 --- a/gdbserver/server.cc +++ b/gdbserver/server.cc @@ -3428,7 +3428,15 @@ handle_v_run (char *own_buf) free_vector_argv (program_args); program_args = new_argv; - target_create_inferior (program_path.get (), program_args); + try + { + target_create_inferior (program_path.get (), program_args); + } + catch (const gdb_exception_error &exception) + { + sprintf (own_buf, "E.%s", exception.what ()); + return; + } if (cs.last_status.kind () == TARGET_WAITKIND_STOPPED) { base-commit: a16034bf6417dc2259fef43fd5bcc2dd1dac562f -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] Improve vRun error reporting 2024-02-12 20:01 [PATCH 0/3] "run" and "attach" failure handling problems Pedro Alves 2024-02-12 20:01 ` [PATCH 1/3] Fix "run" failure with GDBserver Pedro Alves @ 2024-02-12 20:01 ` Pedro Alves 2024-02-13 12:56 ` Pedro Alves 2024-02-13 15:36 ` Lancelot SIX 2024-02-12 20:01 ` [PATCH 3/3] Windows: Fix run/attach hang after bad run/attach Pedro Alves 2 siblings, 2 replies; 11+ messages in thread From: Pedro Alves @ 2024-02-12 20:01 UTC (permalink / raw) To: gdb-patches After the previous commit, if starting the inferior process with "run" (vRun packet) fails, GDBserver reports an error using the "E." verbose error packet. On the GDB side, however, GDB doesn't yet do anything with verbose error strings when handling vRun errors. This commit fixes that. This makes remote debugging output the same as native output, when possible, another small step in the "local/remote parity" project. E.g., before, against GNU/Linux GDBserver: (gdb) run Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox Running ".../gdb.base/run-fail-twice/run-fail-twice.nox" on the remote target failed After, against GNU/Linux GDBserver (same as native): (gdb) run Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox During startup program exited with code 126. Change-Id: Ib386f267522603f554b52a885b15229c9639e870 --- gdb/remote.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index b58dbd4cb66..dca5add260a 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -2400,6 +2400,19 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name, } } +/* Strings that starts with "E.", are verbose error messages, like + "E.ERROR_MESSAGE". If BUF is such an error message, return a + pointer to message after the period. Otherwise return NULL. */ + +static const char * +verbose_error_message (const char *buf) +{ + if (buf[0] == 'E' && buf[1] == '.') + return buf + 2; + else + return nullptr; +} + static enum packet_result packet_check_result (const char *buf) { @@ -2415,7 +2428,7 @@ packet_check_result (const char *buf) /* Always treat "E." as an error. This will be used for more verbose error messages, such as E.memtypes. */ - if (buf[0] == 'E' && buf[1] == '.') + if (verbose_error_message (buf) != nullptr) return PACKET_ERROR; /* The packet may or may not be OK. Just assume it is. */ @@ -10502,7 +10515,13 @@ remote_target::extended_remote_run (const std::string &args) case PACKET_UNKNOWN: return -1; case PACKET_ERROR: - if (remote_exec_file[0] == '\0') + /* If we have a verbose error message, print just that. This + makes remote debugging output the same as native output, when + possible. */ + if (const char *msg = verbose_error_message (rs->buf.data ()); + msg != nullptr) + error (("%s"), msg); + else if (remote_exec_file[0] == '\0') error (_("Running the default executable on the remote target failed; " "try \"set remote exec-file\"?")); else -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] Improve vRun error reporting 2024-02-12 20:01 ` [PATCH 2/3] Improve vRun error reporting Pedro Alves @ 2024-02-13 12:56 ` Pedro Alves 2024-02-13 15:36 ` Lancelot SIX 1 sibling, 0 replies; 11+ messages in thread From: Pedro Alves @ 2024-02-13 12:56 UTC (permalink / raw) To: gdb-patches Hi! This conflicts with Alexandra's just-merged packet_check_result changes. I see she has some follow up patches that would help me here. I will take a look at those. For the meantime, disregard this patch. Pedro Alves On 2024-02-12 20:01, Pedro Alves wrote: > After the previous commit, if starting the inferior process with "run" > (vRun packet) fails, GDBserver reports an error using the "E." verbose > error packet. On the GDB side, however, GDB doesn't yet do anything > with verbose error strings when handling vRun errors. This commit > fixes that. > > This makes remote debugging output the same as native output, when > possible, another small step in the "local/remote parity" project. > > E.g., before, against GNU/Linux GDBserver: > > (gdb) run > Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox > Running ".../gdb.base/run-fail-twice/run-fail-twice.nox" on the remote target failed > > After, against GNU/Linux GDBserver (same as native): > > (gdb) run > Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox > During startup program exited with code 126. > > Change-Id: Ib386f267522603f554b52a885b15229c9639e870 > --- > gdb/remote.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index b58dbd4cb66..dca5add260a 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -2400,6 +2400,19 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name, > } > } > > +/* Strings that starts with "E.", are verbose error messages, like > + "E.ERROR_MESSAGE". If BUF is such an error message, return a > + pointer to message after the period. Otherwise return NULL. */ > + > +static const char * > +verbose_error_message (const char *buf) > +{ > + if (buf[0] == 'E' && buf[1] == '.') > + return buf + 2; > + else > + return nullptr; > +} > + > static enum packet_result > packet_check_result (const char *buf) > { > @@ -2415,7 +2428,7 @@ packet_check_result (const char *buf) > > /* Always treat "E." as an error. This will be used for > more verbose error messages, such as E.memtypes. */ > - if (buf[0] == 'E' && buf[1] == '.') > + if (verbose_error_message (buf) != nullptr) > return PACKET_ERROR; > > /* The packet may or may not be OK. Just assume it is. */ > @@ -10502,7 +10515,13 @@ remote_target::extended_remote_run (const std::string &args) > case PACKET_UNKNOWN: > return -1; > case PACKET_ERROR: > - if (remote_exec_file[0] == '\0') > + /* If we have a verbose error message, print just that. This > + makes remote debugging output the same as native output, when > + possible. */ > + if (const char *msg = verbose_error_message (rs->buf.data ()); > + msg != nullptr) > + error (("%s"), msg); > + else if (remote_exec_file[0] == '\0') > error (_("Running the default executable on the remote target failed; " > "try \"set remote exec-file\"?")); > else ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] Improve vRun error reporting 2024-02-12 20:01 ` [PATCH 2/3] Improve vRun error reporting Pedro Alves 2024-02-13 12:56 ` Pedro Alves @ 2024-02-13 15:36 ` Lancelot SIX 1 sibling, 0 replies; 11+ messages in thread From: Lancelot SIX @ 2024-02-13 15:36 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Hi, I only glanced at this patch, but it seems to me that is would conflict with this patch: commit 7e9d8a3627c8a80b76c250b6881b7eb6fc2f4443 Date: Fri Nov 24 14:33:42 2023 +0100 remote.c: Make packet_check_result return a structure https://sourceware.org/pipermail/gdb-patches/2024-February/206483.html I expect that this patch would remove the need for verbose_error_message, you just need to have some changes in remote_target::extended_remote_run. Best, Lancelot. On Mon, Feb 12, 2024 at 08:01:52PM +0000, Pedro Alves wrote: > After the previous commit, if starting the inferior process with "run" > (vRun packet) fails, GDBserver reports an error using the "E." verbose > error packet. On the GDB side, however, GDB doesn't yet do anything > with verbose error strings when handling vRun errors. This commit > fixes that. > > This makes remote debugging output the same as native output, when > possible, another small step in the "local/remote parity" project. > > E.g., before, against GNU/Linux GDBserver: > > (gdb) run > Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox > Running ".../gdb.base/run-fail-twice/run-fail-twice.nox" on the remote target failed > > After, against GNU/Linux GDBserver (same as native): > > (gdb) run > Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox > During startup program exited with code 126. > > Change-Id: Ib386f267522603f554b52a885b15229c9639e870 > --- > gdb/remote.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index b58dbd4cb66..dca5add260a 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -2400,6 +2400,19 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name, > } > } > > +/* Strings that starts with "E.", are verbose error messages, like > + "E.ERROR_MESSAGE". If BUF is such an error message, return a > + pointer to message after the period. Otherwise return NULL. */ > + > +static const char * > +verbose_error_message (const char *buf) > +{ > + if (buf[0] == 'E' && buf[1] == '.') > + return buf + 2; > + else > + return nullptr; > +} > + > static enum packet_result > packet_check_result (const char *buf) > { > @@ -2415,7 +2428,7 @@ packet_check_result (const char *buf) ss> > /* Always treat "E." as an error. This will be used for > more verbose error messages, such as E.memtypes. */ > - if (buf[0] == 'E' && buf[1] == '.') > + if (verbose_error_message (buf) != nullptr) > return PACKET_ERROR; > > /* The packet may or may not be OK. Just assume it is. */ > @@ -10502,7 +10515,13 @@ remote_target::extended_remote_run (const std::string &args) > case PACKET_UNKNOWN: > return -1; > case PACKET_ERROR: > - if (remote_exec_file[0] == '\0') > + /* If we have a verbose error message, print just that. This > + makes remote debugging output the same as native output, when > + possible. */ > + if (const char *msg = verbose_error_message (rs->buf.data ()); > + msg != nullptr) > + error (("%s"), msg); > + else if (remote_exec_file[0] == '\0') > error (_("Running the default executable on the remote target failed; " > "try \"set remote exec-file\"?")); > else > > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] Windows: Fix run/attach hang after bad run/attach 2024-02-12 20:01 [PATCH 0/3] "run" and "attach" failure handling problems Pedro Alves 2024-02-12 20:01 ` [PATCH 1/3] Fix "run" failure with GDBserver Pedro Alves 2024-02-12 20:01 ` [PATCH 2/3] Improve vRun error reporting Pedro Alves @ 2024-02-12 20:01 ` Pedro Alves 2024-02-12 20:14 ` Hannes Domani 2 siblings, 1 reply; 11+ messages in thread From: Pedro Alves @ 2024-02-12 20:01 UTC (permalink / raw) To: gdb-patches On Cygwin, gdb.base/attach.exp exposes that a attach after a previously failed attach hangs: (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to digits-starting nonsense is prohibited attach 0 Can't attach to process 0 (error 2: The system cannot find the file specified.) (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to nonexistent process is prohibited attach 10644 FAIL: gdb.base/attach.exp: do_attach_failure_tests: first attach (timeout) The problem is that windows_nat_target::attach always returns success even if the attach fails. When we return success, the helper thread begins waiting for events (which will never come), and thus the next attach deadlocks on the do_synchronously call within windows_nat_target::attach. "run" has the same problem, which is exposed by the new gdb.base/run-fail-twice.exp testcase: (gdb) run Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox Error creating process .../gdb.base/run-fail-twice/run-fail-twice.nox, (error 6: The handle is invalid.) (gdb) PASS: gdb.base/run-fail-twice.exp: test: bad run 1 run Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox FAIL: gdb.base/run-fail-twice.exp: test: bad run 2 (timeout) The problem here is the same, except that this time it is windows_nat_target::create_inferior that returns the incorrect result. This commit fixes both the "attach" and "run" paths. The tests mentioned above now pass on Cygwin. Change-Id: I15ec9fa279aff269d4982b00f4ea7c25ae917239 --- gdb/windows-nat.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index b446afd72d8..5d2e23600e3 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -2043,7 +2043,7 @@ windows_nat_target::attach (const char *args, int from_tty) if (!ok) err = (unsigned) GetLastError (); - return true; + return ok; }); if (err.has_value ()) @@ -2642,12 +2642,15 @@ windows_nat_target::create_inferior (const char *exec_file, windows_init_thread_list (); do_synchronously ([&] () { - if (!create_process (nullptr, args, flags, w32_env, - inferior_cwd != nullptr ? infcwd : nullptr, - disable_randomization, - &si, &pi)) + BOOL ok = create_process (nullptr, args, flags, w32_env, + inferior_cwd != nullptr ? infcwd : nullptr, + disable_randomization, + &si, &pi); + + if (!ok) ret = (unsigned) GetLastError (); - return true; + + return ok; }); if (w32_env) -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] Windows: Fix run/attach hang after bad run/attach 2024-02-12 20:01 ` [PATCH 3/3] Windows: Fix run/attach hang after bad run/attach Pedro Alves @ 2024-02-12 20:14 ` Hannes Domani 2024-02-13 12:20 ` Pedro Alves 0 siblings, 1 reply; 11+ messages in thread From: Hannes Domani @ 2024-02-12 20:14 UTC (permalink / raw) To: gdb-patches, Pedro Alves Am Montag, 12. Februar 2024 um 21:02:54 MEZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben: > On Cygwin, gdb.base/attach.exp exposes that a attach after a > previously failed attach hangs: > > (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to digits-starting nonsense is prohibited > attach 0 > Can't attach to process 0 (error 2: The system cannot find the file specified.) > (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to nonexistent process is prohibited > attach 10644 > FAIL: gdb.base/attach.exp: do_attach_failure_tests: first attach (timeout) > > The problem is that windows_nat_target::attach always returns success > even if the attach fails. When we return success, the helper thread > begins waiting for events (which will never come), and thus the next > attach deadlocks on the do_synchronously call within > windows_nat_target::attach. > > "run" has the same problem, which is exposed by the new > gdb.base/run-fail-twice.exp testcase: > > (gdb) run > Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox > Error creating process .../gdb.base/run-fail-twice/run-fail-twice.nox, (error 6: The handle is invalid.) > (gdb) PASS: gdb.base/run-fail-twice.exp: test: bad run 1 > run > Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox > FAIL: gdb.base/run-fail-twice.exp: test: bad run 2 (timeout) > > The problem here is the same, except that this time it is > windows_nat_target::create_inferior that returns the incorrect result. > > This commit fixes both the "attach" and "run" paths. The tests > mentioned above now pass on Cygwin. > > Change-Id: I15ec9fa279aff269d4982b00f4ea7c25ae917239 > --- > gdb/windows-nat.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c > index b446afd72d8..5d2e23600e3 100644 > --- a/gdb/windows-nat.c > +++ b/gdb/windows-nat.c > @@ -2043,7 +2043,7 @@ windows_nat_target::attach (const char *args, int from_tty) > if (!ok) > err = (unsigned) GetLastError (); > > - return true; > + return ok; > }); > > if (err.has_value ()) > @@ -2642,12 +2642,15 @@ windows_nat_target::create_inferior (const char *exec_file, > windows_init_thread_list (); > do_synchronously ([&] () > { > - if (!create_process (nullptr, args, flags, w32_env, > - inferior_cwd != nullptr ? infcwd : nullptr, > - disable_randomization, > - &si, &pi)) > + BOOL ok = create_process (nullptr, args, flags, w32_env, > + inferior_cwd != nullptr ? infcwd : nullptr, > + disable_randomization, > + &si, &pi); > + > + if (!ok) > ret = (unsigned) GetLastError (); > - return true; > + > + return ok; > }); > > if (w32_env) > > -- > 2.43.0 The same problem also exists in the !__CYGWIN__ branch, I ran into this multiple times already. Hannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] Windows: Fix run/attach hang after bad run/attach 2024-02-12 20:14 ` Hannes Domani @ 2024-02-13 12:20 ` Pedro Alves 2024-02-13 21:14 ` Pedro Alves 0 siblings, 1 reply; 11+ messages in thread From: Pedro Alves @ 2024-02-13 12:20 UTC (permalink / raw) To: Hannes Domani, gdb-patches On 2024-02-12 20:14, Hannes Domani wrote: > The same problem also exists in the !__CYGWIN__ branch, I ran into this > multiple times already. Wow, the #ifdef region is so long that I didn't notice this particular code was Cygwin-only. I had put "Windows" in the subject as I thought I was fixing native Windows too... I'll go fix the !__CYGWIN__ branches too. Thanks! Pedro Alves ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] Windows: Fix run/attach hang after bad run/attach 2024-02-13 12:20 ` Pedro Alves @ 2024-02-13 21:14 ` Pedro Alves 0 siblings, 0 replies; 11+ messages in thread From: Pedro Alves @ 2024-02-13 21:14 UTC (permalink / raw) To: Hannes Domani, gdb-patches On 2024-02-13 12:20, Pedro Alves wrote: > On 2024-02-12 20:14, Hannes Domani wrote: > >> The same problem also exists in the !__CYGWIN__ branch, I ran into this >> multiple times already. > > Wow, the #ifdef region is so long that I didn't notice this particular code was Cygwin-only. > > I had put "Windows" in the subject as I thought I was fixing native Windows too... > > I'll go fix the !__CYGWIN__ branches too. > Here's an updated version that now handles the !__CYGWIN__ branch too. I confirmed manually that I saw the hangs on MinGW gdb without the fix, and that both the "attach" and "run" hangs go away with this version patch (the previous version still hanged with "run"). ---- 8< ---- From af5cae1e2c0bf2cac2c5178d45f70eae5795cf8f Mon Sep 17 00:00:00 2001 From: Pedro Alves <pedro@palves.net> Subject: [PATCH] Windows: Fix run/attach hang after bad run/attach On Cygwin, gdb.base/attach.exp exposes that a attach after a previously failed attach hangs: (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to digits-starting nonsense is prohibited attach 0 Can't attach to process 0 (error 2: The system cannot find the file specified.) (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to nonexistent process is prohibited attach 10644 FAIL: gdb.base/attach.exp: do_attach_failure_tests: first attach (timeout) The problem is that windows_nat_target::attach always returns success even if the attach fails. When we return success, the helper thread begins waiting for events (which will never come), and thus the next attach deadlocks on the do_synchronously call within windows_nat_target::attach. "run" has the same problem, which is exposed by the new gdb.base/run-fail-twice.exp testcase: (gdb) run Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox Error creating process .../gdb.base/run-fail-twice/run-fail-twice.nox, (error 6: The handle is invalid.) (gdb) PASS: gdb.base/run-fail-twice.exp: test: bad run 1 run Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox FAIL: gdb.base/run-fail-twice.exp: test: bad run 2 (timeout) The problem here is the same, except that this time it is windows_nat_target::create_inferior that returns the incorrect result. This commit fixes both the "attach" and "run" paths, and the latter both the Cygwin and MinGW paths. The tests mentioned above now pass on Cygwin. Confirmed the fixes manually for MinGW GDB. Change-Id: I15ec9fa279aff269d4982b00f4ea7c25ae917239 --- gdb/windows-nat.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index 7f3044fc61d..5c47dd40738 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -2059,7 +2059,7 @@ windows_nat_target::attach (const char *args, int from_tty) if (!ok) err = (unsigned) GetLastError (); - return true; + return ok; }); if (err.has_value ()) @@ -2664,12 +2664,15 @@ windows_nat_target::create_inferior (const char *exec_file, windows_init_thread_list (); do_synchronously ([&] () { - if (!create_process (nullptr, args, flags, w32_env, - inferior_cwd != nullptr ? infcwd : nullptr, - disable_randomization, - &si, &pi)) + BOOL ok = create_process (nullptr, args, flags, w32_env, + inferior_cwd != nullptr ? infcwd : nullptr, + disable_randomization, + &si, &pi); + + if (!ok) ret = (unsigned) GetLastError (); - return true; + + return ok; }); if (w32_env) @@ -2790,16 +2793,18 @@ windows_nat_target::create_inferior (const char *exec_file, windows_init_thread_list (); do_synchronously ([&] () { - if (!create_process (nullptr, /* image */ - args, /* command line */ - flags, /* start flags */ - w32env, /* environment */ - inferior_cwd, /* current directory */ - disable_randomization, - &si, - &pi)) + BOOL ok = create_process (nullptr, /* image */ + args, /* command line */ + flags, /* start flags */ + w32env, /* environment */ + inferior_cwd, /* current directory */ + disable_randomization, + &si, + &pi); + if (!ok) ret = (unsigned) GetLastError (); - return true; + + return ok; }); if (tty != INVALID_HANDLE_VALUE) CloseHandle (tty); base-commit: a16034bf6417dc2259fef43fd5bcc2dd1dac562f -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-02-13 21:14 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-12 20:01 [PATCH 0/3] "run" and "attach" failure handling problems Pedro Alves 2024-02-12 20:01 ` [PATCH 1/3] Fix "run" failure with GDBserver Pedro Alves 2024-02-13 15:19 ` Lancelot SIX 2024-02-13 21:11 ` Pedro Alves 2024-02-12 20:01 ` [PATCH 2/3] Improve vRun error reporting Pedro Alves 2024-02-13 12:56 ` Pedro Alves 2024-02-13 15:36 ` Lancelot SIX 2024-02-12 20:01 ` [PATCH 3/3] Windows: Fix run/attach hang after bad run/attach Pedro Alves 2024-02-12 20:14 ` Hannes Domani 2024-02-13 12:20 ` Pedro Alves 2024-02-13 21:14 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).