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

[Pedro Alves <pedro@palves.net>]

On 2022-10-31 12:47 p.m., Simon Marchi wrote:
> On 10/28/22 14:23, Pedro Alves wrote:

...

>> @@ -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.
> 

Man...  The problem is that enum_flags::to_string is defined inside the template
class, before the global operators for enum_type are defined, so the to_string
method is being compiled as if the global operators don't exist.  I guess newer
GCC is more stringent than the GCC 9.4 from Ubuntu 20.04, which is what I was
using.  The fix is then to move the method outside the class, after the operators.

Here's a new version...

I'm now also doing the op~ using an unsigned type, in case ubsan complains
or something.  

And that made me realize, that only testing with the unsigned enum completely bypasses
the need for these casts in the first place, so better re-add testing with the signed
enum.  Better yet, test both signed and unsigned.

I also tweaked the testcase to no longer need "test_flag (0xff)" -- I remembered what
was the original reason I had added that -- it was to check whether multiple
leftover unmapped flags end up printed with a single hex.

I also changed the numbers behind FLAG1, FLAG2, etc., to make it easier to
come up with the numbers for the tests.

Tested GCC 9, 13 (some self-built trunk version from a few months back), clang 10 and clang 13.

-- >8 --
From 9ab8b34f50dd219d817d333d52a3d8430cff9db2 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 | 69 +++++++++++++++++++++++++---
 gdbsupport/enum-flags.h              | 66 ++++++++++++++++++++++++++
 2 files changed, 129 insertions(+), 6 deletions(-)

diff --git a/gdb/unittests/enum-flags-selftests.c b/gdb/unittests/enum-flags-selftests.c
index f52fc7220a1..0fd35262469 100644
--- a/gdb/unittests/enum-flags-selftests.c
+++ b/gdb/unittests/enum-flags-selftests.c
@@ -359,21 +359,47 @@ CHECK_VALID (true,  bool, NF (1) == char (1))
 
 enum test_flag
   {
-    FLAG1 = 1 << 1,
-    FLAG2 = 1 << 2,
-    FLAG3 = 1 << 3,
+    FLAG1 = 1 << 0,
+    FLAG2 = 1 << 1,
+    FLAG3 = 1 << 2,
+    FLAG4 = 1 << 3,
   };
 
 enum test_uflag : unsigned
   {
-    UFLAG1 = 1 << 1,
-    UFLAG2 = 1 << 2,
-    UFLAG3 = 1 << 3,
+    UFLAG1 = 1 << 0,
+    UFLAG2 = 1 << 1,
+    UFLAG3 = 1 << 2,
+    UFLAG4 = 1 << 3,
   };
 
 DEF_ENUM_FLAGS_TYPE (test_flag, test_flags);
 DEF_ENUM_FLAGS_TYPE (test_uflag, test_uflags);
 
+/* to_string enumerator->string mapping functions used to test
+   enum_flags::to_string.  These intentionally miss mapping a couple
+   enumerators each (xFLAG2, xFLAG4).  */
+
+static std::string
+to_string_flags (test_flags flags)
+{
+  static constexpr test_flags::string_mapping mapping[] = {
+    MAP_ENUM_FLAG (FLAG1),
+    MAP_ENUM_FLAG (FLAG3),
+  };
+  return flags.to_string (mapping);
+}
+
+static std::string
+to_string_uflags (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 +607,37 @@ self_test ()
 
     SELF_CHECK (ok);
   }
+
+  /* Check string conversion.  */
+  {
+    SELF_CHECK (to_string_uflags (0)
+		== "0x0 []");
+    SELF_CHECK (to_string_uflags (UFLAG1)
+		== "0x1 [UFLAG1]");
+    SELF_CHECK (to_string_uflags (UFLAG1 | UFLAG3)
+		== "0x5 [UFLAG1 UFLAG3]");
+    SELF_CHECK (to_string_uflags (UFLAG1 | UFLAG2 | UFLAG3)
+		== "0x7 [UFLAG1 UFLAG3 0x2]");
+    SELF_CHECK (to_string_uflags (UFLAG2)
+		== "0x2 [0x2]");
+    /* Check that even with multiple unmapped flags, we only print one
+       unmapped hex number (0xa, in this case).  */
+    SELF_CHECK (to_string_uflags (UFLAG1 | UFLAG2 | UFLAG3 | UFLAG4)
+		== "0xf [UFLAG1 UFLAG3 0xa]");
+
+    SELF_CHECK (to_string_flags (0)
+		== "0x0 []");
+    SELF_CHECK (to_string_flags (FLAG1)
+		== "0x1 [FLAG1]");
+    SELF_CHECK (to_string_flags (FLAG1 | FLAG3)
+		== "0x5 [FLAG1 FLAG3]");
+    SELF_CHECK (to_string_flags (FLAG1 | FLAG2 | FLAG3)
+		== "0x7 [FLAG1 FLAG3 0x2]");
+    SELF_CHECK (to_string_flags (FLAG2)
+		== "0x2 [0x2]");
+    SELF_CHECK (to_string_flags (FLAG1 | FLAG2 | FLAG3 | FLAG4)
+		== "0xf [FLAG1 FLAG3 0xa]");
+  }
 }
 
 } /* namespace enum_flags_tests */
diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
index cd500f55ff3..a8f2bfd7a5c 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,18 @@ 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.
+
+     Note: this is defined outside the template class so it can use
+     the global operators for enum_type, which are only defined after
+     the template class.  */
+  template<size_t N>
+  std::string to_string (const string_mapping (&mapping)[N]) const;
+
 private:
   /* Stored as enum_type because GDB knows to print the bit flags
      neatly if the enum values look like bit flags.  */
@@ -424,4 +447,47 @@ void operator>> (const enum_flags<enum_type> &, const any_type &) = delete;
 
 #endif /* __cplusplus */
 
+template<typename E>
+template<size_t N>
+std::string
+enum_flags<E>::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)
+	{
+	  /* Work with an unsigned version of the underlying type,
+	     because if enum_type's underlying type is signed, op~
+	     won't be defined for it, and, bitwise operatons on signed
+	     types are implementation defined.  */
+	  using uns = typename std::make_unsigned<underlying_type>::type;
+	  flags &= (enum_type) ~(uns) 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;
+}
+
 #endif /* COMMON_ENUM_FLAGS_H */

base-commit: b7096df235486ce718c2a0bfda8a0db074dcb671
-- 
2.36.0