From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 4EB43382EA04 for ; Fri, 28 Oct 2022 15:59:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4EB43382EA04 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [10.0.235.143] (modemcable075.250-20-96.mc.videotron.ca [96.20.250.75]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 83E571E0CB; Fri, 28 Oct 2022 11:59:25 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1666972765; bh=D2VKfJbYybwe/kFa4yGP8WOiiaUeyR27wyE9yLcGlow=; h=Date:Subject:To:References:From:In-Reply-To:From; b=sfSSOsC0hyRhdtgWjlURPOEdw0AqCAIUgTCRAHyrop2mutbCPL8SoK6lK1N9+5Ra4 w5OAbhMmJ4jDr5bND/xu0tFrF6zS5YFbXb9b+ghEGkx6r/E6l7Uj4tI70eQr8SOhII JawslPHzVB08B+tb69X0+JE+qkQsSDUoVrqhffTg= Message-ID: <6aee3c00-b334-97e2-62f7-0434822bcf7a@simark.ca> Date: Fri, 28 Oct 2022 11:59:25 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Subject: Re: [PATCH v3] enum_flags to_string (was: Re: [PATCH v2 07/29] Thread options & clone events (core + remote)) To: Pedro Alves , gdb-patches@sourceware.org References: <20220713222433.374898-1-pedro@palves.net> <20220713222433.374898-8-pedro@palves.net> <5d68cd36-e8d6-e8ad-5428-863e79742062@simark.ca> <6b91f8f6-d3b5-c44b-297c-ce1c3a1c80f6@palves.net> Content-Language: fr From: Simon Marchi In-Reply-To: <6b91f8f6-d3b5-c44b-297c-ce1c3a1c80f6@palves.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 > 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 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 - 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 > \ + __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 > \ + __attribute__((no_sanitize_undefined)) \ constexpr enum_flags \ OPERATOR_OP (enum_flags 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 > \ + __attribute__((no_sanitize_undefined)) \ constexpr enum_flags \ OPERATOR_OP (enum_type e1, enum_flags e2) \ { return e1 OP e2.raw (); } \ @@ -283,6 +291,7 @@ using is_enum_flags_enum_type_t /* enum_flags on both LHS/RHS. */ \ template > \ + __attribute__((no_sanitize_undefined)) \ constexpr enum_flags \ OPERATOR_OP (enum_flags e1, enum_flags e2) \ { return e1.raw () OP e2.raw (); } \ @@ -291,21 +300,25 @@ using is_enum_flags_enum_type_t \ template > \ + __attribute__((no_sanitize_undefined)) \ constexpr enum_flags \ OPERATOR_OP (enum_type e1, unrelated_type e2) = delete; \ \ template > \ + __attribute__((no_sanitize_undefined)) \ constexpr enum_flags \ OPERATOR_OP (unrelated_type e1, enum_type e2) = delete; \ \ template > \ + __attribute__((no_sanitize_undefined)) \ constexpr enum_flags \ OPERATOR_OP (enum_flags e1, unrelated_type e2) = delete; \ \ template > \ + __attribute__((no_sanitize_undefined)) \ constexpr enum_flags \ OPERATOR_OP (unrelated_type e1, enum_flags e2) = delete; @@ -333,6 +346,7 @@ using is_enum_flags_enum_type_t /* lval reference version. */ \ template > \ + __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 \ + __attribute__((no_sanitize_undefined)) \ constexpr bool \ OPERATOR_OP (enum_flags lhs, enum_flags rhs) \ { return lhs.raw () OP rhs.raw (); } \ @@ -384,32 +399,38 @@ ENUM_FLAGS_GEN_COMPOUND_ASSIGN (operator^=, ^) /* enum_flags OP other */ \ \ template \ + __attribute__((no_sanitize_undefined)) \ constexpr bool \ OPERATOR_OP (enum_flags lhs, enum_type rhs) \ { return lhs.raw () OP rhs; } \ \ template \ + __attribute__((no_sanitize_undefined)) \ constexpr bool \ OPERATOR_OP (enum_flags lhs, int rhs) \ { return lhs.raw () OP rhs; } \ \ template \ + __attribute__((no_sanitize_undefined)) \ constexpr bool \ OPERATOR_OP (enum_flags lhs, U rhs) = delete; \ \ /* other OP enum_flags */ \ \ template \ + __attribute__((no_sanitize_undefined)) \ constexpr bool \ OPERATOR_OP (enum_type lhs, enum_flags rhs) \ { return lhs OP rhs.raw (); } \ \ template \ + __attribute__((no_sanitize_undefined)) \ constexpr bool \ OPERATOR_OP (int lhs, enum_flags rhs) \ { return lhs OP rhs.raw (); } \ \ template \ + __attribute__((no_sanitize_undefined)) \ constexpr bool \ OPERATOR_OP (U lhs, enum_flags rhs) = delete; @@ -426,6 +447,7 @@ template , typename = gdb::Requires>> +__attribute__((no_sanitize_undefined)) constexpr enum_type operator~ (enum_type e) { @@ -442,6 +464,7 @@ template , typename = gdb::Requires>> +__attribute__((no_sanitize_undefined)) constexpr enum_flags operator~ (enum_flags 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::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 > + 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? Once the undefined behavior thing is fixed, this patch LGTM: Approved-By: Simon Marchi Simon