From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id RjSaAU5QAWaTRxUAWB0awg (envelope-from ) for ; Mon, 25 Mar 2024 06:22:06 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=FMGIMorf; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id E89A41E0C0; Mon, 25 Mar 2024 06:22:05 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id B23001E030 for ; Mon, 25 Mar 2024 06:22:03 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 264283858412 for ; Mon, 25 Mar 2024 10:22:03 +0000 (GMT) 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 B3295385840A for ; Mon, 25 Mar 2024 10:21:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B3295385840A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B3295385840A Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711362103; cv=none; b=IQXS2cRRsr/89+oxMG8ZqKt1lP+HADif3DW60HAVZt01j1fLffCx2XhZ2KBNxiPKjycorqL8BrwthWY2MPsS8jJkEqk5BesH6lLT8nN65tLFjVFCpHpDvZjJiT6qsfRx1lPq7qhZdLJ7SABxK60Frs9XSWMEAyy+0TpBiER3SpY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711362103; c=relaxed/simple; bh=myIWNLBihEdc/N9KWIbRu5vS71BwUx6+UOj4NwUNo1I=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=UvZdUnVomNfRP4h/GboStC66VvA0ioKbdZJRiAWPXeq2uFDrHM9wsSERouiHHpUDj707JNVES9Dbgc2REZDTJEAo/tT84WOJlX1aCaSGqBCYUlCOFy1Y/SmOQ2Lzgf+rZjJUTPFr3ZqNUSuxdq+BEBjBC5o4h3zrHjmbh/bkdpQ= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1711362101; 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=jWNHN5mjGLM8VN/djwXLxQ0pvjKa8SlTnAEfSWBgk4k=; b=FMGIMorf/oTuCw4uT/qLsrSpU9KaYO66dbFuTFirDeZ5EAqt+Pt2hpnrnjVTTcP+xuhR7x iY78zsRv+yTM8M96PkGi46oUEQ/5LKjAzvV2QIg8g8De8Re8f2tAVL6nHtQ0f5lUTiN1J7 bt6zFs4Fk9Pz8KKWFHvfZw+Gl448bp8= Received: from mail-ed1-f69.google.com (mail-ed1-f69.google.com [209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-249-SrikQFlNN5Cga6xhKroMmQ-1; Mon, 25 Mar 2024 06:21:40 -0400 X-MC-Unique: SrikQFlNN5Cga6xhKroMmQ-1 Received: by mail-ed1-f69.google.com with SMTP id 4fb4d7f45d1cf-56c0d3517deso606173a12.0 for ; Mon, 25 Mar 2024 03:21:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711362097; x=1711966897; 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=eVUUp9jlB1jFMEeQfs5BIdv2uRiTMrXUaj9W3P7y8ds=; b=RSywqZA/bRVqjZYt/+kv/6qZPLLInEMhD2Xmbi/xXROPYuvGg7ke4iITQjdN6xCI/5 kbUxTceO5Ey43gqGGlZEjNM67vZCQb/eD1x+zL+iMdRdYZP1m9hG3zJMJ4bYMuTLstiJ E4Uyto5BbbjKNmI+DyN6vdzj6AYOI5qZEojD5CmyUHU8wuPWV+QqIiu35lltowV1OY4w b7ATIeL59GA3yKcE76O1zv3OLV2pKg9H6VfcQX2S94o9ozXAQ6jZp+WPt/EcExJKBqH/ ss5bvalUfXAVbGpjynJpuQfj+gy+qJcSAb5uXQnI6TWOhZrNlbjfbC3ODYg5ay2SyNyM 9Drg== X-Forwarded-Encrypted: i=1; AJvYcCV5+63VpDunz9AdZoLU1GnlUM17sBwpHoWFAPz0Nesx+fHiWDaG4VOL1egeW1oi40yKR1/Q5gHYlagd6No0Ng/HkPeSQxygh/VPTA== X-Gm-Message-State: AOJu0YwdaCVoNiohj6+IW3IYmlr9UQ1a1BGKmUHgsgsQnuQkVoEMJUUw Wx+SM7tKwK+GeXK31u3AGCWJ0x5PyMMOsLOOxCFh1j84sFX66u1Ebc5/eMRhvOsiivbjz/7FHxd RLXKT1SUR5dzS8NLcMuCvULnXz9GsbLIabYhEEYhoBkBpWzNBiXuguEVq8VPQ8JLkRfo= X-Received: by 2002:a50:8d44:0:b0:568:ab85:d85 with SMTP id t4-20020a508d44000000b00568ab850d85mr4988840edt.5.1711362097500; Mon, 25 Mar 2024 03:21:37 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHntb6Sq9yrDQwd+YDWa21vxdLk3gpigSTOXHR0sdn38TdHJx1EMBDpolBoVEhSn+4dEqyzUw== X-Received: by 2002:a50:8d44:0:b0:568:ab85:d85 with SMTP id t4-20020a508d44000000b00568ab850d85mr4988813edt.5.1711362096871; Mon, 25 Mar 2024 03:21:36 -0700 (PDT) Received: from localhost (185.223.159.143.dyn.plus.net. [143.159.223.185]) by smtp.gmail.com with ESMTPSA id i10-20020a0564020f0a00b0056c09a0ed79sm1522729eda.11.2024.03.25.03.21.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Mar 2024 03:21:36 -0700 (PDT) From: Andrew Burgess To: Alexandra =?utf-8?B?SMOhamtvdsOh?= , gdb-patches@sourceware.org Cc: ahajkova@redhat.com Subject: Re: [PATCH v2 1/2] remote.c: Use packet_check_result In-Reply-To: <20240319135829.662941-1-ahajkova@khirnov.net> References: <20240319135829.662941-1-ahajkova@khirnov.net> Date: Mon, 25 Mar 2024 10:21:35 +0000 Message-ID: <878r26txw0.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=-12.0 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_H4, RCVD_IN_MSPIKE_WL, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Alexandra H=C3=A1jkov=C3=A1 writes: > From: Alexandra H=C3=A1jkov=C3=A1 > > when processing the GDBserver reply to qRcmd packet. > Print error message or the error code. > Currently, when qRcmd request returns an error, > GDB just prints: > > Protocol error with Rcmd > > After this change, GDB will also print the error code: > > Protocol error with Rcmd: 01. > > Add an accept_msg argument to packet_check result. qRcmd > request (such as many other packets) does not recognise > "E.msg" form as an error right now. We want to recognise > "E.msg" as an error response only for the packets where > it's documented. > --- > gdb/remote.c | 75 +++++++++++++++++++++++++++++----------------------- > 1 file changed, 42 insertions(+), 33 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index 14c8b020b1e..8462b7e4e60 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -2451,11 +2451,15 @@ add_packet_config_cmd (const unsigned int which_p= acket, const char *name, > } > } > =20 > -/* Check GDBserver's reply packet. Return packet_result > - structure which contains the packet_status enum > - and an error message for the PACKET_ERROR case. */ > +/* Check GDBserver's reply packet. Return packet_result structure > + which contains the packet_status enum and an error message for the > + PACKET_ERROR case. > + > + An error packet can always take the form Exx (where xx is a hex > + code). When ACCEPT_MSG is true error messages can also take the > + form E.msg (where msg is any arbitrary string). */ > static packet_result > -packet_check_result (const char *buf) > +packet_check_result (const char *buf, bool accept_msg) > { > if (buf[0] !=3D '\0') > { > @@ -2467,14 +2471,20 @@ packet_check_result (const char *buf) > =09/* "Enn" - definitely an error. */ > =09return { PACKET_ERROR, buf + 1 }; > =20 > - /* Always treat "E." as an error. This will be used for > -=09 more verbose error messages, such as E.memtypes. */ > - if (buf[0] =3D=3D 'E' && buf[1] =3D=3D '.') > + /* Not every request accepts an error in a E.msg form. > +=09 Some packets accepts only Enn, in this case E. is not > +=09 defined and E. is treated as PACKET_OK. */ > + if (accept_msg) > =09{ > -=09 if (buf[2] !=3D '\0') > -=09 return { PACKET_ERROR, buf + 2 }; > -=09 else > -=09 return { PACKET_ERROR, "no error provided" }; > +=09 /* Always treat "E." as an error. This will be used for > +=09 more verbose error messages, such as E.memtypes. */ > +=09 if (buf[0] =3D=3D 'E' && buf[1] =3D=3D '.') > +=09 { > +=09 if (buf[2] !=3D '\0') > +=09=09return { PACKET_ERROR, buf + 2 }; > +=09 else > +=09=09return { PACKET_ERROR, "no error provided" }; > +=09 } > =09} > =20 > /* The packet may or may not be OK. Just assume it is. */ > @@ -2488,9 +2498,9 @@ packet_check_result (const char *buf) > } > =20 > static packet_result > -packet_check_result (const gdb::char_vector &buf) > +packet_check_result (const gdb::char_vector &buf, bool accept_msg) > { > - return packet_check_result (buf.data ()); > + return packet_check_result (buf.data (), accept_msg); > } > =20 > packet_status > @@ -2503,7 +2513,7 @@ remote_features::packet_ok (const char *buf, const = int which_packet) > && config->support =3D=3D PACKET_DISABLE) > internal_error (_("packet_ok: attempt to use a disabled packet")); > =20 > - packet_result result =3D packet_check_result (buf); > + packet_result result =3D packet_check_result (buf, true); > switch (result.status ()) > { > case PACKET_OK: > @@ -8831,7 +8841,7 @@ remote_target::send_g_packet () > xsnprintf (rs->buf.data (), get_remote_packet_size (), "g"); > putpkt (rs->buf); > getpkt (&rs->buf); > - packet_result result =3D packet_check_result (rs->buf); > + packet_result result =3D packet_check_result (rs->buf, true); > if (result.status () =3D=3D PACKET_ERROR) > error (_("Could not read registers; remote failure reply '%s'"), > =09 result.err_msg ()); > @@ -9140,7 +9150,7 @@ remote_target::store_registers_using_G (const struc= t regcache *regcache) > bin2hex (regs, p, rsa->sizeof_g_packet); > putpkt (rs->buf); > getpkt (&rs->buf); > - packet_result pkt_status =3D packet_check_result (rs->buf); > + packet_result pkt_status =3D packet_check_result (rs->buf, true); > if (pkt_status.status () =3D=3D PACKET_ERROR) > error (_("Could not write registers; remote failure reply '%s'"), > =09 pkt_status.err_msg ()); > @@ -9748,7 +9758,7 @@ remote_target::remote_send_printf (const char *form= at, ...) > rs->buf[0] =3D '\0'; > getpkt (&rs->buf); > =20 > - return packet_check_result (rs->buf).status (); > + return packet_check_result (rs->buf, true).status (); > } > =20 > /* Flash writing can take quite some time. We'll set > @@ -11931,20 +11941,19 @@ remote_target::rcmd (const char *command, struc= t ui_file *outbuf) > =09 continue; > =09} > buf =3D rs->buf.data (); > - if (buf[0] =3D=3D '\0') > -=09error (_("Target does not support this command.")); > if (buf[0] =3D=3D 'O' && buf[1] !=3D 'K') > =09{ > =09 remote_console_output (buf + 1); /* 'O' message from stub. */ > =09 continue; > =09} > - if (strcmp (buf, "OK") =3D=3D 0) > + packet_result result =3D packet_check_result (buf, false); > + if (result.status () =3D=3D PACKET_OK) > =09break; Unfortunately this is a change in behaviour. PACKET_OK status does not mean that the packet contains the text "OK", it just means that the packet was not an error, and was not the empty (PACKET_UNKNOWN) packet. If the packet contained the text "ABCD" then previously we would end up in the loop below which converts the value from a hex-encoded string, but now I believe we'll end up breaking out of the while loop. > - if (strlen (buf) =3D=3D 3 && buf[0] =3D=3D 'E' > -=09 && isxdigit (buf[1]) && isxdigit (buf[2])) > -=09{ > -=09 error (_("Protocol error with Rcmd")); > -=09} > + else if (result.status () =3D=3D PACKET_UNKNOWN) > +=09error (_("Target does not support this command.")); > + else > +=09error (_("Protocol error with Rcmd: %s."), result.err_msg ()); > + > for (p =3D buf; p[0] !=3D '\0' && p[1] !=3D '\0'; p +=3D 2) > =09{ > =09 char c =3D (fromhex (p[0]) << 4) + fromhex (p[1]); > @@ -15571,7 +15580,7 @@ remote_target::store_memtags (CORE_ADDR address, = size_t len, > getpkt (&rs->buf); > =20 > /* Verify if the request was successful. */ > - return packet_check_result (rs->buf).status () =3D=3D PACKET_OK; > + return (packet_check_result (rs->buf, true).status () =3D=3D PACKET_OK= ); > } > =20 > /* Return true if remote target T is non-stop. */ > @@ -15672,26 +15681,26 @@ static void > test_packet_check_result () > { > std::string buf =3D "E.msg"; > - packet_result result =3D packet_check_result (buf.data ()); > + packet_result result =3D packet_check_result (buf.data (), true); > =20 > SELF_CHECK (result.status () =3D=3D PACKET_ERROR); > SELF_CHECK (strcmp(result.err_msg (), "msg") =3D=3D 0); > =20 > - result =3D packet_check_result ("E01"); > + result =3D packet_check_result ("E01", true); > SELF_CHECK (result.status () =3D=3D PACKET_ERROR); > SELF_CHECK (strcmp(result.err_msg (), "01") =3D=3D 0); > =20 > - SELF_CHECK (packet_check_result ("E1").status () =3D=3D PACKET_OK); > + SELF_CHECK (packet_check_result ("E1", true).status () =3D=3D PACKET_O= K); > =20 > - SELF_CHECK (packet_check_result ("E000").status () =3D=3D PACKET_OK); > + SELF_CHECK (packet_check_result ("E000", true).status () =3D=3D PACKET= _OK); > =20 > - result =3D packet_check_result ("E."); > + result =3D packet_check_result ("E.", true); > SELF_CHECK (result.status () =3D=3D PACKET_ERROR); > SELF_CHECK (strcmp(result.err_msg (), "no error provided") =3D=3D 0); > =20 > - SELF_CHECK (packet_check_result ("some response").status () =3D=3D PAC= KET_OK); > + SELF_CHECK (packet_check_result ("some response", true).status () =3D= =3D PACKET_OK); > =20 > - SELF_CHECK (packet_check_result ("").status () =3D=3D PACKET_UNKNOWN); > + SELF_CHECK (packet_check_result ("", true).status () =3D=3D PACKET_UNK= NOWN); I wonder if we should add a unit test here where we pass an 'E.msg' and pass the second argument to packet_check_result as false and ensure we get back the results we expect (which I think is PACKET_OK). Thanks, Andrew > } > } // namespace selftests > #endif /* GDB_SELF_TEST */ > --=20 > 2.44.0