[PATCH] remote.c: Allow inferior to reply with an error

[PATCH] remote.c: Allow inferior to reply with an error

[@ 2023-01-04 11:39 UTC (permalink / raw)]

From: Alexandra Hájková <ahajkova@redhat.com>

When gdb communicates with the inferior with the remote
protocol, the only possible response to the QSetWorkingDir
packet is "OK". If the inferior will reply with anything
else, gdb will complain about the unexpected reply and stop
its communication with the inferior.

[remote] Sending packet: $QSetWorkingDir:2f746d70#bb
[remote] Packet received: E00
Remote replied unexpectedly while setting the inferior's working
directory: E00
(gdb)

But setting the inferior's working dir is not always possible due
to various reasons and we may not want to always stop the communication.
This patch proposes to just warn the user in a case it wasn't possible
to set the working dir but proceed without an error.

[remote] Sending packet: $QSetWorkingDir:2f746d70#bb
remote] Packet received: E00
warning: Remote failed to set the inferior's working directory: E00
[remote] Sending packet: $vRun;2f7573722f62696e2f6563686f#3d
---
 gdb/doc/gdb.texinfo | 3 +++
 gdb/remote.c        | 8 +++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index dd8f8bc757c..54cab3afa9c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -42515,6 +42515,9 @@ Reply:
 @table @samp
 @item OK
 The request succeeded.
+
+@item E @var{nn}
+An error occurred.  The error number @var{nn} is given as hex digits.
 @end table
 
 @item qfThreadInfo
diff --git a/gdb/remote.c b/gdb/remote.c
index 218bca30d04..db5b949a28b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10421,11 +10421,9 @@ remote_target::extended_remote_set_inferior_cwd ()
       if (packet_ok (rs->buf,
 		     &remote_protocol_packets[PACKET_QSetWorkingDir])
 	  != PACKET_OK)
-	error (_("\
-Remote replied unexpectedly while setting the inferior's working\n\
-directory: %s"),
-	       rs->buf.data ());
-
+	warning (_("\
+Remote failed to set the inferior's working directory: %s"),
+                 rs->buf.data ());
     }
 }
 
-- 
2.38.1

Re: [PATCH] remote.c: Allow inferior to reply with an error

[Eli Zaretskii]

> From: AlexandraHájková@sourceware.org
> Date: Wed,  4 Jan 2023 12:39:09 +0100
> From: Alexandra Hájková <ahajkova@redhat.com>
> 
> When gdb communicates with the inferior with the remote
> protocol, the only possible response to the QSetWorkingDir
> packet is "OK". If the inferior will reply with anything
> else, gdb will complain about the unexpected reply and stop
> its communication with the inferior.
> 
> [remote] Sending packet: $QSetWorkingDir:2f746d70#bb
> [remote] Packet received: E00
> Remote replied unexpectedly while setting the inferior's working
> directory: E00
> (gdb)
> 
> But setting the inferior's working dir is not always possible due
> to various reasons and we may not want to always stop the communication.
> This patch proposes to just warn the user in a case it wasn't possible
> to set the working dir but proceed without an error.
> 
> [remote] Sending packet: $QSetWorkingDir:2f746d70#bb
> remote] Packet received: E00
> warning: Remote failed to set the inferior's working directory: E00
> [remote] Sending packet: $vRun;2f7573722f62696e2f6563686f#3d
> ---
>  gdb/doc/gdb.texinfo | 3 +++
>  gdb/remote.c        | 8 +++-----
>  2 files changed, 6 insertions(+), 5 deletions(-)

OK for the documentation part.

Thanks.

Re: [PATCH] remote.c: Allow inferior to reply with an error

[Tom Tromey]

>>>>> AlexandraHájková  <AlexandraHájková@sourceware.org> writes:

> But setting the inferior's working dir is not always possible due
> to various reasons and we may not want to always stop the communication.
> This patch proposes to just warn the user in a case it wasn't possible
> to set the working dir but proceed without an error.

Thank you, this is ok.

Tom

Re: [PATCH] remote.c: Allow inferior to reply with an error

[Andrew Burgess]

AlexandraHájková@sourceware.org writes:

> From: Alexandra Hájková <ahajkova@redhat.com>
>
> When gdb communicates with the inferior with the remote
> protocol, the only possible response to the QSetWorkingDir
> packet is "OK". If the inferior will reply with anything
> else, gdb will complain about the unexpected reply and stop
> its communication with the inferior.
>
> [remote] Sending packet: $QSetWorkingDir:2f746d70#bb
> [remote] Packet received: E00
> Remote replied unexpectedly while setting the inferior's working
> directory: E00
> (gdb)
>
> But setting the inferior's working dir is not always possible due
> to various reasons and we may not want to always stop the communication.
> This patch proposes to just warn the user in a case it wasn't possible
> to set the working dir but proceed without an error.
>
> [remote] Sending packet: $QSetWorkingDir:2f746d70#bb
> remote] Packet received: E00
> warning: Remote failed to set the inferior's working directory: E00
> [remote] Sending packet: $vRun;2f7573722f62696e2f6563686f#3d
> ---
>  gdb/doc/gdb.texinfo | 3 +++
>  gdb/remote.c        | 8 +++-----

