From: Carlos Galvez <carlosgalvezp@gmail.com>
To: gdb-patches@sourceware.org, Pedro Alves <pedro@palves.net>
Subject: Re: [PATCH v2] [gdbsupport] Fix -Wenum-constexpr-conversion in enum-flags.h
Date: Sun, 30 Jun 2024 21:38:36 +0200 [thread overview]
Message-ID: <CAEYdZDoPNk-vB4KiOMdb=eAHTZzOY+sgy_7zzzwKo8LTLCZvoQ@mail.gmail.com> (raw)
In-Reply-To: <20240630193624.2906762-1-carlosgalvezp@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 14019 bytes --]
@Pedro Alves <pedro@palves.net> I have reworked the patch to follow a
different approach, it feels a lot more solid now! This time there is no
need for follow-up work, the patch is self-contained. The only thing I
would need confirmation from you guys if it's OK to remove the mentioned
tests.
On Sun, 30 Jun 2024 at 21:36, Carlos Galvez <carlosgalvezp@gmail.com> wrote:
> This fixes PR 31331:
> https://sourceware.org/bugzilla/show_bug.cgi?id=31331
>
> Currently, enum-flags.h is suppressing the warning
> -Wenum-constexpr-conversion coming from recent versions of Clang.
> This warning is intended to be made a compiler error
> (non-downgradeable) in future versions of Clang:
>
> https://github.com/llvm/llvm-project/issues/59036
>
> The rationale is that casting a value of an integral type into an
> enumeration is Undefined Behavior if the value does not fit in the
> range of values of the enum:
> https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1766
>
> Undefined Behavior is not allowed in constant expressions, leading to
> an ill-formed program.
>
> In this case, in enum-flags.h, we are casting the value -1 to an enum
> of a positive range only, which is UB as per the Standard and thus not
> allowed in a constexpr context.
>
> The purpose of doing this instead of using std::underlying_type is
> because, for C-style enums, std::underlying_type typically returns
> "unsigned int". However, when operating with it arithmetically, the
> enum is promoted to *signed* int, which is what we want to avoid.
>
> This patch solves this issue as follows:
>
> * Use std::underlying_type and remove the custom enum_underlying_type.
>
> * Ensure that operator~ is called always on an unsigned integer. We do
> this by casting the input enum into std::size_t, which can fit any
> unsigned integer. We have the guarantee that the cast is safe,
> because we have checked that the underlying type is unsigned. If the
> enum had negative values, the underlying type would be signed.
>
> This solves the issue with C-style enums, but also solves a hidden
> issue: enums with underlying type of std::uint8_t or std::uint16_t are
> *also* promoted to signed int. Now they are all explicitly casted
> to the largest unsigned int type and operator~ is safe to use.
>
> * There is one more thing that needs fix. Currently, operator~ is
> implemented as follows:
>
> return (enum_type) ~underlying(e);
>
> After applying ~underlying(e), the result is a very large value,
> which we then cast to "enum_type". This cast is Undefined Behavior
> if the large value does not fit in the range of the enum. For
> C++ enums (scoped and/or with explicit underlying type), the range
> of the enum is the entire range of the underlying type, so the cast
> is safe. However, for C-style enums, the range is the smallest
> bit-field that can hold all the values of the enumeration. So the
> range is a lot smaller and casting a large value to the enum would
> invoke Undefined Behavior.
>
> To solve this problem, we create a new trait
> EnumHasFixedUnderlyingType, to ensure operator~ may only be called
> on C++-style enums. This behavior is roughly the same as what we
> had on trunk, but relying on different properties of the enums.
>
> * Once this is implemented, the following tests fail to compile:
>
> CHECK_VALID (true, int, true ? EF () : EF2 ())
>
> This is because it expects the enums to be promoted to signed int,
> instead of unsigned int (which is the true underlying type).
>
> I propose to remove these tests altogether, because:
>
> - The comment nearby say they are not very important.
> - Comparing 2 enums of different type like that is strange, relies
> on integer promotions and thus hurts readability. As per comments
> in the related PR, we likely don't want this type of code in gdb
> code anyway, so there's no point in testing it.
> - Most importantly, this type of comparison will be ill-formed in
> C++26 for regular enums, so enum_flags does not need to emulate
> that.
>
> Since this is the only place where the warning was suppressed, remove
> also the corresponding macro in include/diagnostics.h.
>
> The change has been tested by running the entire gdb test suite
> (make check) and comparing the results (testsuite/gdb.sum) against
> trunk. No noticeable differences have been observed.
> ---
> gdb/unittests/enum-flags-selftests.c | 27 -------
> gdbsupport/enum-flags.h | 104 ++++++++++++++++++---------
> include/diagnostics.h | 5 --
> 3 files changed, 70 insertions(+), 66 deletions(-)
>
> diff --git a/gdb/unittests/enum-flags-selftests.c
> b/gdb/unittests/enum-flags-selftests.c
> index b55d8c31406..02563e568d1 100644
> --- a/gdb/unittests/enum-flags-selftests.c
> +++ b/gdb/unittests/enum-flags-selftests.c
> @@ -233,33 +233,6 @@ CHECK_VALID (true, UEF, ~UEF ())
> CHECK_VALID (true, EF, true ? EF () : RE ())
> CHECK_VALID (true, EF, true ? RE () : EF ())
>
> -/* These are valid, but it's not a big deal since you won't be able to
> - assign the resulting integer to an enum or an enum_flags without a
> - cast.
> -
> - The latter two tests are disabled on older GCCs because they
> - incorrectly fail with gcc 4.8 and 4.9 at least. Running the test
> - outside a SFINAE context shows:
> -
> - invalid user-defined conversion from ‘EF’ to ‘RE2’
> -
> - They've been confirmed to compile/pass with gcc 5.3, gcc 7.1 and
> - clang 3.7. */
> -
> -CHECK_VALID (true, int, true ? EF () : EF2 ())
> -CHECK_VALID (true, int, true ? EF2 () : EF ())
> -CHECK_VALID (true, int, true ? EF () : RE2 ())
> -CHECK_VALID (true, int, true ? RE2 () : EF ())
> -
> -/* Same, but with an unsigned enum. */
> -
> -typedef unsigned int uns;
> -
> -CHECK_VALID (true, uns, true ? EF () : UEF ())
> -CHECK_VALID (true, uns, true ? UEF () : EF ())
> -CHECK_VALID (true, uns, true ? EF () : URE ())
> -CHECK_VALID (true, uns, true ? URE () : EF ())
> -
> /* Unfortunately this can't work due to the way C++ computes the
> return type of the ternary conditional operator. int isn't
> implicitly convertible to the raw enum type, so the type of the
> diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
> index 50780043477..acec2030fca 100644
> --- a/gdbsupport/enum-flags.h
> +++ b/gdbsupport/enum-flags.h
> @@ -75,30 +75,6 @@
> namespace. The compiler finds the corresponding
> is_enum_flags_enum_type function via ADL. */
>
> -/* Note that std::underlying_type<enum_type> is not what we want here,
> - since that returns unsigned int even when the enum decays to signed
> - int. */
> -template<int size, bool sign> class integer_for_size { typedef void type;
> };
> -template<> struct integer_for_size<1, 0> { typedef uint8_t type; };
> -template<> struct integer_for_size<2, 0> { typedef uint16_t type; };
> -template<> struct integer_for_size<4, 0> { typedef uint32_t type; };
> -template<> struct integer_for_size<8, 0> { typedef uint64_t type; };
> -template<> struct integer_for_size<1, 1> { typedef int8_t type; };
> -template<> struct integer_for_size<2, 1> { typedef int16_t type; };
> -template<> struct integer_for_size<4, 1> { typedef int32_t type; };
> -template<> struct integer_for_size<8, 1> { typedef int64_t type; };
> -
> -template<typename T>
> -struct enum_underlying_type
> -{
> - DIAGNOSTIC_PUSH
> - DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION
> - typedef typename
> - integer_for_size<sizeof (T), static_cast<bool>(T (-1) < T (0))>::type
> - type;
> - DIAGNOSTIC_POP
> -};
> -
> namespace enum_flags_detail
> {
>
> @@ -119,10 +95,62 @@ struct zero_type;
> /* gdb::Requires trait helpers. */
> template <typename enum_type>
> using EnumIsUnsigned
> - = std::is_unsigned<typename enum_underlying_type<enum_type>::type>;
> + = std::is_unsigned<typename std::underlying_type<enum_type>::type>;
> +
> +/* Helper to detect whether an enum has a fixed underlying type. This can
> be
> + achieved by using a scoped enum (in which case the type is "int") or
> + an explicit underlying type. C-style enums that are unscoped or do not
> + have an explicit underlying type have an implementation-defined
> underlying
> + type.
> +
> + https://timsong-cpp.github.io/cppwp/n4659/dcl.enum#5
> +
> + We need this trait in order to ensure that operator~ below does NOT
> + operate on old-style enums. This is because we apply operator~ on
> + the value and then cast the result to the enum_type. This is however
> + Undefined Behavior if the result does not fit in the range of possible
> + values for the enum. For enums with fixed underlying type, the entire
> + range of the integer is available. However, for old-style enums, the
> range
> + is only the smallest bit-field that can hold all the values of the
> + enumeration, typically much smaller than the underlying integer:
> +
> + https://timsong-cpp.github.io/cppwp/n4659/expr.static.cast#10
> + https://timsong-cpp.github.io/cppwp/n4659/dcl.enum#8
> +
> + To implement this, we leverage the fact that, since C++17, enums with
> + fixed underlying type can be list-initialized from an integer:
> + https://timsong-cpp.github.io/cppwp/n4659/dcl.init.list#3.7
> +
> + Old-style enums cannot be initialized like that, leading to ill-formed
> + code.
> +
> + We then use this together with SFINAE to create the desired trait.
> +
> +*/
> +// Primary template
> +template <typename enum_type, typename = void>
> +struct EnumHasFixedUnderlyingType : std::false_type
> +{
> + static_assert(std::is_enum<enum_type>::value);
> +};
> +
> +// Specialization that is active only if enum_type can be list-initialized
> +// from an integer (0). Only enums with fixed underlying type satisfy this
> +// property in C++17.
> +template <typename enum_type>
> +struct EnumHasFixedUnderlyingType<enum_type,
> std::void_t<decltype(enum_type{0})>> : std::true_type
> +{
> + static_assert(std::is_enum<enum_type>::value);
> +};
> +
> +template <typename enum_type>
> +using EnumIsSafeForBitwiseComplement = std::conjunction<
> + EnumIsUnsigned<enum_type>,
> + EnumHasFixedUnderlyingType<enum_type>
> +>;
> +
> template <typename enum_type>
> -using EnumIsSigned
> - = std::is_signed<typename enum_underlying_type<enum_type>::type>;
> +using EnumIsUnsafeForBitwiseComplement =
> std::negation<EnumIsSafeForBitwiseComplement<enum_type>>;
>
> }
>
> @@ -131,7 +159,7 @@ class enum_flags
> {
> public:
> typedef E enum_type;
> - typedef typename enum_underlying_type<enum_type>::type underlying_type;
> + typedef typename std::underlying_type<enum_type>::type underlying_type;
>
> /* For to_string. Maps one enumerator of E to a string. */
> struct string_mapping
> @@ -394,33 +422,41 @@ ENUM_FLAGS_GEN_COMP (operator!=, !=)
> template <typename enum_type,
> typename = is_enum_flags_enum_type_t<enum_type>,
> typename
> - = gdb::Requires<enum_flags_detail::EnumIsUnsigned<enum_type>>>
> + =
> gdb::Requires<enum_flags_detail::EnumIsSafeForBitwiseComplement<enum_type>>>
> constexpr enum_type
> operator~ (enum_type e)
> {
> using underlying = typename enum_flags<enum_type>::underlying_type;
> - return (enum_type) ~underlying (e);
> + // Cast to std::size_t first, to prevent integer promotions from
> + // enums with fixed underlying type std::uint8_t or std::uint16_t
> + // to signed int.
> + // This ensures we apply the bitwise complement on an unsigned type.
> + return (enum_type)(underlying) ~std::size_t (e);
> }
>
> template <typename enum_type,
> typename = is_enum_flags_enum_type_t<enum_type>,
> - typename =
> gdb::Requires<enum_flags_detail::EnumIsSigned<enum_type>>>
> + typename =
> gdb::Requires<enum_flags_detail::EnumIsUnsafeForBitwiseComplement<enum_type>>>
> constexpr void operator~ (enum_type e) = delete;
>
> template <typename enum_type,
> typename = is_enum_flags_enum_type_t<enum_type>,
> typename
> - = gdb::Requires<enum_flags_detail::EnumIsUnsigned<enum_type>>>
> + =
> gdb::Requires<enum_flags_detail::EnumIsSafeForBitwiseComplement<enum_type>>>
> constexpr enum_flags<enum_type>
> operator~ (enum_flags<enum_type> e)
> {
> using underlying = typename enum_flags<enum_type>::underlying_type;
> - return (enum_type) ~underlying (e);
> + // Cast to std::size_t first, to prevent integer promotions from
> + // enums with fixed underlying type std::uint8_t or std::uint16_t
> + // to signed int.
> + // This ensures we apply the bitwise complement on an unsigned type.
> + return (enum_type)(underlying) ~std::size_t (e);
> }
>
> template <typename enum_type,
> typename = is_enum_flags_enum_type_t<enum_type>,
> - typename =
> gdb::Requires<enum_flags_detail::EnumIsSigned<enum_type>>>
> + typename =
> gdb::Requires<enum_flags_detail::EnumIsUnsafeForBitwiseComplement<enum_type>>>
> constexpr void operator~ (enum_flags<enum_type> e) = delete;
>
> /* Delete operator<< and operator>>. */
> diff --git a/include/diagnostics.h b/include/diagnostics.h
> index 97e30ab807f..14575e20e27 100644
> --- a/include/diagnostics.h
> +++ b/include/diagnostics.h
> @@ -76,11 +76,6 @@
> # define DIAGNOSTIC_ERROR_SWITCH \
> DIAGNOSTIC_ERROR ("-Wswitch")
>
> -# if __has_warning ("-Wenum-constexpr-conversion")
> -# define DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION \
> - DIAGNOSTIC_IGNORE ("-Wenum-constexpr-conversion")
> -# endif
> -
> #elif defined (__GNUC__) /* GCC */
>
> # define DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS \
> --
> 2.34.1
>
>
prev parent reply other threads:[~2024-06-30 19:39 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-30 19:36 Carlos Galvez
2024-06-30 19:38 ` Carlos Galvez [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEYdZDoPNk-vB4KiOMdb=eAHTZzOY+sgy_7zzzwKo8LTLCZvoQ@mail.gmail.com' \
--to=carlosgalvezp@gmail.com \
--cc=gdb-patches@sourceware.org \
--cc=pedro@palves.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).