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 2D6493858438 for ; Mon, 29 Jan 2024 10:01:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2D6493858438 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 2D6493858438 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=1706522501; cv=none; b=gczSIquJse4Y5hX6VbdVZCzjVP7O27+PShc3aCn2unJDuagIMOMsqM5/N+VBBCjLpm7LCVDS6p6y4a1efXhdFRIoxvj+YZxEiKVX/pg5DdtzOlBl2ZznJcHzQFj1lnSMyx7XnB8pfTX90HkBSuHPZ5161yZevXNvJGRyzfc3dDA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706522501; c=relaxed/simple; bh=KqklKb6yqp77MCSQ6Y/6zOdO8gv47xSiR2cBZYEK9Us=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=pQvzAj+xHrzkbxVocPM8zUtbmR0pk23koeRZlDeefm1+oPX8nl2wUWrgJIEYAupmInngxrblUXytwo0Hs6EyJxgnlC+6Nu/QCSdGjShTxubp/7A2rbA7eDxaRhPisbZuH4N602czXHqftSr0KqCaiBvjNquGzmrZfRT2rLQEBvY= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1706522494; 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=eg+sovbXfWDR1ujBMs699Ulxh6MzyW0KiXWU7vCeTP4=; b=P87UHUzwgNtYNFlA49ympTye4H49Oyo3s4yogKnkuZVGWo5c7yVlPPUujkFYl6LqcNw3iB PnxMbbGnJIMAzQ7WdbMYXXW8r5tTgQ5WJ2Lf7+b+dykAHLP7yvY4/5+TnoqiqdAGRhx+Ow hXnK3uc9fpb6iV8Ci8XX7AAXzYBceDc= Received: from mail-io1-f69.google.com (mail-io1-f69.google.com [209.85.166.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-85-EHi9ojR8NAGnzB66GUcEcw-1; Mon, 29 Jan 2024 05:01:33 -0500 X-MC-Unique: EHi9ojR8NAGnzB66GUcEcw-1 Received: by mail-io1-f69.google.com with SMTP id ca18e2360f4ac-7bef67c486aso170396739f.2 for ; Mon, 29 Jan 2024 02:01:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706522492; x=1707127292; 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=NC0DpMCcEh1gotMBq/RYlmN02xvBYsBwyVzIxygwJfo=; b=OWzaowdQxs2XA+L88ZepuI0EoSRf3nIIKVEa8qQPBJoz3brwGmI2jJvCzfs0fgfcwH 3nFyo9vUFPhShfe8p8wpPUM25PsVUihhsjQfR7fj2py8082tl+7fK88XS3v3zoLOGXuA wdeaLp0M/PKeqBr1DV3dB8ofzembfJiT1mjcniHI0fLDDlIj3qGcQoAqyKyFcyn+1Aqn Ro/e0q04TXV3ADd2QXLfjifeOLHHrh9ogSBhHhiVUDxp8kFZ9zilydhxGrIEjfMPEtQK Tvedhv69NECMlV/NDVumAhwsbRzzw49VNuVrIzUZiwqywGzEih3RNUKPe7l7FedIDOBF Lskw== X-Gm-Message-State: AOJu0YzdzolYod4AYduHey68vBmc5M1L7icDMkiPgSkH2Xf2EcTDORPc WifXGJF42iqH75XJw0AcTnSoYTgrLGmFAPjQ/NOLny4gtIML6c5cucrM+QHQI+9leYAhdZwupT5 PZpZNWyjxm3AY16lJ6S8njskdRBycdukNfuvzL9n740tRskP3eP6qxPBzyG8CgbpDbVE= X-Received: by 2002:a5e:920b:0:b0:7bf:fd6a:dba4 with SMTP id y11-20020a5e920b000000b007bffd6adba4mr658941iop.5.1706522491898; Mon, 29 Jan 2024 02:01:31 -0800 (PST) X-Google-Smtp-Source: AGHT+IEJYtFYtC51nMMN9gZCwjxf0UBh2uXPt9sC/OEWDCxm7QzNZJ/V5OwS7nAjFTE5kZgPErlY1Q== X-Received: by 2002:a5e:920b:0:b0:7bf:fd6a:dba4 with SMTP id y11-20020a5e920b000000b007bffd6adba4mr658920iop.5.1706522491497; Mon, 29 Jan 2024 02:01:31 -0800 (PST) Received: from localhost (185.223.159.143.dyn.plus.net. [143.159.223.185]) by smtp.gmail.com with ESMTPSA id dq14-20020a0566384d0e00b0046e3432a5b0sm1669280jab.177.2024.01.29.02.01.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 Jan 2024 02:01:31 -0800 (PST) From: Andrew Burgess To: Alexandra =?utf-8?B?SMOhamtvdsOh?= , gdb-patches@sourceware.org Subject: Re: [PATCH v4] remote.c: Make packet_check_result return a structure In-Reply-To: <20240122164031.207232-1-ahajkova@redhat.com> References: <20240122164031.207232-1-ahajkova@redhat.com> Date: Mon, 29 Jan 2024 10:01:28 +0000 Message-ID: <871qa0zcef.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.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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 H=C3=A1jkov=C3=A1 writes: > packet_check_result currently returns an packet_result enum. > If GDB will recieve an error in a format E.errtext, which > is possible for some q packets, such errtext is lost if > treated by packet_check_result. > Introduce a new structure which bundles enum packet_result > with possible error message or error code returned by > the GDBserver. > I plan to make more GDBserver response checking functions to use > packet_check_result to make remote.c code more consistent. > This will also allow to use E.errtext more in the future. > > Beside adding the unit test, I tested this by modifying > store_registers_using_G function to get an error message: > > [remote] Sending packet: $GG00000000 ... > > gdbserver: Wrong sized register packet (expected 1792 bytes, got 1793) > gdbserver: Invalid hex digit 71 > Killing process(es): 1104002 > [remote] Packet received: E01 > Could not write registers; remote failure reply '01' > > Reviewed-by: Thiago Jung Bauermann > --- > v4: - cosmetics > - simplify the code a bit > - include a test checking packet_status::err_msg > to the unit test > > gdb/remote.c | 134 +++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 98 insertions(+), 36 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index 72f14e28f54..f33d1ba0fa4 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -18,7 +18,6 @@ > along with this program. If not, see .= */ > =20 > /* See the GDB User Guide for details of the GDB remote protocol. */ > - Spurious line deletion here. > #include "defs.h" > #include > #include > @@ -149,13 +148,46 @@ get_target_type_name (bool target_connected) > /* Analyze a packet's return value and update the packet config > accordingly. */ > =20 > -enum packet_result > +enum packet_status > { > PACKET_ERROR, > PACKET_OK, > PACKET_UNKNOWN > }; > =20 > +/* Keeps packet's return value. If packet's return value is PACKET_ERROR= , > + err_msg contains an error message string from E.string or the number > + stored as a string from E.num. */ > +struct packet_result > +{ > + packet_result (enum packet_status status, std::string err_msg) > + : m_status (status), m_err_msg (std::move (err_msg)) > + { > + gdb_assert (status =3D=3D PACKET_ERROR); > + } > + > + explicit packet_result (enum packet_status status) > + : m_status (status) > + { > + gdb_assert (status !=3D PACKET_ERROR); > + } > + > + enum packet_status status () const > + { > + return this->m_status; > + } > + > + const char *err_msg () const > + { > + gdb_assert (this->m_status =3D=3D PACKET_ERROR); > + return this->m_err_msg.c_str (); > + } > + > +private: > + enum packet_status m_status; > + std::string m_err_msg; > +}; > + > /* Enumeration of packets for a remote target. */ > =20 > enum { > @@ -732,8 +764,8 @@ struct remote_features > =20 > /* Check result value in BUF for packet WHICH_PACKET and update the pack= et's > support configuration accordingly. */ > - packet_result packet_ok (const char *buf, const int which_packet); > - packet_result packet_ok (const gdb::char_vector &buf, const int which_= packet); > + packet_status packet_ok (const char *buf, const int which_packet); > + packet_status packet_ok (const gdb::char_vector &buf, const int which_= packet); > =20 > /* Configuration of a remote target's memory read packet. */ > memory_packet_config m_memory_read_packet_config; > @@ -1254,7 +1286,7 @@ class remote_target : public process_stratum_target > =09=09=09=09=09int unit_size, > =09=09=09=09=09ULONGEST *xfered_len); > =20 > - packet_result remote_send_printf (const char *format, ...) > + packet_status remote_send_printf (const char *format, ...) > ATTRIBUTE_PRINTF (2, 3); > =20 > target_xfer_status remote_flash_write (ULONGEST address, > @@ -2418,7 +2450,10 @@ add_packet_config_cmd (const unsigned int which_pa= cket, const char *name, > } > } > =20 > -static enum packet_result > +/* Check GDBserver's reply packet. Return packet_result > + structure which contains the packet_status enum > + and an error message for the PACKET_ERROR case. */ > +static packet_result > packet_check_result (const char *buf) > { > if (buf[0] !=3D '\0') > @@ -2429,41 +2464,41 @@ packet_check_result (const char *buf) > =09 && isxdigit (buf[1]) && isxdigit (buf[2]) > =09 && buf[3] =3D=3D '\0') > =09/* "Enn" - definitely an error. */ > -=09return PACKET_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 '.') > -=09return PACKET_ERROR; > +=09return { PACKET_ERROR, buf + 2 }; > =20 > /* The packet may or may not be OK. Just assume it is. */ > - return PACKET_OK; > + return packet_result ( PACKET_OK ); Unnecessary whitepsace around PACKET_OK, should be: return packet_result (PACKET_OK); > } > else > + { > /* The stub does not support the packet. */ > - return PACKET_UNKNOWN; > + return packet_result ( PACKET_UNKNOWN ); Same here, but PACKET_UNKNOWN. > + } > } > =20 > -static enum packet_result > +static packet_result > packet_check_result (const gdb::char_vector &buf) > { > return packet_check_result (buf.data ()); > } > =20 > -packet_result > +packet_status > remote_features::packet_ok (const char *buf, const int which_packet) > { > packet_config *config =3D &m_protocol_packets[which_packet]; > packet_description *descr =3D &packets_descriptions[which_packet]; > =20 > - enum packet_result result; > - > if (config->detect !=3D AUTO_BOOLEAN_TRUE > && config->support =3D=3D PACKET_DISABLE) > internal_error (_("packet_ok: attempt to use a disabled packet")); > =20 > - result =3D packet_check_result (buf); > - switch (result) > + packet_result result =3D packet_check_result (buf); > + switch (result.status ()) > { > case PACKET_OK: > case PACKET_ERROR: > @@ -2498,10 +2533,10 @@ remote_features::packet_ok (const char *buf, cons= t int which_packet) > break; > } > =20 > - return result; > + return result.status (); > } > =20 > -packet_result > +packet_status > remote_features::packet_ok (const gdb::char_vector &buf, const int which= _packet) > { > return packet_ok (buf.data (), which_packet); > @@ -3000,7 +3035,7 @@ remote_target::set_syscall_catchpoint (int pid, boo= l needed, int any_count, > =09=09=09=09 gdb::array_view syscall_counts) > { > const char *catch_packet; > - enum packet_result result; > + enum packet_status result; > int n_sysno =3D 0; > =20 > if (m_features.packet_support (PACKET_QCatchSyscalls) =3D=3D PACKET_DI= SABLE) > @@ -8791,9 +8826,10 @@ remote_target::send_g_packet () > xsnprintf (rs->buf.data (), get_remote_packet_size (), "g"); > putpkt (rs->buf); > getpkt (&rs->buf); > - if (packet_check_result (rs->buf) =3D=3D PACKET_ERROR) > + packet_result result =3D packet_check_result (rs->buf); > + if (result.status () =3D=3D PACKET_ERROR) > error (_("Could not read registers; remote failure reply '%s'"), > -=09 rs->buf.data ()); > +=09 result.err_msg ()); > =20 > /* We can get out of synch in various cases. If the first character > in the buffer is not a hex character, assume that has happened > @@ -9099,9 +9135,10 @@ remote_target::store_registers_using_G (const stru= ct regcache *regcache) > bin2hex (regs, p, rsa->sizeof_g_packet); > putpkt (rs->buf); > getpkt (&rs->buf); > - if (packet_check_result (rs->buf) =3D=3D PACKET_ERROR) > + packet_result pkt_status =3D packet_check_result (rs->buf); > + if (pkt_status.status () =3D=3D PACKET_ERROR) > error (_("Could not write registers; remote failure reply '%s'"), > -=09 rs->buf.data ()); > +=09 pkt_status.err_msg ()); > } > =20 > /* Store register REGNUM, or all registers if REGNUM =3D=3D -1, from the= contents > @@ -9683,7 +9720,7 @@ remote_target::remote_read_bytes (CORE_ADDR memaddr= , > FORMAT and the remaining arguments, then gets the reply. Returns > whether the packet was a success, a failure, or unknown. */ > =20 > -packet_result > +packet_status > remote_target::remote_send_printf (const char *format, ...) > { > struct remote_state *rs =3D get_remote_state (); > @@ -9706,7 +9743,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); > + return packet_check_result (rs->buf).status (); > } > =20 > /* Flash writing can take quite some time. We'll set > @@ -9718,7 +9755,7 @@ void > remote_target::flash_erase (ULONGEST address, LONGEST length) > { > int addr_size =3D gdbarch_addr_bit (current_inferior ()->arch ()) / 8; > - enum packet_result ret; > + enum packet_status ret; > scoped_restore restore_timeout > =3D make_scoped_restore (&remote_timeout, remote_flash_timeout); > =20 > @@ -11308,7 +11345,7 @@ remote_target::verify_memory (const gdb_byte *dat= a, CORE_ADDR lma, ULONGEST size > if (target_has_execution () > && m_features.packet_support (PACKET_qCRC) !=3D PACKET_DISABLE) > { > - enum packet_result result; > + enum packet_status status; > =20 > /* Make sure the remote is pointing at the right process. */ > set_general_process (); > @@ -11324,10 +11361,10 @@ remote_target::verify_memory (const gdb_byte *d= ata, CORE_ADDR lma, ULONGEST size > =20 > getpkt (&rs->buf); > =20 > - result =3D m_features.packet_ok (rs->buf, PACKET_qCRC); > - if (result =3D=3D PACKET_ERROR) > + status =3D m_features.packet_ok (rs->buf, PACKET_qCRC); > + if (status =3D=3D PACKET_ERROR) > =09return -1; > - else if (result =3D=3D PACKET_OK) > + else if (status =3D=3D PACKET_OK) > =09{ > =09 for (target_crc =3D 0, tmp =3D &rs->buf[1]; *tmp; tmp++) > =09 target_crc =3D target_crc * 16 + fromhex (*tmp); > @@ -12210,7 +12247,7 @@ remote_target::get_thread_local_address (ptid_t p= tid, CORE_ADDR lm, > struct remote_state *rs =3D get_remote_state (); > char *p =3D rs->buf.data (); > char *endp =3D p + get_remote_packet_size (); > - enum packet_result result; > + enum packet_status result; > =20 > strcpy (p, "qGetTLSAddr:"); > p +=3D strlen (p); > @@ -12256,7 +12293,7 @@ remote_target::get_tib_address (ptid_t ptid, CORE= _ADDR *addr) > struct remote_state *rs =3D get_remote_state (); > char *p =3D rs->buf.data (); > char *endp =3D p + get_remote_packet_size (); > - enum packet_result result; > + enum packet_status result; > =20 > strcpy (p, "qGetTIBAddr:"); > p +=3D strlen (p); > @@ -13821,7 +13858,7 @@ remote_target::get_trace_status (struct trace_sta= tus *ts) > { > /* Initialize it just to avoid a GCC false warning. */ > char *p =3D NULL; > - enum packet_result result; > + enum packet_status result; > struct remote_state *rs =3D get_remote_state (); > =20 > if (m_features.packet_support (PACKET_qTStatus) =3D=3D PACKET_DISABLE) > @@ -14201,7 +14238,7 @@ remote_target::set_trace_buffer_size (LONGEST val= ) > struct remote_state *rs =3D get_remote_state (); > char *buf =3D rs->buf.data (); > char *endbuf =3D buf + get_remote_packet_size (); > - enum packet_result result; > + enum packet_status result; > =20 > gdb_assert (val >=3D 0 || val =3D=3D -1); > buf +=3D xsnprintf (buf, endbuf - buf, "QTBuffer:size:"); > @@ -15529,7 +15566,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.data ()) =3D=3D PACKET_OK; > + return packet_check_result (rs->buf).status () =3D=3D PACKET_OK; > } > =20 > /* Return true if remote target T is non-stop. */ > @@ -15626,8 +15663,31 @@ test_memory_tagging_functions () > =09=09 expected.length ()) =3D=3D 0); > } > =20 > +static void > +test_packet_check_result () > +{ > + std::string buf =3D "E.msg"; > + packet_result result =3D packet_check_result (buf.data ()); > + > + SELF_CHECK (result.status () =3D=3D PACKET_ERROR); > + SELF_CHECK (strcmp(result.err_msg (), "msg") =3D=3D 0); > + > + SELF_CHECK (packet_check_result ("E01").status () =3D=3D PACKET_ERROR)= ; I think you should rewrite this check in the same way that you check 'E.msg', this will allow you to check both the status and the err_msg. > + > + SELF_CHECK (packet_check_result ("E1").status () =3D=3D PACKET_OK); > + > + SELF_CHECK (packet_check_result ("E000").status () =3D=3D PACKET_OK); > + > + SELF_CHECK (packet_check_result ("E.").status () =3D=3D PACKET_ERROR); Again, you should check the err_msg content here, though I think this will highlight an issue: should we ever get this back from the remote then the err_msg text will be empty, which isn't going to look good in the output. I think the choices here are (a) define that 'E.' is NOT an error packet (same as E1 or E000 is not an error packet), in which case 'E.' should return PACKET_OK, or (b) provide some default text, e.g. 'no error provided'. Given how we treat E1 and E000, I'd be tempted to go with option (a) myself. > + > + SELF_CHECK (packet_check_result ("some response").status () =3D=3D PAC= KET_OK); > + > + SELF_CHECK (packet_check_result ("").status () =3D=3D PACKET_UNKNOWN); > + > +} > + > } // namespace selftests > -#endif /* GDB_SELF_TEST */ > +#endif /* GDB_SELTEST */ Spurious change here. Thanks, Andrew > =20 > void _initialize_remote (); > void > @@ -16125,5 +16185,7 @@ from the target."), > #if GDB_SELF_TEST > selftests::register_test ("remote_memory_tagging", > =09=09=09 selftests::test_memory_tagging_functions); > + selftests::register_test ("packet_check_result", > +=09=09=09 selftests::test_packet_check_result); > #endif > } > --=20 > 2.43.0