Changes like this should definitely have a test, but, in this case, I
have bigger worries, see below...

>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index dd8f8bc757c..54cab3afa9c 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -42515,6 +42515,9 @@ Reply:
>  @table @samp
>  @item OK
>  The request succeeded.
> +
> +@item E @var{nn}
> +An error occurred.  The error number @var{nn} is given as hex digits.
>  @end table
>  
>  @item qfThreadInfo
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 218bca30d04..db5b949a28b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -10421,11 +10421,9 @@ remote_target::extended_remote_set_inferior_cwd ()
>        if (packet_ok (rs->buf,
>  		     &remote_protocol_packets[PACKET_QSetWorkingDir])
>  	  != PACKET_OK)
> -	error (_("\
> -Remote replied unexpectedly while setting the inferior's working\n\
> -directory: %s"),
> -	       rs->buf.data ());
> -
> +	warning (_("\
> +Remote failed to set the inferior's working directory: %s"),
> +                 rs->buf.data ());

I don't think this should be merged.

I don't think changing this error into a warning is a good idea.
Surely, if GDB can't switch to the directory that the user expects, then
we should stop and tell the user that we can't do what they've asked,
then the user could update things and try again.

As a (maybe extreme) example, what if the user was debugging 'rm -f *',
but first changed from their $HOME directory to '/tmp/test-dir/'.  If
the requested directory doesn't exist then we're going to just carry on
and run the test in $HOME .... that seems like a bad idea to me.

Thanks,
Andrew

Re: [PATCH] remote.c: Allow inferior to reply with an error

[Simon Marchi]

> I don't think this should be merged.
> 
> I don't think changing this error into a warning is a good idea.
> Surely, if GDB can't switch to the directory that the user expects, then
> we should stop and tell the user that we can't do what they've asked,
> then the user could update things and try again.
> 
> As a (maybe extreme) example, what if the user was debugging 'rm -f *',
> but first changed from their $HOME directory to '/tmp/test-dir/'.  If
> the requested directory doesn't exist then we're going to just carry on
> and run the test in $HOME .... that seems like a bad idea to me.
I agree.

Simon

Re: [PATCH] remote.c: Allow inferior to reply with an error

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

Ping

On Wed, Jan 4, 2023 at 12:39 PM <AlexandraHájková@sourceware.org> wrote:

> From: Alexandra Hájková <ahajkova@redhat.com>
>
> When gdb communicates with the inferior with the remote
> protocol, the only possible response to the QSetWorkingDir
> packet is "OK". If the inferior will reply with anything
> else, gdb will complain about the unexpected reply and stop
> its communication with the inferior.
>
> [remote] Sending packet: $QSetWorkingDir:2f746d70#bb
> [remote] Packet received: E00
> Remote replied unexpectedly while setting the inferior's working
> directory: E00
> (gdb)
>
> But setting the inferior's working dir is not always possible due
> to various reasons and we may not want to always stop the communication.
> This patch proposes to just warn the user in a case it wasn't possible
> to set the working dir but proceed without an error.
>
> [remote] Sending packet: $QSetWorkingDir:2f746d70#bb
> remote] Packet received: E00
> warning: Remote failed to set the inferior's working directory: E00
> [remote] Sending packet: $vRun;2f7573722f62696e2f6563686f#3d
> ---
>  gdb/doc/gdb.texinfo | 3 +++
>  gdb/remote.c        | 8 +++-----
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index dd8f8bc757c..54cab3afa9c 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -42515,6 +42515,9 @@ Reply:
>  @table @samp
>  @item OK
>  The request succeeded.
> +
> +@item E @var{nn}
> +An error occurred.  The error number @var{nn} is given as hex digits.
>  @end table
>
>  @item qfThreadInfo
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 218bca30d04..db5b949a28b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -10421,11 +10421,9 @@ remote_target::extended_remote_set_inferior_cwd ()
>        if (packet_ok (rs->buf,
>                      &remote_protocol_packets[PACKET_QSetWorkingDir])
>           != PACKET_OK)
> -       error (_("\
> -Remote replied unexpectedly while setting the inferior's working\n\
> -directory: %s"),
> -              rs->buf.data ());
> -
> +       warning (_("\
> +Remote failed to set the inferior's working directory: %s"),
> +                 rs->buf.data ());
>      }
>  }
>
> --
> 2.38.1
>
>

Re: [PATCH] remote.c: Allow inferior to reply with an error

[Tom Tromey]

>>>>> "Alexandra" == Alexandra Petlanova Hajkova via Gdb-patches <gdb-patches@sourceware.org> writes:

Alexandra> Ping

I think there were a few replies to this one on the list.

Tom