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

[Simon Marchi <simark@simark.ca>]

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)
 {
-- 
2.37.3



> +  }
>  }
>  
>  } /* 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

Just wondering, any reason why you chose a template rather than, say,
gdb::array_view<const string_mapping>?

Once the undefined behavior thing is fixed, this patch LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon