[v3] enum_flags to_string (was: Re: [PATCH v2 07/29] Thread options & clone events (core + remote))

  On 2022-10-28 11:26 a.m., Pedro Alves wrote:
> Some self review.  :-)
> 
> On 2022-10-27 8:55 p.m., Pedro Alves wrote:
> 
>>
>>  static std::string
>>  to_string (some_flags flags)
>>  {
>>  #define CASE(X) { X, #X }
>>    static constexpr test_flags::string_map_element mapping[] = {
>>      CASE (SOME_FLAG1),
>>      CASE (SOME_FLAG2),
>>      CASE (SOME_FLAG3),
>>    };
>>  #undef CASE
>>
>>    return flags.to_string (mapping);
>>  }
> 
> I was looking at this again, and noticed I named the macro "CASE".  That's just
> because that style of macro is typically used in a switch/case, my first approach
> used one.  That isn't what's being done in the end, so better rename it.  And while doing that,
> I realized that you don't even need to name the array or its type.  Like so:
> 
> static std::string
> to_string (some_flags flags)
> {
> #define MAP_ENUM_FLAG(X) { X, #X }
>   return flags.to_string ({
>     MAP_ENUM_FLAG (SOME_FLAG1),
>     MAP_ENUM_FLAG (SOME_FLAG3),
>   });
> #undef MAP_ENUM_FLAG
> }
> 
> But then, every enum_flags to_string implementation will end up defining/undefining that
> same macro, so might as well put it in gdbsupport/enum-flags.h, resulting in:
> 
> static std::string
> to_string (test_flags flags)
> {
>   return flags.to_string ({
>     MAP_ENUM_FLAG (FLAG1),
>     MAP_ENUM_FLAG (FLAG3),
>   });
> }

Bah, I now looked at the generated code at -O2, and with the unnamed array, the compiler
emits code to manually compose a temporary array on the stack at every call...

3rd time's a charm?

-- >8 --
From 0fe3edef69360c6795ab6811c4f3cba06159a50d Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Tue, 25 Oct 2022 15:39:37 +0100
Subject: [PATCH] enum_flags to_string

This commit introduces shared infrastructure that can be used to
implement enum_flags -> to_string functions.  With this, if we want to
support converting a given enum_flags specialization to string, we
just need to implement a function that provides the enumerator->string
mapping, like so:

 enum some_flag
   {
     SOME_FLAG1 = 1 << 0,
     SOME_FLAG2 = 1 << 1,
     SOME_FLAG3 = 1 << 2,
   };

 DEF_ENUM_FLAGS_TYPE (some_flag, some_flags);

 static std::string
 to_string (some_flags flags)
 {
   static constexpr some_flags::string_mapping mapping[] = {
     MAP_ENUM_FLAG (SOME_FLAG1),
     MAP_ENUM_FLAG (SOME_FLAG2),
     MAP_ENUM_FLAG (SOME_FLAG3),
   };
   return flags.to_string (mapping);
 }

.. and then to_string(SOME_FLAG2 | SOME_FLAG3) produces a string like
"0x6 [SOME_FLAG2 SOME_FLAG3]".

If we happen to forget to update the mapping array when we introduce a
new enumerator, then the string representation will pretty-print the
flags it knows about, and then the leftover flags in hex (one single
number).  For example, if we had missed mapping SOME_FLAG2 above, we'd
end up with:

  to_string(SOME_FLAG2 | SOME_FLAG3)  => "0x6 [SOME_FLAG2 0x4]");

Other than in the unit tests included, no actual usage of the
functionality is added in this commit.

Change-Id: I835de43c33d13bc0c95132f42c3f97318b875779
---
 gdb/unittests/enum-flags-selftests.c | 24 ++++++++++++
 gdbsupport/enum-flags.h              | 57 ++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)


base-commit: 508ccf9b3e1db355037a4a1c9004efe0d6d3ffbf
  

