From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 994FC3858C52 for ; Tue, 17 Jan 2023 16:30:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 994FC3858C52 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673973033; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Me/1nqv83C9AdD+uZ7RU6gUEatbgB/jdA7Pfkb+K9zY=; b=EkKGN7tkiyKZ054Ez7IaYY3ivuLaEEvli6Ivq9EPGXolBJ4fLH/qYwbAHKwya3y7qtfvls zi1eBf3R9JLbkeXTM8TTzeyGa9l8x+gtnaZaUw9JWh5Z6u5pySlN3fJo+7MMpGIu4Jel+F LRphhCa7+T0NOfNONzWRE3zq2+vWDAk= Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-577--EkRqKOQOTOVCP0230e5Xg-1; Tue, 17 Jan 2023 11:30:32 -0500 X-MC-Unique: -EkRqKOQOTOVCP0230e5Xg-1 Received: by mail-qv1-f71.google.com with SMTP id i7-20020a056214020700b004ffce246a2bso16095504qvt.3 for ; Tue, 17 Jan 2023 08:30:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:to:from:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=bXZGQ7fJmI1yAaSr1p3r0Jwsgx4xK/nYpeMG4FqDfQg=; b=bLCFhCmJGk82jQIf76fvoCB94RW9lLH3UD7s32dL88mX90tClm/hhGcaYw6zRLoCL4 a3Kjj+bpHRpEGEG+VInDv0tqhnmBz/nWh02/4f8Cj5132xeoWJ21DboV1vJx4lZUZ/Jc 0sh37rlTuAFEN73/zdpCB9WvuRlOzX3hFpMhxKAyR145GKs2M1n9KEncPqB6dbvQ/353 oPUt32S6EGr8waGtJh9nFAJaH9IP4fAoCfFNsgx4MKipbkSelPcQ4cmAS3KKN3iaLqeM QK5icM4k5yeluhrg+lWCSdTpVFAG7A+fN5LTSWzxYVBsS1eCvz+vJaEao4K5On1ZUzV9 WtDQ== X-Gm-Message-State: AFqh2koIxCiwq7MuPjxu5AAJPVDkl1UwsY+OPznb1iXXc25v/H9GLp8m hoqeSzed6zRelJFu9eCNHA3aufD/iD42haGlmAIu4xoHV69yE6aNGSVW5JRac/LteTFKCUuID22 1n9x0eZ+cNxx0VtRbVxKrfA== X-Received: by 2002:a05:6214:3019:b0:532:33e4:2d70 with SMTP id ke25-20020a056214301900b0053233e42d70mr7803786qvb.12.1673973032069; Tue, 17 Jan 2023 08:30:32 -0800 (PST) X-Google-Smtp-Source: AMrXdXtBVkw4AsX2OR5hRNj6CPMDgm1EWnRw04nhUMkNSnLepZQkF40TeYcFpTlMoC2zNb2/5VzrmA== X-Received: by 2002:a05:6214:3019:b0:532:33e4:2d70 with SMTP id ke25-20020a056214301900b0053233e42d70mr7803725qvb.12.1673973031666; Tue, 17 Jan 2023 08:30:31 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id d6-20020a37b406000000b007049f19c736sm20143134qkf.7.2023.01.17.08.30.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Jan 2023 08:30:31 -0800 (PST) From: Andrew Burgess To: =?us-ascii?Q?AlexandraH=3D=3FUTF-8=3FB=3Fw6E=3D=3F=3Djkov=3D=3FUTF-8?= =?us-ascii?Q?=3FB=3Fw6E=3D=3F=3D?=@sourceware.org, gdb-patches@sourceware.org Subject: Re: [PATCH v2] remote.c: Allow inferior to reply with an error In-Reply-To: <20230113115910.3215524-1-ahajkova@redhat.com> References: <20230113115910.3215524-1-ahajkova@redhat.com> Date: Tue, 17 Jan 2023 16:30:29 +0000 Message-ID: <87zgahdsyi.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: AlexandraH=C3=A1jkov=C3=A1@sourceware.org writes: > From: Alexandra H=C3=A1jkov=C3=A1 > > When gdb communicates with some kind of gdbserver or gdbserver > stub over the remote protocol, the only possible response to > the QSetWorkingDir packet is "OK". If the remote will reply > with anything else, gdb will complain about the unexpected reply. > > [remote] Sending packet: $QSetWorkingDir:2f746d70#bb > [remote] Packet received: E00 > Remote replied unexpectedly while setting the inferior's working > directory: E00 > (gdb) > > Allow remote to send an error message over as a QSetWorkingDir > packet reply. > > [remote] Sending packet: $QSetWorkingDir:2f746d70#bb > [remote] Packet received: E.directory does not exist > Remote failed to set working directory: directory does not exist. So, I started reviewing this patch, and wrote a whole bunch of stuff. One thing I wrote was, you should really add a test for this that uses the existing gdbserver. So, I thought, I wonder what errors gdbserver currently sends back, so I took a look at the QSetWorkingDir handling in gdbserver/server.cc. Weird, I thought, it doesn't appear to send back any errors. So then I took a look at set_inferior_cwd in gdbserver/inferiors.cc, and all that it does is cache the requested directory. So, then I went to the docs again, and re-read what we say about QSetWorkingDir: This packet is used to inform the remote server of the intended current working directory for programs that are going to be executed. Note the use of the word 'intended'. This packet does not try to change the directory NOW, it will try to change the directory LATER, when the inferior is actually executed. My current reading of the docs is that the QSetWorkingDir packet, is either not supported, or should never fail. If you specify an invalid directory then future attempts to start an inferior will fail, but the QSetWorkingDir packet itself shouldn't fail. I guess we could decide that we want to extend QSetWorkingDir to allow for it to send back error packets, I'm not sure that's entirely a good idea though as that's not how the existing gdbserver works... but if we did want to do that then we should probably improve the documentation to explain that some gdbservers will cache the passed in directory and return an error later, while others might choose to immediately change directory and return an error now. Because I already wrote it, I've left all my original review comments inline below, but I'm not sure if you will want to roll a v3 or not. Thanks, Andrew > > --- > V2 does not change the behaviour of gdb in a case it wasn't possible to > set the inferior's working directory. It just allows to pass the error > message to gdb. > > gdb/doc/gdb.texinfo | 3 +++ > gdb/remote.c | 12 +++++++++--- > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index 9c0018ea5c1..5df9a5a9178 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -42517,6 +42517,9 @@ Reply: > @table @samp > @item OK > The request succeeded. > + > +@item E.errtext > +An error occurred. Reply with an error message. I think that we should not limit the description here to just 'E.errtext', all the other error packets also support the 'Enn' format, and for consistency, I think we should document that as supported here too. > @end table > =20 > @item qfThreadInfo > diff --git a/gdb/remote.c b/gdb/remote.c > index 218bca30d04..ea89759e85a 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -10397,8 +10397,10 @@ remote_target::extended_remote_set_inferior_cwd = () > if (packet_support (PACKET_QSetWorkingDir) !=3D PACKET_DISABLE) > { > const std::string &inferior_cwd =3D current_inferior ()->cwd (); > + char *buf; > remote_state *rs =3D get_remote_state (); > =20 > + buf =3D rs->buf.data (); Given that BUF is only used later within the 'if (packet_ok (...))' block, I'd move the declaration, and definition of BUF into that block. Also, for C++, you don't need to declare the variable at the start of the block, just 'char *buf =3D ....' is fine. > if (!inferior_cwd.empty ()) > =09{ > =09 std::string hexpath > @@ -10420,11 +10422,15 @@ remote_target::extended_remote_set_inferior_cwd= () > getpkt (&rs->buf, 0); > if (packet_ok (rs->buf, > =09=09 &remote_protocol_packets[PACKET_QSetWorkingDir]) > -=09 !=3D PACKET_OK) > -=09error (_("\ > +=09 !=3D PACKET_OK) { A bunch of the following lines are indented entirely with spaces. Unfortunately GNU style is a mix of tabs and spaces, so you need to figure out how to configure your editor to indent lines correctly. I'm surprised that this doesn't get highlighted in a 'git diff' / 'git show' output due to our .gitattributes file. > + if (buf[0] =3D=3D 'E' && buf[1] =3D=3D '.') > +=09 error (_("Remote failed to set working directory: %s"), buf + 2); > + else > + error (_("\ > Remote replied unexpectedly while setting the inferior's working\n\ I think we should handle both error packet formats here. Also, while looking at this code, I noticed that we don't appear to check the contents of the reply packet in the PACKET_OK case. The docs say the reply should be 'OK', but, we actually accept anything. Maybe we should "fix" that while we're here? I'd suggest something like this: switch (packet_ok (rs->buf, =09=09=09 &remote_protocol_packets[PACKET_QSetWorkingDir])) =09{ =09case PACKET_OK: =09 if (strcmp (rs->buf.data (), "OK") =3D=3D 0) =09 break; =09 error (_("\ Remote replied unexpectedly while setting the inferior's working\n=09\ directory: %s."), =09=09 rs->buf.data ()); =09case PACKET_ERROR: =09 { =09 char *buf =3D rs->buf.data (); =09 if (buf[0] =3D=3D 'E' && buf[1] =3D=3D '.' && buf[2] !=3D '\0') =09 error (_("Remote failed to set working directory: %s"), buf + 2); =09 else =09 error (_("Remote failed to set working directory: %s"), buf); =09 } =09case PACKET_UNKNOWN: =09 error (_("Remote does not support changing the working directory")); =09} This includes support for the 'E.msg' style error that you're adding, but also allows for the more generic Enn error that all the other packets seem to support. In the PACKET_OK case we check that the remote sent back and 'OK', and if not we get to reuse the existing unexpected reply error. And finally, we handle the PACKET_UNKNOWN case with an appropriate error. > -directory: %s"), > +directory: %s."), > =09 rs->buf.data ()); With the definition of BUF now in this block, I'd probably replace the 'rs->buf.data()' with a reuse of BUF, I think that's going to appear more consistent. > + } > =20 > } > } > --=20 > 2.39.0