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 5F55B3858D35 for ; Wed, 18 Jan 2023 18:19:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5F55B3858D35 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=1674065984; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4FoxbrtTe6tSQQIr+3+Apdpg9ZwDdHe6yadPRNFqO88=; b=N47mP40DhBZJ5q7RYYjDUVrGfZAJiGSKZeg3kIdUMX5/t81ZmU3XMgW6U2enDW8PgwtQFL bsYYsPCcks8dk5xzEg/kx6O4W0bp6OZ5vHzTDCBBD3F7P2fefCtcaR9rtyWbHbMcbLRgkc hbyd3lfc939r1tI82LnqOQqyj/aJpjs= Received: from mail-vs1-f71.google.com (mail-vs1-f71.google.com [209.85.217.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-653-Sl3sXFqFM7udb07bctOpQg-1; Wed, 18 Jan 2023 13:19:43 -0500 X-MC-Unique: Sl3sXFqFM7udb07bctOpQg-1 Received: by mail-vs1-f71.google.com with SMTP id p9-20020a056102200900b003d0a4ef871aso7912043vsr.6 for ; Wed, 18 Jan 2023 10:19:42 -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:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=4FoxbrtTe6tSQQIr+3+Apdpg9ZwDdHe6yadPRNFqO88=; b=oP9X+xY8WK9SrJnpyXxptnS17fcqux/zU5OFvxsrF3FquZrGmXpjTYGtIf4/15ouvs dQzbQYJDJlnE/EoCg5+XXJsMBI+rOqK/e/YaK1vQh3gI5XQoYvnEAkOKy2NgU8tjI62k hm2h4lc9/BrjMhMzdDMCPXgp+Yb035soq5EKvQcTy0Y6mUnP+M63IbZdkNdtFkReG6ZN 71oCsdgPK4R6Lkf4vO/hrZj0lG0llXSe7MjvwZGSNfgCJDqUnOkuf7hgLY/vckSlU8EJ DtiF3hTah9/Hotstwk2Or/2OVGQrxasQ/y0XNMdSOWDa0nFBPK/XerMsyNNUg+ckPWUm aalw== X-Gm-Message-State: AFqh2kpccFR9B6DkZ3XLouNKHL7jOvyogmOCAPz6Lj+FZVSFN8TiAq7B XVMGHLVWiriFSyX/ZMmz02m/W68ZEf1Q/NaEaAixndIW8aBQ1NHAVTeF4Zb6f5IyHjeJWX/ZY2m yZj9ujq//d7EVJVa5bBLULQ== X-Received: by 2002:a67:ff8d:0:b0:3d3:ca1a:8ffb with SMTP id v13-20020a67ff8d000000b003d3ca1a8ffbmr4638079vsq.35.1674065982404; Wed, 18 Jan 2023 10:19:42 -0800 (PST) X-Google-Smtp-Source: AMrXdXug6CkyCM95I/huvEU7kFyZ6EjdVoXX5CE+vESJWDMoO5WN0No4zIO4ZULcdaLksSEfsDj+7A== X-Received: by 2002:a67:ff8d:0:b0:3d3:ca1a:8ffb with SMTP id v13-20020a67ff8d000000b003d3ca1a8ffbmr4638059vsq.35.1674065982137; Wed, 18 Jan 2023 10:19:42 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id c5-20020a05620a268500b006e8f8ca8287sm18043337qkp.120.2023.01.18.10.19.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 18 Jan 2023 10:19:41 -0800 (PST) From: Andrew Burgess To: Alexandra Petlanova Hajkova Cc: =?us-ascii?Q?AlexandraH=3D=3FUTF-8=3FB=3Fw6E=3D=3F=3Djkov=3D=3FUTF-?= =?us-ascii?Q?8?=@sourceware.org, =?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: References: <20230113115910.3215524-1-ahajkova@redhat.com> <87zgahdsyi.fsf@redhat.com> Date: Wed, 18 Jan 2023 18:19:40 +0000 Message-ID: <87r0vremdf.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=-6.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,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: Alexandra Petlanova Hajkova writes: >> >> 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. >> > I haven't added one because I wasn't sure this patch would be accepted. > I'll add one if we agree this work makes sense. > >> >> 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 think I misinterpreted this part. My understanding was the remote > gdbserver (or the stub) will change the directory and then it's weird > to not be able to send back errors. Your interpretation makes perfect > sense. > > In this case, if this behaviour is the preferred way to handle the > QSetWOrkingDir, > I propose to change the documentation to make it better understandable. > > >> 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. >> >> What is your preference and does anybody else have an opinion on > QSetWorkingDir > (and similar packets) behaviour? > > Another confusing thing in remote protocol documentation ( > https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#Gene= ral-Query-Packets) > is the error handling. Various packets can reply with an error: > > =E2=80=98E NN=E2=80=99 Indeed, and laying this out at 'E NN' seems really bad to me as there's no space in the packet. Though looking through the docs we seem to do this everywhere, so I guess anyone working on this will figure that bit our pretty quick. > > Indicate a badly formed request. The error number NN is given as hex > digits. > But packet_check_result in remote.c just checks if it's an error and does > not handle the 'NN'. And I haven't found the meaning of 'NN'. Just throwing out an idea here, maybe packet_check_result should return an object, not an enum. Maybe a packet_result. Properties of this object would be: struct packet_result { /* Return the type of packet we encountered. */ enum packet_result status () const; /* Return a string representing the error, only valid if status() is PACKET_ERROR. */ std::string error_message () const; }; Then packet_check_result could parse the packet just like it currently does. If it sees 'Enn' then the error message becomes 'Unknown error: nn', if it sees 'E.msg' then the error message becomes the 'msg' part of the packet. We could even imagine a future where callers of packet_check_result would pass in a error code translation table that maps values of 'nn' to defined error messages ... maybe? I feel this is drifting away from your original patch, but is an interesting idea of better error reporting. Thanks, Andrew > > Thank you very much for the in-depth review.