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

[Pedro Alves <pedro@palves.net>]

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
-- 
2.36.0