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.133.124]) by sourceware.org (Postfix) with ESMTPS id E56DC3858C54 for ; Tue, 6 Feb 2024 16:06:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E56DC3858C54 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 E56DC3858C54 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707235575; cv=none; b=QJQGM1A1qrSmEGQ6cX+UGtKx97x8Fn7hsjKjWnXtWGSqmiJnbLy/tRxU6bdlNKU33F4G7g1LVd58ikw4mq9sgyevDbQNZiWsJZiXwGjpsuMNH5owDv3DqipenOV1m1Bu4Ct+2DGx/3SV7eWT6iPiK6EqUjMxqs03HPQlayP4+iE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707235575; c=relaxed/simple; bh=kLo0QZcG7n849fmlcDWNYhmm7RUANDkywEHTTISjIwc=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=ZBHqYzsmMlr+8iN/ViSSaJKrcxby/M9ZO6hiTUSRppdO5Kt2Cp5oXLNrj1uJ+63q829AAncqmNMPneDJtyTF8bTHw1Ij083GSoOjg9ZMNUf8hhGTtsig4M8Fn7dZ1xvN1nEeQnb0THX0WATVdJ7J5RLckoQWDv2x49/8e1tfEP4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1707235569; 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=aRcO5UJuSqEZleUa/X6795DuiPqN1cEwbFkIK4Tx8+0=; b=YUd/fY4CF3xAYn1bQIF5wtGWd826TIsuRnZq4cOI8fC46opk7nwzCRIkzVNoeq+//N6Q4L LMm810CL0kbD2PPCC+s9ZYjxP8xpK91DYmZsjAYcjBA37cEPwxuxYwjfU7ScO4IcTozqm1 2KRcCIZWSlNqiYhUYS2aM21Wj4itAdo= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-356-IEGi_skWOgeyIIQ_P97O7g-1; Tue, 06 Feb 2024 11:06:07 -0500 X-MC-Unique: IEGi_skWOgeyIIQ_P97O7g-1 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-33b12b75b70so2431001f8f.3 for ; Tue, 06 Feb 2024 08:06:07 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707235565; x=1707840365; 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=IkutKk8r7NJbZFJm0X6RGxcj43UJSvs7/ymdrSIoENo=; b=USXN3F0TfS4Qr18vPyxN75kezippcy3x798t3kUbtbTrpl142tlczNAKuHBFnPXq+m Q0DlqsikqybowYK40o0M6jdvBRRqFW5qtD2Rck1kBlZ4tuBADtbd/cfdi3i8DtgJEmkj GX/YGdle0OJ27Unwl5XixXa6nOkagyU4kvIDHd5kv+KJzBZsdk0zGDfk58G67ArwnIHc 9v+3dut1hUIoBDtOf4KpF9apCpvP0cgbWfvVliJljW7RqUl/v/MDEBG/b1lSTt5TKnr2 nGXa+pJVgn/eJ+QPK5E+JsDJ9iQ3naAfsxbX7Lak1LOQo2Ro7C1l2Oq/qClYljr/j+2p fzNA== X-Gm-Message-State: AOJu0YxXMt4/YbSsy/7xxaTUQATPlYafijio6RfRIswA7sL1a9rkRxjA T3eU4pkBWmDvbqot1VV81Vx8oBt1pJD8dpYXjUr8p/1t7V0xwBWqiLPhVYu46BrPukiJ8XLlh75 dFiRZWNijkTUsFWGD58R7FpzSUkLLkqh8wJMj4ch2DiP6fC/HfUbGi5btEg5nlFm8zNg= X-Received: by 2002:a5d:64c1:0:b0:339:2c1a:5d79 with SMTP id f1-20020a5d64c1000000b003392c1a5d79mr1906650wri.6.1707235565337; Tue, 06 Feb 2024 08:06:05 -0800 (PST) X-Google-Smtp-Source: AGHT+IEib7HGznRR63uyEkNBiRlvNh8+lJfg2zxT7RveAAp9t5/LwqhSAmJzelw56peJeCSxebu8Jw== X-Received: by 2002:a5d:64c1:0:b0:339:2c1a:5d79 with SMTP id f1-20020a5d64c1000000b003392c1a5d79mr1906628wri.6.1707235564852; Tue, 06 Feb 2024 08:06:04 -0800 (PST) X-Forwarded-Encrypted: i=0; AJvYcCW9Xgq5hVWKI0kiVVnluyDtCG9nUKxwWTkdY2zA5uOuw7/v5g0p53yTFFLY89KelN5lMISjyzTVsqOzHS1Dz8mNrpcXvwYewEDLHA== Received: from localhost (185.223.159.143.dyn.plus.net. [143.159.223.185]) by smtp.gmail.com with ESMTPSA id e16-20020a5d5010000000b0033b46b1b6adsm1885477wrt.21.2024.02.06.08.06.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Feb 2024 08:06:04 -0800 (PST) From: Andrew Burgess To: Alexandra =?utf-8?B?SMOhamtvdsOh?= , gdb-patches@sourceware.org Subject: Re: [PATCH v5] remote.c: Make packet_check_result return a structure In-Reply-To: <20240205102117.34147-1-ahajkova@redhat.com> References: <20240205102117.34147-1-ahajkova@redhat.com> Date: Tue, 06 Feb 2024 16:06:03 +0000 Message-ID: <87eddp8tn8.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.2 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_H5,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 > --- > v5: - rewrote some parts of the unit test to to check both the status > and the err_msg. > - cosmetics > - improve treating the case when we get a response from gdbserver > with an empty "E.", GDB would print 'no error provided' in such a > case > > gdb/remote.c | 139 ++++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 104 insertions(+), 35 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index f77548427a4..fb04faf67b2 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. */ > =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 +765,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 +1287,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 +2451,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 +2465,46 @@ 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; > +=09{ > +=09 if (buf[2] !=3D '\0') > +=09 return { PACKET_ERROR, buf + 2 }; > +=09 else > +=09 return { PACKET_ERROR, "no error provided" }; > +=09} > =20 > /* 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); > + } > } > =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 +2539,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 +3041,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) > @@ -8790,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'"), > -=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 > @@ -9098,9 +9140,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 > @@ -9682,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. */ > =20 > -packet_result > +packet_status > remote_target::remote_send_printf (const char *format, ...) > { > struct remote_state *rs =3D get_remote_state (); > @@ -9705,7 +9748,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 > @@ -9717,7 +9760,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 > @@ -11307,7 +11350,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 (); > @@ -11323,10 +11366,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); > @@ -12209,7 +12252,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); > @@ -12255,7 +12298,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); > @@ -13820,7 +13863,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) > @@ -14200,7 +14243,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:"); > @@ -15528,7 +15571,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. */ > @@ -15625,8 +15668,32 @@ 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)= ; > + SELF_CHECK (strcmp(result.err_msg (), "msg") =3D=3D 0); This isn't right, it should be: result =3D packet_check_result ("E01"); SELF_CHECK (result.status () =3D=3D PACKET_ERROR); SELF_CHECK (strcmp(result.err_msg (), "01") =3D=3D 0); > + > + SELF_CHECK (packet_check_result ("E1").status () =3D=3D PACKET_OK); > + > + SELF_CHECK (packet_check_result ("E000").status () =3D=3D PACKET_OK); > +=20 You have a trailing white space on this line. If you 'git show' it should highlight this for you as a result of our .gitattributes file. If this isn't highlighted I'd love to know so we can get this fixed. > + result =3D packet_check_result ("E."); > + SELF_CHECK (result.status () =3D=3D PACKET_ERROR); > + SELF_CHECK (strcmp(result.err_msg (), "no error provided") =3D=3D 0); > + > + 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 */ The change in this comment (after the #endif) is a mistake and needs fixing= . Thanks, Andrew > =20 > void _initialize_remote (); > void > @@ -16124,5 +16191,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