On 2022-10-28 07 h 08, Pedro Alves wrote:
> On 2022-10-28 11:26 a.m., Pedro Alves wrote:
>> Some self review.  :-)
>>
>> On 2022-10-27 8:55 p.m., Pedro Alves wrote:
>>
>>>
>>>  static std::string
>>>  to_string (some_flags flags)
>>>  {
>>>  #define CASE(X) { X, #X }
>>>    static constexpr test_flags::string_map_element mapping[] = {
>>>      CASE (SOME_FLAG1),
>>>      CASE (SOME_FLAG2),
>>>      CASE (SOME_FLAG3),
>>>    };
>>>  #undef CASE
>>>
>>>    return flags.to_string (mapping);
>>>  }
>>
>> I was looking at this again, and noticed I named the macro "CASE".  That's just
>> because that style of macro is typically used in a switch/case, my first approach
>> used one.  That isn't what's being done in the end, so better rename it.  And while doing that,
>> I realized that you don't even need to name the array or its type.  Like so:
>>
>> static std::string
>> to_string (some_flags flags)
>> {
>> #define MAP_ENUM_FLAG(X) { X, #X }
>>   return flags.to_string ({
>>     MAP_ENUM_FLAG (SOME_FLAG1),
>>     MAP_ENUM_FLAG (SOME_FLAG3),
>>   });
>> #undef MAP_ENUM_FLAG
>> }
>>
>> But then, every enum_flags to_string implementation will end up defining/undefining that
>> same macro, so might as well put it in gdbsupport/enum-flags.h, resulting in:
>>
>> static std::string
>> to_string (test_flags flags)
>> {
>>   return flags.to_string ({
>>     MAP_ENUM_FLAG (FLAG1),
>>     MAP_ENUM_FLAG (FLAG3),
>>   });
>> }
> 
> Bah, I now looked at the generated code at -O2, and with the unnamed array, the compiler
> emits code to manually compose a temporary array on the stack at every call...

Maybe because of this?

https://tristanbrindle.com/posts/beware-copies-initializer-list

