[2/2] remote.c: Make packet_ok return struct packet_result

  This allows to print the error message stored in a packet_result
to be easily used in the calling function.
---
 gdb/remote.c | 166 +++++++++++++++++++++++----------------------------
 1 file changed, 76 insertions(+), 90 deletions(-)
  

ping

On Mon, Feb 12, 2024 at 3:40 PM Alexandra Hájková <ahajkova@redhat.com>
wrote:

> This allows to print the error message stored in a packet_result
> to be easily used in the calling function.
> ---
>  gdb/remote.c | 166 +++++++++++++++++++++++----------------------------
>  1 file changed, 76 insertions(+), 90 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 8caee0dcff9..85f5624f2b6 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -765,8 +765,8 @@ struct remote_features
>
>  /* Check result value in BUF for packet WHICH_PACKET and update the
> packet's
>     support configuration accordingly.  */
> -  packet_status packet_ok (const char *buf, const int which_packet);
> -  packet_status packet_ok (const gdb::char_vector &buf, const int
> which_packet);
> +  packet_result packet_ok (const char *buf, const int which_packet);
> +  packet_result 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;
> @@ -2499,7 +2499,7 @@ packet_check_result (const gdb::char_vector &buf,
> bool accept_msg)
>    return packet_check_result (buf.data (), accept_msg);
>  }
>
> -packet_status
> +packet_result
>  remote_features::packet_ok (const char *buf, const int which_packet)
>  {
>    packet_config *config = &m_protocol_packets[which_packet];
> @@ -2545,10 +2545,10 @@ remote_features::packet_ok (const char *buf, const
> int which_packet)
>        break;
>      }
>
> -  return result.status ();
> +  return result;
>  }
>
> -packet_status
> +packet_result
>  remote_features::packet_ok (const gdb::char_vector &buf, const int
> which_packet)
>  {
>    return packet_ok (buf.data (), which_packet);
> @@ -2735,14 +2735,15 @@ remote_target::remote_query_attached (int pid)
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, PACKET_qAttached))
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_qAttached);
> +  switch (result.status())
>      {
>      case PACKET_OK:
>        if (strcmp (rs->buf.data (), "1") == 0)
>         return 1;
>        break;
>      case PACKET_ERROR:
> -      warning (_("Remote failure reply: %s"), rs->buf.data ());
> +      warning (_("Remote failure reply: %s"), result.err_msg());
>        break;
>      case PACKET_UNKNOWN:
>        break;
> @@ -3047,7 +3048,6 @@ remote_target::set_syscall_catchpoint (int pid, bool
> needed, int any_count,
>                                        gdb::array_view<const int>
> syscall_counts)
>  {
>    const char *catch_packet;
> -  enum packet_status result;
>    int n_sysno = 0;
>
>    if (m_features.packet_support (PACKET_QCatchSyscalls) == PACKET_DISABLE)
> @@ -3103,8 +3103,8 @@ remote_target::set_syscall_catchpoint (int pid, bool
> needed, int any_count,
>
>    putpkt (catch_packet);
>    getpkt (&rs->buf);
> -  result = m_features.packet_ok (rs->buf, PACKET_QCatchSyscalls);
> -  if (result == PACKET_OK)
> +  packet_result result = m_features.packet_ok (rs->buf,
> PACKET_QCatchSyscalls);
> +  if (result.status() == PACKET_OK)
>      return 0;
>    else
>      return -1;
> @@ -5109,7 +5109,8 @@ remote_target::start_remote_1 (int from_tty, int
> extended_p)
>      {
>        putpkt ("QStartNoAckMode");
>        getpkt (&rs->buf);
> -      if (m_features.packet_ok (rs->buf, PACKET_QStartNoAckMode) ==
> PACKET_OK)
> +      if ((m_features.packet_ok (rs->buf, PACKET_QStartNoAckMode)).status
> ()
> +         == PACKET_OK)
>         rs->noack_mode = 1;
>      }
>
> @@ -5894,9 +5895,10 @@ remote_target::remote_query_supported ()
>
>        /* If an error occurred, warn, but do not return - just reset the
>          buffer to empty and go on to disable features.  */
> -      if (m_features.packet_ok (rs->buf, PACKET_qSupported) ==
> PACKET_ERROR)
> +      packet_result result = m_features.packet_ok (rs->buf,
> PACKET_qSupported);
> +      if (result.status () == PACKET_ERROR)
>         {
> -         warning (_("Remote failure reply: %s"), rs->buf.data ());
> +         warning (_("Remote failure reply: %s"), result.err_msg ());
>           rs->buf[0] = 0;
>         }
>      }
> @@ -6548,7 +6550,8 @@ extended_remote_target::attach (const char *args,
> int from_tty)
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, PACKET_vAttach))
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_vAttach);
> +  switch (result.status ())
>      {
>      case PACKET_OK:
>        if (!target_is_non_stop_p ())
> @@ -6560,7 +6563,7 @@ extended_remote_target::attach (const char *args,
> int from_tty)
>        else if (strcmp (rs->buf.data (), "OK") != 0)
>         error (_("Attaching to %s failed with: %s"),
>                target_pid_to_str (ptid_t (pid)).c_str (),
> -              rs->buf.data ());
> +              result.err_msg ());
>        break;
>      case PACKET_UNKNOWN:
>        error (_("This target does not support attaching to a process"));
> @@ -7489,14 +7492,15 @@ remote_target::remote_interrupt_ns ()
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, PACKET_vCtrlC))
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_vCtrlC);
> +  switch (result.status ())
>      {
>      case PACKET_OK:
>        break;
>      case PACKET_UNKNOWN:
>        error (_("No support for interrupting the remote target."));
>      case PACKET_ERROR:
> -      error (_("Interrupting target failed: %s"), rs->buf.data ());
> +      error (_("Interrupting target failed: %s"), result.err_msg ());
>      }
>  }
>
> @@ -8792,7 +8796,8 @@ remote_target::fetch_register_using_p (struct
> regcache *regcache,
>
>    buf = rs->buf.data ();
>
> -  switch (m_features.packet_ok (rs->buf, PACKET_p))
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_p);
> +  switch (result.status ())
>      {
>      case PACKET_OK:
>        break;
> @@ -8801,7 +8806,7 @@ remote_target::fetch_register_using_p (struct
> regcache *regcache,
>      case PACKET_ERROR:
>        error (_("Could not fetch register \"%s\"; remote failure reply
> '%s'"),
>              gdbarch_register_name (regcache->arch (), reg->regnum),
> -            buf);
> +            result.err_msg ());
>      }
>
>    /* If this register is unfetchable, tell the regcache.  */
> @@ -9098,13 +9103,14 @@ remote_target::store_register_using_P (const
> struct regcache *regcache,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, PACKET_P))
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_P);
> +  switch (result.status ())
>      {
>      case PACKET_OK:
>        return 1;
>      case PACKET_ERROR:
>        error (_("Could not write register \"%s\"; remote failure reply
> '%s'"),
> -            gdbarch_register_name (gdbarch, reg->regnum), rs->buf.data
> ());
> +            gdbarch_register_name (gdbarch, reg->regnum), result.err_msg
> ());
>      case PACKET_UNKNOWN:
>        return 0;
>      default:
> @@ -10535,7 +10541,7 @@ remote_target::remote_vkill (int pid)
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, PACKET_vKill))
> +  switch ((m_features.packet_ok (rs->buf, PACKET_vKill)).status ())
>      {
>      case PACKET_OK:
>        return 0;
> @@ -10691,7 +10697,7 @@ remote_target::extended_remote_run (const
> std::string &args)
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, PACKET_vRun))
> +  switch ((m_features.packet_ok (rs->buf, PACKET_vRun)).status ())
>      {
>      case PACKET_OK:
>        /* We have a wait response.  All is well.  */
> @@ -10798,11 +10804,12 @@ remote_target::extended_remote_set_inferior_cwd
> ()
>
>        putpkt (rs->buf);
>        getpkt (&rs->buf);
> -      if (m_features.packet_ok (rs->buf, PACKET_QSetWorkingDir) !=
> PACKET_OK)
> +      packet_result result = m_features.packet_ok (rs->buf,
> PACKET_QSetWorkingDir);
> +      if (result.status () != PACKET_OK)
>         error (_("\
>  Remote replied unexpectedly while setting the inferior's working\n\
>  directory: %s"),
> -              rs->buf.data ());
> +              result.err_msg ());
>
>      }
>  }
> @@ -10971,7 +10978,7 @@ remote_target::insert_breakpoint (struct gdbarch
> *gdbarch,
>        putpkt (rs->buf);
>        getpkt (&rs->buf);
>
> -      switch (m_features.packet_ok (rs->buf, PACKET_Z0))
> +      switch ((m_features.packet_ok (rs->buf, PACKET_Z0)).status ())
>         {
>         case PACKET_ERROR:
>           return -1;
> @@ -11072,8 +11079,8 @@ remote_target::insert_watchpoint (CORE_ADDR addr,
> int len,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> -                                         + to_underlying (packet))))
> +  switch ((m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> +                                         + to_underlying
> (packet)))).status ())
>      {
>      case PACKET_ERROR:
>        return -1;
> @@ -11121,8 +11128,8 @@ remote_target::remove_watchpoint (CORE_ADDR addr,
> int len,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> -                                         + to_underlying (packet))))
> +  switch ((m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> +                                         + to_underlying
> (packet)))).status ())
>      {
>      case PACKET_ERROR:
>      case PACKET_UNKNOWN:
> @@ -11284,7 +11291,7 @@ remote_target::insert_hw_breakpoint (struct
> gdbarch *gdbarch,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, PACKET_Z1))
> +  switch ((m_features.packet_ok (rs->buf, PACKET_Z1)).status ())
>      {
>      case PACKET_ERROR:
>        if (rs->buf[1] == '.')
> @@ -11331,7 +11338,7 @@ remote_target::remove_hw_breakpoint (struct
> gdbarch *gdbarch,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, PACKET_Z1))
> +  switch ((m_features.packet_ok (rs->buf, PACKET_Z1)).status ())
>      {
>      case PACKET_ERROR:
>      case PACKET_UNKNOWN:
> @@ -11372,7 +11379,7 @@ remote_target::verify_memory (const gdb_byte
> *data, CORE_ADDR lma, ULONGEST size
>
>        getpkt (&rs->buf);
>
> -      status = m_features.packet_ok (rs->buf, PACKET_qCRC);
> +      status = (m_features.packet_ok (rs->buf, PACKET_qCRC)).status ();
>        if (status == PACKET_ERROR)
>         return -1;
>        else if (status == PACKET_OK)
> @@ -11494,7 +11501,7 @@ remote_target::remote_write_qxfer (const char
> *object_name,
>
>    if (putpkt_binary (rs->buf.data (), i + buf_len) < 0
>        || getpkt (&rs->buf) < 0
> -      || m_features.packet_ok (rs->buf, which_packet) != PACKET_OK)
> +      || (m_features.packet_ok (rs->buf, which_packet)).status () !=
> PACKET_OK)
>      return TARGET_XFER_E_IO;
>
>    unpack_varlen_hex (rs->buf.data (), &n);
> @@ -11559,7 +11566,7 @@ remote_target::remote_read_qxfer (const char
> *object_name,
>    rs->buf[0] = '\0';
>    packet_len = getpkt (&rs->buf);
>    if (packet_len < 0
> -      || m_features.packet_ok (rs->buf, which_packet) != PACKET_OK)
> +      || (m_features.packet_ok (rs->buf, which_packet)).status () !=
> PACKET_OK)
>      return TARGET_XFER_E_IO;
>
>    if (rs->buf[0] != 'l' && rs->buf[0] != 'm')
> @@ -11864,7 +11871,8 @@ remote_target::search_memory (CORE_ADDR
> start_addr, ULONGEST search_space_len,
>
>    if (putpkt_binary (rs->buf.data (), i + escaped_pattern_len) < 0
>        || getpkt (&rs->buf) < 0
> -      || m_features.packet_ok (rs->buf, PACKET_qSearch_memory) !=
> PACKET_OK)
> +      || (m_features.packet_ok (rs->buf, PACKET_qSearch_memory)).status ()
> +      != PACKET_OK)
>      {
>        /* The request may not have worked because the command is not
>          supported.  If so, fall back to the simple way.  */
> @@ -12257,7 +12265,6 @@ remote_target::get_thread_local_address (ptid_t
> ptid, CORE_ADDR lm,
>        struct remote_state *rs = get_remote_state ();
>        char *p = rs->buf.data ();
>        char *endp = p + get_remote_packet_size ();
> -      enum packet_status result;
>
>        strcpy (p, "qGetTLSAddr:");
>        p += strlen (p);
> @@ -12270,15 +12277,15 @@ remote_target::get_thread_local_address (ptid_t
> ptid, CORE_ADDR lm,
>
>        putpkt (rs->buf);
>        getpkt (&rs->buf);
> -      result = m_features.packet_ok (rs->buf, PACKET_qGetTLSAddr);
> -      if (result == PACKET_OK)
> +      packet_result result = m_features.packet_ok (rs->buf,
> PACKET_qGetTLSAddr);
> +      if (result.status () == PACKET_OK)
>         {
>           ULONGEST addr;
>
>           unpack_varlen_hex (rs->buf.data (), &addr);
>           return addr;
>         }
> -      else if (result == PACKET_UNKNOWN)
> +      else if (result.status () == PACKET_UNKNOWN)
>         throw_error (TLS_GENERIC_ERROR,
>                      _("Remote target doesn't support qGetTLSAddr
> packet"));
>        else
> @@ -12303,7 +12310,6 @@ remote_target::get_tib_address (ptid_t ptid,
> CORE_ADDR *addr)
>        struct remote_state *rs = get_remote_state ();
>        char *p = rs->buf.data ();
>        char *endp = p + get_remote_packet_size ();
> -      enum packet_status result;
>
>        strcpy (p, "qGetTIBAddr:");
>        p += strlen (p);
> @@ -12312,8 +12318,8 @@ remote_target::get_tib_address (ptid_t ptid,
> CORE_ADDR *addr)
>
>        putpkt (rs->buf);
>        getpkt (&rs->buf);
> -      result = m_features.packet_ok (rs->buf, PACKET_qGetTIBAddr);
> -      if (result == PACKET_OK)
> +      packet_result result = m_features.packet_ok (rs->buf,
> PACKET_qGetTIBAddr);
> +      if (result.status () == PACKET_OK)
>         {
>           ULONGEST val;
>           unpack_varlen_hex (rs->buf.data (), &val);
> @@ -12321,7 +12327,7 @@ remote_target::get_tib_address (ptid_t ptid,
> CORE_ADDR *addr)
>             *addr = (CORE_ADDR) val;
>           return true;
>         }
> -      else if (result == PACKET_UNKNOWN)
> +      else if (result.status () == PACKET_UNKNOWN)
>         error (_("Remote target doesn't support qGetTIBAddr packet"));
>        else
>         error (_("Remote target failed to process qGetTIBAddr request"));
> @@ -12580,7 +12586,7 @@ remote_target::remote_hostio_send_command (int
> command_bytes, int which_packet,
>        return -1;
>      }
>
> -  switch (m_features.packet_ok (rs->buf, which_packet))
> +  switch ((m_features.packet_ok (rs->buf, which_packet)).status ())
>      {
>      case PACKET_ERROR:
>        *remote_errno = FILEIO_EINVAL;
> @@ -13868,7 +13874,6 @@ remote_target::get_trace_status (struct
> trace_status *ts)
>  {
>    /* Initialize it just to avoid a GCC false warning.  */
>    char *p = NULL;
> -  enum packet_status result;
>    struct remote_state *rs = get_remote_state ();
>
>    if (m_features.packet_support (PACKET_qTStatus) == PACKET_DISABLE)
> @@ -13894,17 +13899,17 @@ remote_target::get_trace_status (struct
> trace_status *ts)
>        throw;
>      }
>
> -  result = m_features.packet_ok (p, PACKET_qTStatus);
> +  packet_result result = m_features.packet_ok (p, PACKET_qTStatus);
>
>    /* If the remote target doesn't do tracing, flag it.  */
> -  if (result == PACKET_UNKNOWN)
> +  if (result.status () == PACKET_UNKNOWN)
>      return -1;
>
>    /* We're working with a live target.  */
>    ts->filename = NULL;
>
>    if (*p++ != 'T')
> -    error (_("Bogus trace status reply from target: %s"), rs->buf.data
> ());
> +    error (_("Bogus trace status reply from target: %s"), result.err_msg
> ());
>
>    /* Function 'parse_trace_status' sets default value of each field of
>       'ts' at first, so we don't have to do it here.  */
> @@ -14248,7 +14253,6 @@ remote_target::set_trace_buffer_size (LONGEST val)
>        struct remote_state *rs = get_remote_state ();
>        char *buf = rs->buf.data ();
>        char *endbuf = buf + get_remote_packet_size ();
> -      enum packet_status result;
>
>        gdb_assert (val >= 0 || val == -1);
>        buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
> @@ -14263,10 +14267,10 @@ remote_target::set_trace_buffer_size (LONGEST
> val)
>
>        putpkt (rs->buf);
>        remote_get_noisy_reply ();
> -      result = m_features.packet_ok (rs->buf, PACKET_QTBuffer_size);
> +      packet_result result = m_features.packet_ok (rs->buf,
> PACKET_QTBuffer_size);
>
> -      if (result != PACKET_OK)
> -       warning (_("Bogus reply from target: %s"), rs->buf.data ());
> +      if (result.status () != PACKET_OK)
> +       warning (_("Bogus reply from target: %s"), result.err_msg ());
>      }
>  }
>
> @@ -14692,14 +14696,9 @@ remote_target::btrace_sync_conf (const
> btrace_config *conf)
>        putpkt (buf);
>        getpkt (&rs->buf);
>
> -      if (m_features.packet_ok (buf, PACKET_Qbtrace_conf_bts_size)
> -         == PACKET_ERROR)
> -       {
> -         if (buf[0] == 'E' && buf[1] == '.')
> -           error (_("Failed to configure the BTS buffer size: %s"), buf +
> 2);
> -         else
> -           error (_("Failed to configure the BTS buffer size."));
> -       }
> +      packet_result result = m_features.packet_ok (buf,
> PACKET_Qbtrace_conf_bts_size);
> +      if (result.status () == PACKET_ERROR)
> +       error (_("Failed to configure the BTS buffer size: %s"),
> result.err_msg ());
>
>        rs->btrace_config.bts.size = conf->bts.size;
>      }
> @@ -14715,14 +14714,9 @@ remote_target::btrace_sync_conf (const
> btrace_config *conf)
>        putpkt (buf);
>        getpkt (&rs->buf);
>
> -      if (m_features.packet_ok (buf, PACKET_Qbtrace_conf_pt_size)
> -         == PACKET_ERROR)
> -       {
> -         if (buf[0] == 'E' && buf[1] == '.')
> -           error (_("Failed to configure the trace buffer size: %s"), buf
> + 2);
> -         else
> -           error (_("Failed to configure the trace buffer size."));
> -       }
> +      packet_result result = m_features.packet_ok (buf,
> PACKET_Qbtrace_conf_pt_size);
> +      if (result.status () == PACKET_ERROR)
> +       error (_("Failed to configure the trace buffer size: %s"),
> result.err_msg ());
>
>        rs->btrace_config.pt.size = conf->pt.size;
>      }
> @@ -14837,15 +14831,10 @@ remote_target::enable_btrace (thread_info *tp,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  if (m_features.packet_ok (rs->buf, which_packet) == PACKET_ERROR)
> -    {
> -      if (rs->buf[0] == 'E' && rs->buf[1] == '.')
> -       error (_("Could not enable branch tracing for %s: %s"),
> -              target_pid_to_str (ptid).c_str (), &rs->buf[2]);
> -      else
> -       error (_("Could not enable branch tracing for %s."),
> -              target_pid_to_str (ptid).c_str ());
> -    }
> +  packet_result result = m_features.packet_ok (rs->buf, which_packet);
> +  if (result.status () == PACKET_ERROR)
> +    error (_("Could not enable branch tracing for %s: %s"),
> +          target_pid_to_str (ptid).c_str (), result.err_msg ());
>
>    btrace_target_info *tinfo = new btrace_target_info { ptid };
>
> @@ -14883,15 +14872,10 @@ remote_target::disable_btrace (struct
> btrace_target_info *tinfo)
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  if (m_features.packet_ok (rs->buf, PACKET_Qbtrace_off) == PACKET_ERROR)
> -    {
> -      if (rs->buf[0] == 'E' && rs->buf[1] == '.')
> +  packet_result result = m_features.packet_ok (rs->buf,
> PACKET_Qbtrace_off);
> +  if (result.status () == PACKET_ERROR)
>         error (_("Could not disable branch tracing for %s: %s"),
> -              target_pid_to_str (tinfo->ptid).c_str (), &rs->buf[2]);
> -      else
> -       error (_("Could not disable branch tracing for %s."),
> -              target_pid_to_str (tinfo->ptid).c_str ());
> -    }
> +              target_pid_to_str (tinfo->ptid).c_str (), result.err_msg
> ());
>
>    delete tinfo;
>  }
> @@ -15156,7 +15140,8 @@ remote_target::thread_events (int enable)
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>
> -  switch (m_features.packet_ok (rs->buf, PACKET_QThreadEvents))
> +  packet_result result = m_features.packet_ok (rs->buf,
> PACKET_QThreadEvents);
> +  switch (result.status ())
>      {
>      case PACKET_OK:
>        if (strcmp (rs->buf.data (), "OK") != 0)
> @@ -15164,7 +15149,7 @@ remote_target::thread_events (int enable)
>        rs->last_thread_events = enable;
>        break;
>      case PACKET_ERROR:
> -      warning (_("Remote failure reply: %s"), rs->buf.data ());
> +      warning (_("Remote failure reply: %s"), result.err_msg ());
>        break;
>      case PACKET_UNKNOWN:
>        break;
> @@ -15211,14 +15196,15 @@ remote_target::commit_requested_thread_options ()
>        putpkt (rs->buf);
>        getpkt (&rs->buf, 0);
>
> -      switch (m_features.packet_ok (rs->buf, PACKET_QThreadOptions))
> +      packet_result result = m_features.packet_ok (rs->buf,
> PACKET_QThreadOptions);
> +      switch (result.status ())
>         {
>         case PACKET_OK:
>           if (strcmp (rs->buf.data (), "OK") != 0)
>             error (_("Remote refused setting thread options: %s"),
> rs->buf.data ());
>           break;
>         case PACKET_ERROR:
> -         error (_("Remote failure reply: %s"), rs->buf.data ());
> +         error (_("Remote failure reply: %s"), result.err_msg ());
>         case PACKET_UNKNOWN:
>           gdb_assert_not_reached ("PACKET_UNKNOWN");
>           break;
> --
> 2.43.0
>
>
  

Alexandra Hájková <ahajkova@redhat.com> writes:

> This allows to print the error message stored in a packet_result
> to be easily used in the calling function.
> ---
>  gdb/remote.c | 166 +++++++++++++++++++++++----------------------------
>  1 file changed, 76 insertions(+), 90 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 8caee0dcff9..85f5624f2b6 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -765,8 +765,8 @@ struct remote_features
>  
>  /* Check result value in BUF for packet WHICH_PACKET and update the packet's
>     support configuration accordingly.  */
> -  packet_status packet_ok (const char *buf, const int which_packet);
> -  packet_status packet_ok (const gdb::char_vector &buf, const int which_packet);
> +  packet_result packet_ok (const char *buf, const int which_packet);
> +  packet_result 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;
> @@ -2499,7 +2499,7 @@ packet_check_result (const gdb::char_vector &buf, bool accept_msg)
>    return packet_check_result (buf.data (), accept_msg);
>  }
>  
> -packet_status
> +packet_result
>  remote_features::packet_ok (const char *buf, const int which_packet)
>  {
>    packet_config *config = &m_protocol_packets[which_packet];
> @@ -2545,10 +2545,10 @@ remote_features::packet_ok (const char *buf, const int which_packet)
>        break;
>      }
>  
> -  return result.status ();
> +  return result;
>  }
>  
> -packet_status
> +packet_result
>  remote_features::packet_ok (const gdb::char_vector &buf, const int which_packet)
>  {
>    return packet_ok (buf.data (), which_packet);
> @@ -2735,14 +2735,15 @@ remote_target::remote_query_attached (int pid)
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> -  switch (m_features.packet_ok (rs->buf, PACKET_qAttached))
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_qAttached);
> +  switch (result.status())
>      {
>      case PACKET_OK:
>        if (strcmp (rs->buf.data (), "1") == 0)
>  	return 1;
>        break;
>      case PACKET_ERROR:
> -      warning (_("Remote failure reply: %s"), rs->buf.data ());
> +      warning (_("Remote failure reply: %s"), result.err_msg());
>        break;
>      case PACKET_UNKNOWN:
>        break;
> @@ -3047,7 +3048,6 @@ remote_target::set_syscall_catchpoint (int pid, bool needed, int any_count,
>  				       gdb::array_view<const int> syscall_counts)
>  {
>    const char *catch_packet;
> -  enum packet_status result;
>    int n_sysno = 0;
>  
>    if (m_features.packet_support (PACKET_QCatchSyscalls) == PACKET_DISABLE)
> @@ -3103,8 +3103,8 @@ remote_target::set_syscall_catchpoint (int pid, bool needed, int any_count,
>  
>    putpkt (catch_packet);
>    getpkt (&rs->buf);
> -  result = m_features.packet_ok (rs->buf, PACKET_QCatchSyscalls);
> -  if (result == PACKET_OK)
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_QCatchSyscalls);
> +  if (result.status() == PACKET_OK)
>      return 0;
>    else
>      return -1;
> @@ -5109,7 +5109,8 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
>      {
>        putpkt ("QStartNoAckMode");
>        getpkt (&rs->buf);
> -      if (m_features.packet_ok (rs->buf, PACKET_QStartNoAckMode) == PACKET_OK)
> +      if ((m_features.packet_ok (rs->buf, PACKET_QStartNoAckMode)).status ()
> +	  == PACKET_OK)
>  	rs->noack_mode = 1;
>      }
>  
> @@ -5894,9 +5895,10 @@ remote_target::remote_query_supported ()
>  
>        /* If an error occurred, warn, but do not return - just reset the
>  	 buffer to empty and go on to disable features.  */
> -      if (m_features.packet_ok (rs->buf, PACKET_qSupported) == PACKET_ERROR)
> +      packet_result result = m_features.packet_ok (rs->buf, PACKET_qSupported);
> +      if (result.status () == PACKET_ERROR)
>  	{
> -	  warning (_("Remote failure reply: %s"), rs->buf.data ());
> +	  warning (_("Remote failure reply: %s"), result.err_msg ());
>  	  rs->buf[0] = 0;
>  	}
>      }
> @@ -6548,7 +6550,8 @@ extended_remote_target::attach (const char *args, int from_tty)
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> -  switch (m_features.packet_ok (rs->buf, PACKET_vAttach))
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_vAttach);
> +  switch (result.status ())
>      {
>      case PACKET_OK:
>        if (!target_is_non_stop_p ())
> @@ -6560,7 +6563,7 @@ extended_remote_target::attach (const char *args, int from_tty)
>        else if (strcmp (rs->buf.data (), "OK") != 0)
>  	error (_("Attaching to %s failed with: %s"),
>  	       target_pid_to_str (ptid_t (pid)).c_str (),
> -	       rs->buf.data ());
> +	       result.err_msg ());

I don't think this change is correct.  Notice that we're inside a
PACKET_OK case at this point.  This is slightly confusing.  When a
packet has packet_status PACKET_OK, it means the contents are not one of
the error forms, but otherwise the content could be anything.  'FOO'
would be PACKET_OK, as would the string 'OK'.

I wonder if this error() call should have a slightly different message,
maybe:

  error (_("Attaching to %s returned unexpected packet: %s"),
         target_pid_to_str (ptid_t (pid)).c_str (),
         rs->buf.data ());

But the important thing is that we need to print the packet contents,
not 'result.err_msg ()' as that will not have been set.

Meanwhile, within this same switch statement, the 'default' case is
where PACKET_ERROR is handled, in this case it might be nice to add
printing of 'result.err_msg ()' as currently we don't do that.  Maybe
the 'default:' case label should be changed to 'case PACKET_ERROR:',
relying on 'default:' here just seems sloppy.

>        break;
>      case PACKET_UNKNOWN:
>        error (_("This target does not support attaching to a process"));
> @@ -7489,14 +7492,15 @@ remote_target::remote_interrupt_ns ()
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> -  switch (m_features.packet_ok (rs->buf, PACKET_vCtrlC))
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_vCtrlC);
> +  switch (result.status ())
>      {
>      case PACKET_OK:
>        break;
>      case PACKET_UNKNOWN:
>        error (_("No support for interrupting the remote target."));
>      case PACKET_ERROR:
> -      error (_("Interrupting target failed: %s"), rs->buf.data ());
> +      error (_("Interrupting target failed: %s"), result.err_msg ());
>      }
>  }
>  
> @@ -8792,7 +8796,8 @@ remote_target::fetch_register_using_p (struct regcache *regcache,
>  
>    buf = rs->buf.data ();
>  
> -  switch (m_features.packet_ok (rs->buf, PACKET_p))
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_p);
> +  switch (result.status ())
>      {
>      case PACKET_OK:
>        break;
> @@ -8801,7 +8806,7 @@ remote_target::fetch_register_using_p (struct regcache *regcache,
>      case PACKET_ERROR:
>        error (_("Could not fetch register \"%s\"; remote failure reply '%s'"),
>  	     gdbarch_register_name (regcache->arch (), reg->regnum),
> -	     buf);
> +	     result.err_msg ());
>      }
>  
>    /* If this register is unfetchable, tell the regcache.  */
> @@ -9098,13 +9103,14 @@ remote_target::store_register_using_P (const struct regcache *regcache,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> -  switch (m_features.packet_ok (rs->buf, PACKET_P))
> +  packet_result result = m_features.packet_ok (rs->buf, PACKET_P);
> +  switch (result.status ())
>      {
>      case PACKET_OK:
>        return 1;
>      case PACKET_ERROR:
>        error (_("Could not write register \"%s\"; remote failure reply '%s'"),
> -	     gdbarch_register_name (gdbarch, reg->regnum), rs->buf.data ());
> +	     gdbarch_register_name (gdbarch, reg->regnum), result.err_msg ());
>      case PACKET_UNKNOWN:
>        return 0;
>      default:
> @@ -10535,7 +10541,7 @@ remote_target::remote_vkill (int pid)
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> -  switch (m_features.packet_ok (rs->buf, PACKET_vKill))
> +  switch ((m_features.packet_ok (rs->buf, PACKET_vKill)).status ())
>      {
>      case PACKET_OK:
>        return 0;
> @@ -10691,7 +10697,7 @@ remote_target::extended_remote_run (const std::string &args)
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> -  switch (m_features.packet_ok (rs->buf, PACKET_vRun))
> +  switch ((m_features.packet_ok (rs->buf, PACKET_vRun)).status ())
>      {
>      case PACKET_OK:
>        /* We have a wait response.  All is well.  */
> @@ -10798,11 +10804,12 @@ remote_target::extended_remote_set_inferior_cwd ()
>  
>        putpkt (rs->buf);
>        getpkt (&rs->buf);
> -      if (m_features.packet_ok (rs->buf, PACKET_QSetWorkingDir) != PACKET_OK)
> +      packet_result result = m_features.packet_ok (rs->buf, PACKET_QSetWorkingDir);
> +      if (result.status () != PACKET_OK)
>  	error (_("\
>  Remote replied unexpectedly while setting the inferior's working\n\
>  directory: %s"),
> -	       rs->buf.data ());
> +	       result.err_msg ());

We need to be careful here too.  There's a risk that we might get
PACKET_UNKNOWN in which case calling 'result.err_msg ()' will be
invalid.  I think you might need to change this into a switch statement
and handle each case differently:

  PACKET_OK - nothing to do,
  PACKET_ERROR - print a message including the error text,
  PACKET_UNKNOWN - tell the user that the remote doesn't support this.

>  
>      }
>  }
> @@ -10971,7 +10978,7 @@ remote_target::insert_breakpoint (struct gdbarch *gdbarch,
>        putpkt (rs->buf);
>        getpkt (&rs->buf);
>  
> -      switch (m_features.packet_ok (rs->buf, PACKET_Z0))
> +      switch ((m_features.packet_ok (rs->buf, PACKET_Z0)).status ())
>  	{
>  	case PACKET_ERROR:
>  	  return -1;
> @@ -11072,8 +11079,8 @@ remote_target::insert_watchpoint (CORE_ADDR addr, int len,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> -  switch (m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> -					  + to_underlying (packet))))
> +  switch ((m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> +					  + to_underlying (packet)))).status ())
>      {
>      case PACKET_ERROR:
>        return -1;
> @@ -11121,8 +11128,8 @@ remote_target::remove_watchpoint (CORE_ADDR addr, int len,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> -  switch (m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> -					  + to_underlying (packet))))
> +  switch ((m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> +					  + to_underlying (packet)))).status ())
>      {
>      case PACKET_ERROR:
>      case PACKET_UNKNOWN:
> @@ -11284,7 +11291,7 @@ remote_target::insert_hw_breakpoint (struct gdbarch *gdbarch,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> -  switch (m_features.packet_ok (rs->buf, PACKET_Z1))
> +  switch ((m_features.packet_ok (rs->buf, PACKET_Z1)).status ())
>      {
>      case PACKET_ERROR:
>        if (rs->buf[1] == '.')

So the code here (the existing code, not what you've written) is a real
mess.  In the PACKET_ERROR case we attempt to "find" an error message in
the case that the packet is an 'E.msg' style packet.

However, if we do then we call 'error()', meanwhile, if the packet isn't
an 'E.msg' style packet, we return -1, which is pretty different than
throwing an exception.

This caught my eye because when I say the 'E.msg' check I thought, Oh,
we can just capture the packet_result object and call ::err_msg(),
however, this leads to an obvious question: should we throw an
exception, or return -1?

The comment in target.h for target_insert_hw_breakpoint suggests that
the function can _either_ return non-zero OR throw an exception on
error, so I'd be tempted to move to an always throw an error solution
for the PACKET_ERROR case.

> @@ -11331,7 +11338,7 @@ remote_target::remove_hw_breakpoint (struct gdbarch *gdbarch,
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> -  switch (m_features.packet_ok (rs->buf, PACKET_Z1))
> +  switch ((m_features.packet_ok (rs->buf, PACKET_Z1)).status ())
>      {
>      case PACKET_ERROR:
>      case PACKET_UNKNOWN:
> @@ -11372,7 +11379,7 @@ remote_target::verify_memory (const gdb_byte *data, CORE_ADDR lma, ULONGEST size
>  
>        getpkt (&rs->buf);
>  
> -      status = m_features.packet_ok (rs->buf, PACKET_qCRC);
> +      status = (m_features.packet_ok (rs->buf, PACKET_qCRC)).status ();
>        if (status == PACKET_ERROR)
>  	return -1;
>        else if (status == PACKET_OK)
> @@ -11494,7 +11501,7 @@ remote_target::remote_write_qxfer (const char *object_name,
>  
>    if (putpkt_binary (rs->buf.data (), i + buf_len) < 0
>        || getpkt (&rs->buf) < 0
> -      || m_features.packet_ok (rs->buf, which_packet) != PACKET_OK)
> +      || (m_features.packet_ok (rs->buf, which_packet)).status () != PACKET_OK)
>      return TARGET_XFER_E_IO;
>  
>    unpack_varlen_hex (rs->buf.data (), &n);
> @@ -11559,7 +11566,7 @@ remote_target::remote_read_qxfer (const char *object_name,
>    rs->buf[0] = '\0';
>    packet_len = getpkt (&rs->buf);
>    if (packet_len < 0
> -      || m_features.packet_ok (rs->buf, which_packet) != PACKET_OK)
> +      || (m_features.packet_ok (rs->buf, which_packet)).status () != PACKET_OK)
>      return TARGET_XFER_E_IO;
>  
>    if (rs->buf[0] != 'l' && rs->buf[0] != 'm')
> @@ -11864,7 +11871,8 @@ remote_target::search_memory (CORE_ADDR start_addr, ULONGEST search_space_len,
>  
>    if (putpkt_binary (rs->buf.data (), i + escaped_pattern_len) < 0
>        || getpkt (&rs->buf) < 0
> -      || m_features.packet_ok (rs->buf, PACKET_qSearch_memory) != PACKET_OK)
> +      || (m_features.packet_ok (rs->buf, PACKET_qSearch_memory)).status ()
> +      != PACKET_OK)
>      {
>        /* The request may not have worked because the command is not
>  	 supported.  If so, fall back to the simple way.  */
> @@ -12257,7 +12265,6 @@ remote_target::get_thread_local_address (ptid_t ptid, CORE_ADDR lm,
>        struct remote_state *rs = get_remote_state ();
>        char *p = rs->buf.data ();
>        char *endp = p + get_remote_packet_size ();
> -      enum packet_status result;
>  
>        strcpy (p, "qGetTLSAddr:");
>        p += strlen (p);
> @@ -12270,15 +12277,15 @@ remote_target::get_thread_local_address (ptid_t ptid, CORE_ADDR lm,
>  
>        putpkt (rs->buf);
>        getpkt (&rs->buf);
> -      result = m_features.packet_ok (rs->buf, PACKET_qGetTLSAddr);
> -      if (result == PACKET_OK)
> +      packet_result result = m_features.packet_ok (rs->buf, PACKET_qGetTLSAddr);
> +      if (result.status () == PACKET_OK)
>  	{
>  	  ULONGEST addr;
>  
>  	  unpack_varlen_hex (rs->buf.data (), &addr);
>  	  return addr;
>  	}
> -      else if (result == PACKET_UNKNOWN)
> +      else if (result.status () == PACKET_UNKNOWN)
>  	throw_error (TLS_GENERIC_ERROR,
>  		     _("Remote target doesn't support qGetTLSAddr packet"));
>        else
> @@ -12303,7 +12310,6 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
>        struct remote_state *rs = get_remote_state ();
>        char *p = rs->buf.data ();
>        char *endp = p + get_remote_packet_size ();
> -      enum packet_status result;
>  
>        strcpy (p, "qGetTIBAddr:");
>        p += strlen (p);
> @@ -12312,8 +12318,8 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
>  
>        putpkt (rs->buf);
>        getpkt (&rs->buf);
> -      result = m_features.packet_ok (rs->buf, PACKET_qGetTIBAddr);
> -      if (result == PACKET_OK)
> +      packet_result result = m_features.packet_ok (rs->buf, PACKET_qGetTIBAddr);
> +      if (result.status () == PACKET_OK)
>  	{
>  	  ULONGEST val;
>  	  unpack_varlen_hex (rs->buf.data (), &val);
> @@ -12321,7 +12327,7 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
>  	    *addr = (CORE_ADDR) val;
>  	  return true;
>  	}
> -      else if (result == PACKET_UNKNOWN)
> +      else if (result.status () == PACKET_UNKNOWN)
>  	error (_("Remote target doesn't support qGetTIBAddr packet"));
>        else
>  	error (_("Remote target failed to process qGetTIBAddr request"));

This seems like somewhere that we could make use of result.err_msg().
Not a hard requirement, but given the nature of this series I think this
would be appropriate.

> @@ -12580,7 +12586,7 @@ remote_target::remote_hostio_send_command (int command_bytes, int which_packet,
>        return -1;
>      }
>  
> -  switch (m_features.packet_ok (rs->buf, which_packet))
> +  switch ((m_features.packet_ok (rs->buf, which_packet)).status ())
>      {
>      case PACKET_ERROR:
>        *remote_errno = FILEIO_EINVAL;
> @@ -13868,7 +13874,6 @@ remote_target::get_trace_status (struct trace_status *ts)
>  {
>    /* Initialize it just to avoid a GCC false warning.  */
>    char *p = NULL;
> -  enum packet_status result;
>    struct remote_state *rs = get_remote_state ();
>  
>    if (m_features.packet_support (PACKET_qTStatus) == PACKET_DISABLE)
> @@ -13894,17 +13899,17 @@ remote_target::get_trace_status (struct trace_status *ts)
>        throw;
>      }
>  
> -  result = m_features.packet_ok (p, PACKET_qTStatus);
> +  packet_result result = m_features.packet_ok (p, PACKET_qTStatus);
>  
>    /* If the remote target doesn't do tracing, flag it.  */
> -  if (result == PACKET_UNKNOWN)
> +  if (result.status () == PACKET_UNKNOWN)
>      return -1;
>  
>    /* We're working with a live target.  */
>    ts->filename = NULL;
>  
>    if (*p++ != 'T')
> -    error (_("Bogus trace status reply from target: %s"), rs->buf.data ());
> +    error (_("Bogus trace status reply from target: %s"), result.err_msg ());

This is another place where we've not confirmed that we have a
PACKET_ERROR, it's just that the packet contents don't match the
"expected" format.

Maybe the original PACKET_UNKNOWN check should be changed into a switch
that can also check for PACKET_ERROR and error() with an appropriate
message?

But right here, when we find invalid packet contents, we have to print
'rs->buf.data ()'.

>  
>    /* Function 'parse_trace_status' sets default value of each field of
>       'ts' at first, so we don't have to do it here.  */
> @@ -14248,7 +14253,6 @@ remote_target::set_trace_buffer_size (LONGEST val)
>        struct remote_state *rs = get_remote_state ();
>        char *buf = rs->buf.data ();
>        char *endbuf = buf + get_remote_packet_size ();
> -      enum packet_status result;
>  
>        gdb_assert (val >= 0 || val == -1);
>        buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
> @@ -14263,10 +14267,10 @@ remote_target::set_trace_buffer_size (LONGEST val)
>  
>        putpkt (rs->buf);
>        remote_get_noisy_reply ();
> -      result = m_features.packet_ok (rs->buf, PACKET_QTBuffer_size);
> +      packet_result result = m_features.packet_ok (rs->buf, PACKET_QTBuffer_size);
>  
> -      if (result != PACKET_OK)
> -	warning (_("Bogus reply from target: %s"), rs->buf.data ());
> +      if (result.status () != PACKET_OK)
> +	warning (_("Bogus reply from target: %s"), result.err_msg ());

Again, we don't know this is a PACKET_ERROR so ::err_msg() might not be
valid.

Thanks,
Andrew