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 A65893858D39 for ; Thu, 22 Feb 2024 14:37:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A65893858D39 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 A65893858D39 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=1708612659; cv=none; b=li/lUoE/P30+udjJkQTk4uK4M8Vly7tydv1s2mlJxtskq2djq90BLlivDbT/AAYv/3Yrzn8M0Uo8saimn76V5ZQtpQzCoFyUu+PyZDsmuLFlnhZm7k+8Tu/if9dAh/mY3ZOV3f523GLYzFKRFdktcjeEmcqtf10ZA94jad6kxxE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708612659; c=relaxed/simple; bh=WPh5Nkfy6jEFpJCt+2/mY8Adhdmk8yHTv2kGqq31TZs=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=NWRE5GluiRSqP6mIbrFXQFPw1ZLNDVLhXodPxzpqx/PRG3wHMWL7nRqspGI7S+UmExbc9XoeN92LIXKuEIyG9l+fJnX75ITylvYNKtxUrNwsn0nrnTCo6AWqs2E64TUEPNZGYXEa0iI+kK3CNjRjngBA/vKveiHggUJfvmfk04U= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1708612655; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Z4p3uGIQf4Ovq9aF/ahes7tDNw/4Mo+YA80VVrvsbDQ=; b=D6Op8ZmlGLuyQ1JU3VtpOrisH3B6MK7Tvvd+eGo9TbTe3KT/3tJ+PiSodCTEGePVg0tShs mTMMBcO4HATkEDFYrUamrEXZgTdWxPsZMFLpU1rIIcfva3QFdV5HzOupmdUrAT5ugMzSSa AbsuFi3OxjdB2zB2U4Zxi4GIAa0gfX8= Received: from mail-pj1-f69.google.com (mail-pj1-f69.google.com [209.85.216.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-641-zALBCPs-M8-ey3GIUK4GJQ-1; Thu, 22 Feb 2024 09:37:33 -0500 X-MC-Unique: zALBCPs-M8-ey3GIUK4GJQ-1 Received: by mail-pj1-f69.google.com with SMTP id 98e67ed59e1d1-29a5bae5b3fso265304a91.2 for ; Thu, 22 Feb 2024 06:37:33 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708612652; x=1709217452; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Z4p3uGIQf4Ovq9aF/ahes7tDNw/4Mo+YA80VVrvsbDQ=; b=kxL2ggWKnCGVDu64KN6uV9K4K+yeJwRh4TUIllLaADOh5vcwoWRuYvgcr0eubCCTa5 qa/vq3M4umqnMsxFi3icWBKJ7OpL1nfEYhEexkUomCTwi7/g5SOosxy0KNQj6hBsjMEm fr3KY4FASw//sQP+i5eWDaSri8/afcACdlQU0ZDRnB8sa8CxPvskd98as/X/R4RlwasO h7RCR/XF/YEALBL0/aQxgRBT3yWLvA8UK2LGwTCyATH7H8Tuagheu+/NF87letyGRHEO d7AMzFer6l97pBvQaeTBcIIOHze1SesZU5SXf2nIL6DvD/Opxq2bxnpoI7VrtLJL8tGh St1A== X-Gm-Message-State: AOJu0YwKqLSpmeaEybbh4fmKG30+Jhybh0uac99psrbJOwBbfqEUgQu5 uRI6PqMpDgsuXOzvHe06mx1gbOiTw0usoc9AK/8KSsgB9JgRawhMXBV6MQr1m+kjFfPJs8dBvvs spvBHZ3kVt0Z0KXarS6Fuk/7Lo7MW8Z4JSEmTN/VI1vOlyxXmcCXAlEA2iT8TqJjhb/Ckhc3IjJ qCqQaQgrn6rrbYufz6Sb/BIZxiVNKYHvsx3LrnLKxOvg== X-Received: by 2002:a17:90a:7403:b0:299:699b:c5de with SMTP id a3-20020a17090a740300b00299699bc5demr11769931pjg.19.1708612652191; Thu, 22 Feb 2024 06:37:32 -0800 (PST) X-Google-Smtp-Source: AGHT+IGar/vVxiHeWBd4Zw62Nz0kkWwXA2WpviyDKnW/2gr2Ppvxr0+Rt541nFC35LUDjWi0G9jB4D4epBJYylBpB9U= X-Received: by 2002:a17:90a:7403:b0:299:699b:c5de with SMTP id a3-20020a17090a740300b00299699bc5demr11769914pjg.19.1708612651810; Thu, 22 Feb 2024 06:37:31 -0800 (PST) MIME-Version: 1.0 References: <20240119115659.491195-1-ahajkova@redhat.com> <20240119115659.491195-3-ahajkova@redhat.com> In-Reply-To: <20240119115659.491195-3-ahajkova@redhat.com> From: Alexandra Petlanova Hajkova Date: Thu, 22 Feb 2024 15:37:20 +0100 Message-ID: Subject: Re: [PATCH v2] remote.c: Make packet_check_result return a structure To: gdb-patches@sourceware.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="0000000000002b95720611f96279" X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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: --0000000000002b95720611f96279 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable ping (this still applies cleanly) On Fri, Jan 19, 2024 at 12:57=E2=80=AFPM Alexandra H=C3=A1jkov=C3=A1 wrote: > 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. > > There's no infrastructure to test this with a test case so > 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 > --- > v2: > - Added a unit test for packet_check_result. > - Improved formatting and reorganised packet_result structure. > - Renamed pkt_status to pkt_result and result to status varariablese > where suitable. > > gdb/remote.c | 147 +++++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 113 insertions(+), 34 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index dcc1a0d0639..8e51e588bce 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -149,13 +149,46 @@ get_target_type_name (bool target_connected) > /* Analyze a packet's return value and update the packet config > accordingly. */ > > -enum packet_result > +enum packet_status > { > PACKET_ERROR, > PACKET_OK, > PACKET_UNKNOWN > }; > > +/* 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. */ > > enum { > @@ -732,8 +765,8 @@ struct remote_features > > /* Check result value in BUF for packet WHICH_PACKET and update the > packet'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); > > /* Configuration of a remote target's memory read packet. */ > memory_packet_config m_memory_read_packet_config; > @@ -1254,7 +1287,7 @@ class remote_target : public process_stratum_target > int unit_size, > ULONGEST *xfered_len); > > - packet_result remote_send_printf (const char *format, ...) > + packet_status remote_send_printf (const char *format, ...) > ATTRIBUTE_PRINTF (2, 3); > > target_xfer_status remote_flash_write (ULONGEST address, > @@ -2418,7 +2451,10 @@ add_packet_config_cmd (const unsigned int > which_packet, const char *name, > } > } > > -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') > @@ -2428,42 +2464,46 @@ packet_check_result (const char *buf) > if (buf[0] =3D=3D 'E' > && isxdigit (buf[1]) && isxdigit (buf[2]) > && buf[3] =3D=3D '\0') > + { > /* "Enn" - definitely an error. */ > - return PACKET_ERROR; > + return { PACKET_ERROR, buf + 1 }; > + } > > /* Always treat "E." as an error. This will be used for > more verbose error messages, such as E.memtypes. */ > if (buf[0] =3D=3D 'E' && buf[1] =3D=3D '.') > - return PACKET_ERROR; > + { > + return { PACKET_ERROR, buf + 2 }; > + } > > /* The packet may or may not be OK. Just assume it is. */ > - return PACKET_OK; > + return packet_result ( PACKET_OK ); > } > else > + { > /* The stub does not support the packet. */ > - return PACKET_UNKNOWN; > + return packet_result ( PACKET_UNKNOWN ); > + } > } > > -static enum packet_result > +static packet_result > packet_check_result (const gdb::char_vector &buf) > { > return packet_check_result (buf.data ()); > } > > -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]; > > - 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")); > > - 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 +2538,10 @@ remote_features::packet_ok (const char *buf, const > int which_packet) > break; > } > > - return result; > + return result.status (); > } > > -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 +3040,7 @@ remote_target::set_syscall_catchpoint (int pid, bool > needed, int any_count, > gdb::array_view > syscall_counts) > { > const char *catch_packet; > - enum packet_result result; > + enum packet_status result; > int n_sysno =3D 0; > > if (m_features.packet_support (PACKET_QCatchSyscalls) =3D=3D PACKET_DI= SABLE) > @@ -8791,9 +8831,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'"), > - rs->buf.data ()); > + result.err_msg ()); > > /* 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 +9140,10 @@ remote_target::store_registers_using_G (const > struct 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'"), > - rs->buf.data ()); > + pkt_status.err_msg ()); > } > > /* Store register REGNUM, or all registers if REGNUM =3D=3D -1, from the > contents > @@ -9683,7 +9725,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. */ > > -packet_result > +packet_status > remote_target::remote_send_printf (const char *format, ...) > { > struct remote_state *rs =3D get_remote_state (); > @@ -9705,8 +9747,9 @@ remote_target::remote_send_printf (const char > *format, ...) > > rs->buf[0] =3D '\0'; > getpkt (&rs->buf); > + packet_result pkt_status =3D packet_check_result (rs->buf); > > - return packet_check_result (rs->buf); > + return pkt_status.status (); > } > > /* Flash writing can take quite some time. We'll set > @@ -9718,7 +9761,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); > > @@ -11308,7 +11351,7 @@ remote_target::verify_memory (const gdb_byte > *data, 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; > > /* Make sure the remote is pointing at the right process. */ > set_general_process (); > @@ -11324,10 +11367,10 @@ remote_target::verify_memory (const gdb_byte > *data, CORE_ADDR lma, ULONGEST size > > getpkt (&rs->buf); > > - 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) > return -1; > - else if (result =3D=3D PACKET_OK) > + else if (status =3D=3D PACKET_OK) > { > for (target_crc =3D 0, tmp =3D &rs->buf[1]; *tmp; tmp++) > target_crc =3D target_crc * 16 + fromhex (*tmp); > @@ -12210,7 +12253,7 @@ remote_target::get_thread_local_address (ptid_t > ptid, 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; > > strcpy (p, "qGetTLSAddr:"); > p +=3D strlen (p); > @@ -12256,7 +12299,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; > > strcpy (p, "qGetTIBAddr:"); > p +=3D strlen (p); > @@ -13821,7 +13864,7 @@ remote_target::get_trace_status (struct > trace_status *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 (); > > if (m_features.packet_support (PACKET_qTStatus) =3D=3D PACKET_DISABLE) > @@ -14201,7 +14244,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; > > gdb_assert (val >=3D 0 || val =3D=3D -1); > buf +=3D xsnprintf (buf, endbuf - buf, "QTBuffer:size:"); > @@ -15527,9 +15570,10 @@ remote_target::store_memtags (CORE_ADDR address, > size_t len, > > putpkt (rs->buf); > getpkt (&rs->buf); > + packet_result pkt_status =3D packet_check_result (rs->buf); > > /* Verify if the request was successful. */ > - return packet_check_result (rs->buf.data ()) =3D=3D PACKET_OK; > + return pkt_status.status () =3D=3D PACKET_OK; > } > > /* Return true if remote target T is non-stop. */ > @@ -15626,6 +15670,39 @@ test_memory_tagging_functions () > expected.length ()) =3D=3D 0); > } > > +static void > +test_packet_check_result () > +{ > + std::string buf =3D "E01"; > + > + packet_result result =3D packet_check_result (buf.data ()); > + SELF_CHECK (result.status () =3D=3D PACKET_ERROR); > + > + buf.assign("E1"); > + result =3D packet_check_result (buf.data ()); > + SELF_CHECK (result.status () =3D=3D PACKET_OK); > + > + buf.assign("E000"); > + result =3D packet_check_result (buf.data ()); > + SELF_CHECK (result.status () =3D=3D PACKET_OK); > + > + buf.assign("E.msg"); > + result =3D packet_check_result (buf.data ()); > + SELF_CHECK (result.status () =3D=3D PACKET_ERROR); > + > + buf.assign("E."); > + result =3D packet_check_result (buf.data ()); > + SELF_CHECK (result.status () =3D=3D PACKET_ERROR); > + > + buf.assign("some response"); > + result =3D packet_check_result (buf.data ()); > + SELF_CHECK (result.status () =3D=3D PACKET_OK); > + > + buf.assign(""); > + result =3D packet_check_result (buf.data ()); > + SELF_CHECK (result.status () =3D=3D PACKET_UNKNOWN); > +} > + > } // namespace selftests > #endif /* GDB_SELF_TEST */ > > @@ -16125,5 +16202,7 @@ from the target."), > #if GDB_SELF_TEST > selftests::register_test ("remote_memory_tagging", > selftests::test_memory_tagging_functions); > + selftests::register_test ("packet_check_result", > + selftests::test_packet_check_result); > #endif > } > -- > 2.43.0 > > --0000000000002b95720611f96279--