> 
> 3rd time's a charm?
> 
> -- >8 --
> From 0fe3edef69360c6795ab6811c4f3cba06159a50d Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Tue, 25 Oct 2022 15:39:37 +0100
> Subject: [PATCH] enum_flags to_string
> 
> This commit introduces shared infrastructure that can be used to
> implement enum_flags -> to_string functions.  With this, if we want to
> support converting a given enum_flags specialization to string, we
> just need to implement a function that provides the enumerator->string
> mapping, like so:
> 
>  enum some_flag
>    {
>      SOME_FLAG1 = 1 << 0,
>      SOME_FLAG2 = 1 << 1,
>      SOME_FLAG3 = 1 << 2,
>    };
> 
>  DEF_ENUM_FLAGS_TYPE (some_flag, some_flags);
> 
>  static std::string
>  to_string (some_flags flags)
>  {
>    static constexpr some_flags::string_mapping mapping[] = {
>      MAP_ENUM_FLAG (SOME_FLAG1),
>      MAP_ENUM_FLAG (SOME_FLAG2),
>      MAP_ENUM_FLAG (SOME_FLAG3),
>    };
>    return flags.to_string (mapping);
>  }
> 
> .. and then to_string(SOME_FLAG2 | SOME_FLAG3) produces a string like
> "0x6 [SOME_FLAG2 SOME_FLAG3]".
> 
> If we happen to forget to update the mapping array when we introduce a
> new enumerator, then the string representation will pretty-print the
> flags it knows about, and then the leftover flags in hex (one single
> number).  For example, if we had missed mapping SOME_FLAG2 above, we'd
> end up with:
> 
>   to_string(SOME_FLAG2 | SOME_FLAG3)  => "0x6 [SOME_FLAG2 0x4]");
> 
> Other than in the unit tests included, no actual usage of the
> functionality is added in this commit.
> 
> Change-Id: I835de43c33d13bc0c95132f42c3f97318b875779
> ---
>  gdb/unittests/enum-flags-selftests.c | 24 ++++++++++++
>  gdbsupport/enum-flags.h              | 57 ++++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/gdb/unittests/enum-flags-selftests.c b/gdb/unittests/enum-flags-selftests.c
> index f52fc7220a1..ef43ba23cdf 100644
> --- a/gdb/unittests/enum-flags-selftests.c
> +++ b/gdb/unittests/enum-flags-selftests.c
> @@ -374,6 +374,20 @@ enum test_uflag : unsigned
>  DEF_ENUM_FLAGS_TYPE (test_flag, test_flags);
>  DEF_ENUM_FLAGS_TYPE (test_uflag, test_uflags);
>  
> +/* to_string enumerator->string mapping function used to test
> +   enum_flags::to_string.  This intentionally misses mapping one
> +   enumerator (FLAG2).  */
> +
> +static std::string
> +to_string (test_flags flags)
> +{
> +  static constexpr test_flags::string_mapping mapping[] = {
> +    MAP_ENUM_FLAG (FLAG1),
> +    MAP_ENUM_FLAG (FLAG3),
> +  };
> +  return flags.to_string (mapping);
> +}
> +
>  static void
>  self_test ()
>  {
> @@ -581,6 +595,16 @@ self_test ()
>  
>      SELF_CHECK (ok);
>    }
> +
> +  /* Check string conversion.  */
> +  {
> +    SELF_CHECK (to_string (0) == "0x0 []");
> +    SELF_CHECK (to_string (FLAG1) == "0x2 [FLAG1]");
> +    SELF_CHECK (to_string (FLAG1 | FLAG3) == "0xa [FLAG1 FLAG3]");
> +    SELF_CHECK (to_string (FLAG1 | FLAG2 | FLAG3) == "0xe [FLAG1 FLAG3 0x4]");
> +    SELF_CHECK (to_string (FLAG2) == "0x4 [0x4]");
> +    SELF_CHECK (to_string (test_flag (0xff)) == "0xff [FLAG1 FLAG3 0xf5]");

Ish, this crashes with:

/home/smarchi/src/binutils-gdb/gdb/../gdbsupport/enum-flags.h:191:12: runtime error: load of value 255, which is not a valid value for type 'test_flag'

with --enable-ubsan.  We can disable UBSan per function using
__attribute__((no_sanitize_undefined)) or
__attribute__((no_sanitize("undefined"))).  With clang, it appears
possible to push and pop function attributes, but it doesn't seem to be
possible with gcc.

I had a hard time identifying which functions needed it, so I slapped it
on all functions in enum-flags.h, and the test passed fine, so that's
good news.  Here's my change, to get you started:

From a2cac0cd6b85de82ce96d5b565f83d152482d403 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Fri, 28 Oct 2022 11:56:01 -0400
Subject: [PATCH] fix

Change-Id: I229d00382d8c0e7af0987f6720bab1ef4094290f
---
 gdbsupport/enum-flags.h | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
index 911f3fd7890..48b38dc88b5 100644
--- a/gdbsupport/enum-flags.h
+++ b/gdbsupport/enum-flags.h
@@ -158,16 +158,19 @@ class enum_flags
     : m_enum_value ((enum_type) 0)
   {}

+  __attribute__((no_sanitize_undefined))
   enum_flags &operator&= (enum_flags e) &
   {
     m_enum_value = (enum_type) (m_enum_value & e.m_enum_value);
     return *this;
   }
+  __attribute__((no_sanitize_undefined))
   enum_flags &operator|= (enum_flags e) &
   {
     m_enum_value = (enum_type) (m_enum_value | e.m_enum_value);
     return *this;
   }
+  __attribute__((no_sanitize_undefined))
   enum_flags &operator^= (enum_flags e) &
   {
     m_enum_value = (enum_type) (m_enum_value ^ e.m_enum_value);
@@ -180,12 +183,14 @@ class enum_flags
   void operator^= (enum_flags e) && = delete;

   /* Like raw enums, allow conversion to the underlying type.  */
+__attribute__((no_sanitize_undefined))
   constexpr operator underlying_type () const
   {
     return m_enum_value;
   }

   /* Get the underlying value as a raw enum.  */
+  __attribute__((no_sanitize_undefined))
   constexpr enum_type raw () const
   {
     return m_enum_value;
@@ -201,7 +206,7 @@ class enum_flags
      mapping array from callers.  */

   template<size_t N>
-  std::string
+  __attribute__((no_sanitize_undefined)) std::string
   to_string (const string_mapping (&mapping)[N]) const
   {
     enum_type flags = raw ();
@@ -259,6 +264,7 @@ using is_enum_flags_enum_type_t
   /* Raw enum on both LHS/RHS.  Returns raw enum type.  */		\
   template <typename enum_type,						\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  __attribute__((no_sanitize_undefined)) \
   constexpr enum_type							\
   OPERATOR_OP (enum_type e1, enum_type e2)				\
   {									\
@@ -269,6 +275,7 @@ using is_enum_flags_enum_type_t
   /* enum_flags on the LHS.  */						\
   template <typename enum_type,						\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  __attribute__((no_sanitize_undefined)) \
   constexpr enum_flags<enum_type>					\
   OPERATOR_OP (enum_flags<enum_type> e1, enum_type e2)			\
   { return e1.raw () OP e2; }						\
@@ -276,6 +283,7 @@ using is_enum_flags_enum_type_t
   /* enum_flags on the RHS.  */						\
   template <typename enum_type,						\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  __attribute__((no_sanitize_undefined)) \
   constexpr enum_flags<enum_type>					\
   OPERATOR_OP (enum_type e1, enum_flags<enum_type> e2)			\
   { return e1 OP e2.raw (); }						\
@@ -283,6 +291,7 @@ using is_enum_flags_enum_type_t
   /* enum_flags on both LHS/RHS.  */					\
   template <typename enum_type,						\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  __attribute__((no_sanitize_undefined)) \
   constexpr enum_flags<enum_type>					\
   OPERATOR_OP (enum_flags<enum_type> e1, enum_flags<enum_type> e2)	\
   { return e1.raw () OP e2.raw (); }					\
@@ -291,21 +300,25 @@ using is_enum_flags_enum_type_t
 									\
   template <typename enum_type, typename unrelated_type,		\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  __attribute__((no_sanitize_undefined)) \
   constexpr enum_flags<enum_type>					\
   OPERATOR_OP (enum_type e1, unrelated_type e2) = delete;		\
 									\
   template <typename enum_type, typename unrelated_type,		\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  __attribute__((no_sanitize_undefined)) \
   constexpr enum_flags<enum_type>					\
   OPERATOR_OP (unrelated_type e1, enum_type e2) = delete;		\
 									\
   template <typename enum_type, typename unrelated_type,		\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  __attribute__((no_sanitize_undefined)) \
   constexpr enum_flags<enum_type>					\
   OPERATOR_OP (enum_flags<enum_type> e1, unrelated_type e2) = delete;	\
 									\
   template <typename enum_type, typename unrelated_type,		\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  __attribute__((no_sanitize_undefined)) \
   constexpr enum_flags<enum_type>					\
   OPERATOR_OP (unrelated_type e1, enum_flags<enum_type> e2) = delete;

@@ -333,6 +346,7 @@ using is_enum_flags_enum_type_t
   /* lval reference version.  */					\
   template <typename enum_type,						\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  __attribute__((no_sanitize_undefined)) \
   constexpr enum_type &							\
   OPERATOR_OP (enum_type &e1, enum_type e2)				\
   { return e1 = e1 OP e2; }						\
@@ -377,6 +391,7 @@ ENUM_FLAGS_GEN_COMPOUND_ASSIGN (operator^=, ^)
   /* enum_flags OP enum_flags */					\
 									\
   template <typename enum_type>						\
+  __attribute__((no_sanitize_undefined)) \
   constexpr bool							\
   OPERATOR_OP (enum_flags<enum_type> lhs, enum_flags<enum_type> rhs)	\
   { return lhs.raw () OP rhs.raw (); }					\
@@ -384,32 +399,38 @@ ENUM_FLAGS_GEN_COMPOUND_ASSIGN (operator^=, ^)
   /* enum_flags OP other */						\
 									\
   template <typename enum_type>						\
+  __attribute__((no_sanitize_undefined)) \
   constexpr bool							\
   OPERATOR_OP (enum_flags<enum_type> lhs, enum_type rhs)		\
   { return lhs.raw () OP rhs; }						\
 									\
   template <typename enum_type>						\
+  __attribute__((no_sanitize_undefined)) \
   constexpr bool							\
   OPERATOR_OP (enum_flags<enum_type> lhs, int rhs)			\
   { return lhs.raw () OP rhs; }						\
 									\
   template <typename enum_type, typename U>				\
+  __attribute__((no_sanitize_undefined)) \
   constexpr bool							\
   OPERATOR_OP (enum_flags<enum_type> lhs, U rhs) = delete;		\
 									\
   /* other OP enum_flags */						\
 									\
   template <typename enum_type>						\
+  __attribute__((no_sanitize_undefined)) \
   constexpr bool							\
   OPERATOR_OP (enum_type lhs, enum_flags<enum_type> rhs)		\
   { return lhs OP rhs.raw (); }						\
 									\
   template <typename enum_type>						\
+  __attribute__((no_sanitize_undefined)) \
   constexpr bool							\
   OPERATOR_OP (int lhs, enum_flags<enum_type> rhs)			\
   { return lhs OP rhs.raw (); }						\
 									\
   template <typename enum_type, typename U>				\
+  __attribute__((no_sanitize_undefined)) \
   constexpr bool							\
   OPERATOR_OP (U lhs, enum_flags<enum_type> rhs) = delete;

@@ -426,6 +447,7 @@ template <typename enum_type,
 	  typename = is_enum_flags_enum_type_t<enum_type>,
 	  typename
 	    = gdb::Requires<enum_flags_detail::EnumIsUnsigned<enum_type>>>
+__attribute__((no_sanitize_undefined))
 constexpr enum_type
 operator~ (enum_type e)
 {
@@ -442,6 +464,7 @@ template <typename enum_type,
 	  typename = is_enum_flags_enum_type_t<enum_type>,
 	  typename
 	    = gdb::Requires<enum_flags_detail::EnumIsUnsigned<enum_type>>>
+__attribute__((no_sanitize_undefined))
 constexpr enum_flags<enum_type>
 operator~ (enum_flags<enum_type> e)
 {
  

On 2022-10-28 4:59 p.m., Simon Marchi wrote:
> On 2022-10-28 07 h 08, Pedro Alves wrote:
>> On 2022-10-28 11:26 a.m., Pedro Alves wrote:
...
>> +  /* Check string conversion.  */
>> +  {
>> +    SELF_CHECK (to_string (0) == "0x0 []");
>> +    SELF_CHECK (to_string (FLAG1) == "0x2 [FLAG1]");
>> +    SELF_CHECK (to_string (FLAG1 | FLAG3) == "0xa [FLAG1 FLAG3]");
>> +    SELF_CHECK (to_string (FLAG1 | FLAG2 | FLAG3) == "0xe [FLAG1 FLAG3 0x4]");
>> +    SELF_CHECK (to_string (FLAG2) == "0x4 [0x4]");
>> +    SELF_CHECK (to_string (test_flag (0xff)) == "0xff [FLAG1 FLAG3 0xf5]");
> 
> Ish, this crashes with:
> 
> /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/enum-flags.h:191:12: runtime error: load of value 255, which is not a valid value for type 'test_flag'

Ah.  That's because I missed that test_flag doesn't have an explicit underlying
type, as in "enum test_flag : underlying".  The rules are different if there's
an explicit underlying type or not.  enums with an explicit type can hold any value
of the underlying type.  Best to fix the testcase.  We could either remove
that particular test, which is not really that important, as what it tests is
exercised by the previous two tests, or switch to test_uflags instead, which
does have an explicit underlying type.  I did the latter.

> 
> with --enable-ubsan.  We can disable UBSan per function using
> __attribute__((no_sanitize_undefined)) or
> __attribute__((no_sanitize("undefined"))).  With clang, it appears
> possible to push and pop function attributes, but it doesn't seem to be
> possible with gcc.
> 
> I had a hard time identifying which functions needed it, so I slapped it
> on all functions in enum-flags.h, and the test passed fine, so that's
> good news.  Here's my change, to get you started:

I'd rather fix the test.  Here's another version of the patch.

Nth time a charm.  :-)

-- >8 --
From cc536203a1898e04dfa96cb56df10cbf7f1fa63d Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Tue, 25 Oct 2022 15:39:37 +0100
Subject: [PATCH v4] enum_flags to_string

This commit introduces shared infrastructure that can be used to
implement enum_flags -> to_string functions.  With this, if we want to
support converting a given enum_flags specialization to string, we
just need to implement a function that provides the enumerator->string
mapping, like so:

 enum some_flag
   {
     SOME_FLAG1 = 1 << 0,
     SOME_FLAG2 = 1 << 1,
     SOME_FLAG3 = 1 << 2,
   };

 DEF_ENUM_FLAGS_TYPE (some_flag, some_flags);

 static std::string
 to_string (some_flags flags)
 {
   static constexpr some_flags::string_mapping mapping[] = {
     MAP_ENUM_FLAG (SOME_FLAG1),
     MAP_ENUM_FLAG (SOME_FLAG2),
     MAP_ENUM_FLAG (SOME_FLAG3),
   };
   return flags.to_string (mapping);
 }

.. and then to_string(SOME_FLAG2 | SOME_FLAG3) produces a string like
"0x6 [SOME_FLAG2 SOME_FLAG3]".

If we happen to forget to update the mapping array when we introduce a
new enumerator, then the string representation will pretty-print the
flags it knows about, and then the leftover flags in hex (one single
number).  For example, if we had missed mapping SOME_FLAG2 above, we'd
end up with:

  to_string(SOME_FLAG2 | SOME_FLAG3)  => "0x6 [SOME_FLAG2 0x4]");

Other than in the unit tests included, no actual usage of the
functionality is added in this commit.

Change-Id: I835de43c33d13bc0c95132f42c3f97318b875779
---
 gdb/unittests/enum-flags-selftests.c | 24 ++++++++++++
 gdbsupport/enum-flags.h              | 57 ++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/gdb/unittests/enum-flags-selftests.c b/gdb/unittests/enum-flags-selftests.c
index f52fc7220a1..b427a99a8f5 100644
--- a/gdb/unittests/enum-flags-selftests.c
+++ b/gdb/unittests/enum-flags-selftests.c
@@ -374,6 +374,20 @@ enum test_uflag : unsigned
 DEF_ENUM_FLAGS_TYPE (test_flag, test_flags);
 DEF_ENUM_FLAGS_TYPE (test_uflag, test_uflags);
 
+/* to_string enumerator->string mapping function used to test
+   enum_flags::to_string.  This intentionally misses mapping one
+   enumerator (FLAG2).  */
+
+static std::string
+to_string (test_uflags flags)
+{
+  static constexpr test_uflags::string_mapping mapping[] = {
+    MAP_ENUM_FLAG (UFLAG1),
+    MAP_ENUM_FLAG (UFLAG3),
+  };
+  return flags.to_string (mapping);
+}
+
 static void
 self_test ()
 {
@@ -581,6 +595,16 @@ self_test ()
 
     SELF_CHECK (ok);
   }
+
+  /* Check string conversion.  */
+  {
+    SELF_CHECK (to_string (0) == "0x0 []");
+    SELF_CHECK (to_string (UFLAG1) == "0x2 [UFLAG1]");
+    SELF_CHECK (to_string (UFLAG1 | UFLAG3) == "0xa [UFLAG1 UFLAG3]");
+    SELF_CHECK (to_string (UFLAG1 | UFLAG2 | UFLAG3) == "0xe [UFLAG1 UFLAG3 0x4]");
+    SELF_CHECK (to_string (UFLAG2) == "0x4 [0x4]");
+    SELF_CHECK (to_string (test_uflag (0xff)) == "0xff [UFLAG1 UFLAG3 0xf5]");
+  }
 }
 
 } /* namespace enum_flags_tests */
diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
index cd500f55ff3..911f3fd7890 100644
--- a/gdbsupport/enum-flags.h
+++ b/gdbsupport/enum-flags.h
@@ -130,6 +130,17 @@ class enum_flags
   typedef E enum_type;
   typedef typename enum_underlying_type<enum_type>::type underlying_type;
 
+  /* For to_string.  Maps one enumerator of E to a string.  */
+  struct string_mapping
+  {
+    E flag;
+    const char *str;
+  };
+
+  /* Convenience for to_string implementations, to build a
+     string_mapping array.  */
+#define MAP_ENUM_FLAG(ENUM_FLAG) { ENUM_FLAG, #ENUM_FLAG }
+
 public:
   /* Allow default construction.  */
   constexpr enum_flags ()
@@ -183,6 +194,52 @@ class enum_flags
   /* Binary operations involving some unrelated type (which would be a
      bug) are implemented as non-members, and deleted.  */
 
+  /* Convert this object to a std::string, using MAPPING as
+     enumerator-to-string mapping array.  This is not meant to be
+     called directly.  Instead, enum_flags specializations should have
+     their own to_string function wrapping this one, thus hidding the
+     mapping array from callers.  */
+
+  template<size_t N>
+  std::string
+  to_string (const string_mapping (&mapping)[N]) const
+  {
+    enum_type flags = raw ();
+    std::string res = hex_string (flags);
+    res += " [";
+
+    bool need_space = false;
+    for (const auto &entry : mapping)
+      {
+	if ((flags & entry.flag) != 0)
+	  {
+	    /* Do op~ in the underlying type, because if enum_type's
+	       underlying type is signed, op~ won't be defined for
+	       it.  */
+	    flags &= (enum_type) ~(underlying_type) entry.flag;
+
+	    if (need_space)
+	      res += " ";
+	    res += entry.str;
+
+	    need_space = true;
+	  }
+      }
+
+    /* If there were flags not included in the mapping, print them as
+       a hex number.  */
+    if (flags != 0)
+      {
+	if (need_space)
+	  res += " ";
+	res += hex_string (flags);
+      }
+
+    res += "]";
+
+    return res;
+  }
+
 private:
   /* Stored as enum_type because GDB knows to print the bit flags
      neatly if the enum values look like bit flags.  */

base-commit: 508ccf9b3e1db355037a4a1c9004efe0d6d3ffbf
  

On 10/28/22 14:23, Pedro Alves wrote:
> On 2022-10-28 4:59 p.m., Simon Marchi wrote:
>> On 2022-10-28 07 h 08, Pedro Alves wrote:
>>> On 2022-10-28 11:26 a.m., Pedro Alves wrote:
> ...
>>> +  /* Check string conversion.  */
>>> +  {
>>> +    SELF_CHECK (to_string (0) == "0x0 []");
>>> +    SELF_CHECK (to_string (FLAG1) == "0x2 [FLAG1]");
>>> +    SELF_CHECK (to_string (FLAG1 | FLAG3) == "0xa [FLAG1 FLAG3]");
>>> +    SELF_CHECK (to_string (FLAG1 | FLAG2 | FLAG3) == "0xe [FLAG1 FLAG3 0x4]");
>>> +    SELF_CHECK (to_string (FLAG2) == "0x4 [0x4]");
>>> +    SELF_CHECK (to_string (test_flag (0xff)) == "0xff [FLAG1 FLAG3 0xf5]");
>>
>> Ish, this crashes with:
>>
>> /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/enum-flags.h:191:12: runtime error: load of value 255, which is not a valid value for type 'test_flag'
> 
> Ah.  That's because I missed that test_flag doesn't have an explicit underlying
> type, as in "enum test_flag : underlying".  The rules are different if there's
> an explicit underlying type or not.  enums with an explicit type can hold any value
> of the underlying type.  Best to fix the testcase.  We could either remove
> that particular test, which is not really that important, as what it tests is
> exercised by the previous two tests, or switch to test_uflags instead, which
> does have an explicit underlying type.  I did the latter.

Ok, didn't know that.  This is fine with me.

> @@ -183,6 +194,52 @@ class enum_flags
>    /* Binary operations involving some unrelated type (which would be a
>       bug) are implemented as non-members, and deleted.  */
>  
> +  /* Convert this object to a std::string, using MAPPING as
> +     enumerator-to-string mapping array.  This is not meant to be
> +     called directly.  Instead, enum_flags specializations should have
> +     their own to_string function wrapping this one, thus hidding the
> +     mapping array from callers.  */
> +
> +  template<size_t N>
> +  std::string
> +  to_string (const string_mapping (&mapping)[N]) const
> +  {
> +    enum_type flags = raw ();
> +    std::string res = hex_string (flags);
> +    res += " [";
> +
> +    bool need_space = false;
> +    for (const auto &entry : mapping)
> +      {
> +	if ((flags & entry.flag) != 0)
> +	  {
> +	    /* Do op~ in the underlying type, because if enum_type's
> +	       underlying type is signed, op~ won't be defined for
> +	       it.  */
> +	    flags &= (enum_type) ~(underlying_type) entry.flag;

This line gives me (GCC 12):

  CXX    unittests/enum-flags-selftests.o
In file included from /home/simark/src/binutils-gdb/gdb/defs.h:65,
                 from /home/simark/src/binutils-gdb/gdb/unittests/enum-flags-selftests.c:20:
/home/simark/src/binutils-gdb/gdb/../gdbsupport/enum-flags.h: In instantiation of ‘std::string enum_flags<E>::to_string(const string_mapping (&)[N]) const [with long unsigned int N = 2; E = selftests::enum_flags_tests::test_uflag; std::string = std::__cxx11::basic_string<char>]’:
/home/simark/src/binutils-gdb/gdb/unittests/enum-flags-selftests.c:388:26:   required from here
/home/simark/src/binutils-gdb/gdb/../gdbsupport/enum-flags.h:219:19: error: invalid conversion from ‘unsigned int’ to ‘enum_flags<selftests::enum_flags_tests::test_uflag>::enum_type’ {aka ‘selftests::enum_flags_tests::test_uflag’} [-fpermissive]
  219 |             flags &= (enum_type) ~(underlying_type) entry.flag;
      |             ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                   |
      |                   unsigned int

This builds though:

   flags = (enum_type) (flags & (enum_type) (~(underlying_type) entry.flag));

I'm not sure why, the operator & between an enum_type and an enum_type
is supposed to return an enum_type already.

Simon