Re: [PATCH 2/3] gdb: introduce target_waitkind_str, use it in target_waitstatus::to_string

[Andrew Burgess <aburgess@redhat.com>]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-11-22 11:27:30 -0500]:

> I would like to print target_waitkind values in debug messages, so I
> think that a target_waitkind-to-string function would be useful.  While
> at it, use it in target_waitstatus::to_string.  This changes the output
> of target_waitstatus::to_string a bit, but I think it is for the better.
> The debug messages will show a string matching exactly the
> target_waitkind enumerator (minus the TARGET_WAITKIND prefix).
> 
> As a convenience, make string_appendf return the same reference to
> string it got as a parameter.  This allows doing this:
> 
>   return string_appendf (str, "foo");
> 
> ... keeping the code concise.

LGTM.

Thanks,
Andrew

> 
> Change-Id: I383dffc9c78614e7d0668b1516073905e798eef7
> ---
>  gdb/target/waitstatus.c                | 66 +++++++++-----------------
>  gdb/target/waitstatus.h                | 49 +++++++++++++++++++
>  gdb/unittests/common-utils-selftests.c |  7 ++-
>  gdbsupport/common-utils.cc             |  8 +++-
>  gdbsupport/common-utils.h              |  4 +-
>  5 files changed, 84 insertions(+), 50 deletions(-)
> 
> diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
> index 2293d83230d..a7209e3f2b7 100644
> --- a/gdb/target/waitstatus.c
> +++ b/gdb/target/waitstatus.c
> @@ -20,73 +20,51 @@
>  #include "gdbsupport/common-defs.h"
>  #include "waitstatus.h"
>  
> -/* Return a pretty printed form of target_waitstatus.  */
> +/* See waitstatus.h.  */
>  
>  std::string
>  target_waitstatus::to_string () const
>  {
> -  const char *kind_str = "status->kind = ";
> +  std::string str = string_printf
> +    ("status->kind = %s", target_waitkind_str (this->kind ()));
>  
> +/* Make sure the compiler warns if a new TARGET_WAITKIND enumerator is added
> +   but not handled here.  */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic error "-Wswitch"
>    switch (this->kind ())
>      {
>      case TARGET_WAITKIND_EXITED:
> -      return string_printf ("%sexited, status = %d",
> -			    kind_str, this->exit_status ());
> +    case TARGET_WAITKIND_THREAD_EXITED:
> +      return string_appendf (str, ", exit_status = %d", this->exit_status ());
>  
>      case TARGET_WAITKIND_STOPPED:
> -      return string_printf ("%sstopped, signal = %s",
> -			    kind_str,
> -			    gdb_signal_to_symbol_string (this->sig ()));
> -
>      case TARGET_WAITKIND_SIGNALLED:
> -      return string_printf ("%ssignalled, signal = %s",
> -			    kind_str,
> -			    gdb_signal_to_symbol_string (this->sig ()));
> -
> -    case TARGET_WAITKIND_LOADED:
> -      return string_printf ("%sloaded", kind_str);
> +      return string_appendf (str, ", sig = %s",
> +			     gdb_signal_to_symbol_string (this->sig ()));
>  
>      case TARGET_WAITKIND_FORKED:
> -      return string_printf ("%sforked, child_ptid = %s", kind_str,
> -			    this->child_ptid ().to_string ().c_str ());
> -
>      case TARGET_WAITKIND_VFORKED:
> -      return string_printf ("%svforked, child_ptid = %s", kind_str,
> -			    this->child_ptid ().to_string ().c_str ());
> +      return string_appendf (str, ", child_ptid = %s",
> +			     this->child_ptid ().to_string ().c_str ());
>  
>      case TARGET_WAITKIND_EXECD:
> -      return string_printf ("%sexecd, execd_pathname = %s", kind_str,
> -			    this->execd_pathname ());
> +      return string_appendf (str, ", execd_pathname = %s",
> +			     this->execd_pathname ());
>  
> +    case TARGET_WAITKIND_LOADED:
>      case TARGET_WAITKIND_VFORK_DONE:
> -      return string_printf ("%svfork-done", kind_str);
> -
> +    case TARGET_WAITKIND_SPURIOUS:
>      case TARGET_WAITKIND_SYSCALL_ENTRY:
> -      return string_printf ("%sentered syscall", kind_str);
> -
>      case TARGET_WAITKIND_SYSCALL_RETURN:
> -      return string_printf ("%sexited syscall", kind_str);
> -
> -    case TARGET_WAITKIND_SPURIOUS:
> -      return string_printf ("%sspurious", kind_str);
> -
>      case TARGET_WAITKIND_IGNORE:
> -      return string_printf ("%signore", kind_str);
> -
>      case TARGET_WAITKIND_NO_HISTORY:
> -      return string_printf ("%sno-history", kind_str);
> -
>      case TARGET_WAITKIND_NO_RESUMED:
> -      return string_printf ("%sno-resumed", kind_str);
> -
>      case TARGET_WAITKIND_THREAD_CREATED:
> -      return string_printf ("%sthread created", kind_str);
> -
> -    case TARGET_WAITKIND_THREAD_EXITED:
> -      return string_printf ("%sthread exited, status = %d",
> -			    kind_str, this->exit_status ());
> -
> -    default:
> -      return string_printf ("%sunknown???", kind_str);
> +      return str;
>      }
> +#pragma GCC diagnostic pop
> +
> +  gdb_assert_not_reached ("invalid target_waitkind value: %d",
> +			  (int) this->kind ());
>  }
> diff --git a/gdb/target/waitstatus.h b/gdb/target/waitstatus.h
> index f5b050b8b82..48405d222f4 100644
> --- a/gdb/target/waitstatus.h
> +++ b/gdb/target/waitstatus.h
> @@ -101,6 +101,55 @@ enum target_waitkind
>    TARGET_WAITKIND_THREAD_EXITED,
>  };
>  
> +/* Return KIND as a string.  */
> +
> +static inline const char *
> +target_waitkind_str (target_waitkind kind)
> +{
> +/* Make sure the compiler warns if a new TARGET_WAITKIND enumerator is added
> +   but not handled here.  */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic error "-Wswitch"
> +  switch (kind)
> +  {
> +    case TARGET_WAITKIND_EXITED:
> +      return "EXITED";
> +    case TARGET_WAITKIND_STOPPED:
> +      return "STOPPED";
> +    case TARGET_WAITKIND_SIGNALLED:
> +      return "SIGNALLED";
> +    case TARGET_WAITKIND_LOADED:
> +      return "LOADED";
> +    case TARGET_WAITKIND_FORKED:
> +      return "FORKED";
> +    case TARGET_WAITKIND_VFORKED:
> +      return "VFORKED";
> +    case TARGET_WAITKIND_EXECD:
> +      return "EXECD";
> +    case TARGET_WAITKIND_VFORK_DONE:
> +      return "VFORK_DONE";
> +    case TARGET_WAITKIND_SYSCALL_ENTRY:
> +      return "SYSCALL_ENTRY";
> +    case TARGET_WAITKIND_SYSCALL_RETURN:
> +      return "SYSCALL_RETURN";
> +    case TARGET_WAITKIND_SPURIOUS:
> +      return "SPURIOUS";
> +    case TARGET_WAITKIND_IGNORE:
> +      return "IGNORE";
> +    case TARGET_WAITKIND_NO_HISTORY:
> +      return "NO_HISTORY";
> +    case TARGET_WAITKIND_NO_RESUMED:
> +      return "NO_RESUMED";
> +    case TARGET_WAITKIND_THREAD_CREATED:
> +      return "THREAD_CREATED";
> +    case TARGET_WAITKIND_THREAD_EXITED:
> +      return "THREAD_EXITED";
> +  };
> +#pragma GCC diagnostic pop
> +
> +  gdb_assert_not_reached ("invalid target_waitkind value: %d\n", (int) kind);
> +}
> +
>  struct target_waitstatus
>  {
>    /* Default constructor.  */
> diff --git a/gdb/unittests/common-utils-selftests.c b/gdb/unittests/common-utils-selftests.c
> index 26d313fd329..aa0f45241b9 100644
> --- a/gdb/unittests/common-utils-selftests.c
> +++ b/gdb/unittests/common-utils-selftests.c
> @@ -80,7 +80,8 @@ string_vprintf_tests ()
>  /* Type of both 'string_appendf' and the 'string_vappendf_wrapper'
>     function below.  Used to run the same tests against both
>     string_appendf and string_vappendf.  */
> -typedef void (string_appendf_func) (std::string &str, const char *fmt, ...)
> +typedef std::string &(string_appendf_func) (std::string &str, const char *fmt,
> +					    ...)
>    ATTRIBUTE_PRINTF (2, 3);
>  
>  static void
> @@ -101,7 +102,7 @@ test_appendf_func (string_appendf_func *func)
>    SELF_CHECK (str == "test23foo 45 bar");
>  }
>  
> -static void ATTRIBUTE_PRINTF (2, 3)
> +static std::string & ATTRIBUTE_PRINTF (2, 3)
>  string_vappendf_wrapper (std::string &str, const char *fmt, ...)
>  {
>    va_list vp;
> @@ -109,6 +110,8 @@ string_vappendf_wrapper (std::string &str, const char *fmt, ...)
>    va_start (vp, fmt);
>    string_vappendf (str, fmt, vp);
>    va_end (vp);
> +
> +  return str;
>  }
>  
>  static void
> diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc
> index 42bce36e535..b591537e62e 100644
> --- a/gdbsupport/common-utils.cc
> +++ b/gdbsupport/common-utils.cc
> @@ -119,7 +119,7 @@ string_vprintf (const char* fmt, va_list args)
>  
>  /* See documentation in common-utils.h.  */
>  
> -void
> +std::string &
>  string_appendf (std::string &str, const char *fmt, ...)
>  {
>    va_list vp;
> @@ -127,12 +127,14 @@ string_appendf (std::string &str, const char *fmt, ...)
>    va_start (vp, fmt);
>    string_vappendf (str, fmt, vp);
>    va_end (vp);
> +
> +  return str;
>  }
>  
>  
>  /* See documentation in common-utils.h.  */
>  
> -void
> +std::string &
>  string_vappendf (std::string &str, const char *fmt, va_list args)
>  {
>    va_list vp;
> @@ -148,6 +150,8 @@ string_vappendf (std::string &str, const char *fmt, va_list args)
>    /* C++11 and later guarantee std::string uses contiguous memory and
>       always includes the terminating '\0'.  */
>    vsprintf (&str[curr_size], fmt, args); /* ARI: vsprintf */
> +
> +  return str;
>  }
>  
>  char *
> diff --git a/gdbsupport/common-utils.h b/gdbsupport/common-utils.h
> index af078475780..98a9dc72f36 100644
> --- a/gdbsupport/common-utils.h
> +++ b/gdbsupport/common-utils.h
> @@ -54,11 +54,11 @@ std::string string_vprintf (const char* fmt, va_list args)
>  
>  /* Like string_printf, but appends to DEST instead of returning a new
>     std::string.  */
> -void string_appendf (std::string &dest, const char* fmt, ...)
> +std::string &string_appendf (std::string &dest, const char* fmt, ...)
>    ATTRIBUTE_PRINTF (2, 3);
>  
>  /* Like string_appendf, but takes a va_list.  */
> -void string_vappendf (std::string &dest, const char* fmt, va_list args)
> +std::string &string_vappendf (std::string &dest, const char* fmt, va_list args)
>    ATTRIBUTE_PRINTF (2, 0);
>  
>  /* Make a copy of the string at PTR with LEN characters
> -- 
> 2.33.1
>