From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x232.google.com (mail-lj1-x232.google.com [IPv6:2a00:1450:4864:20::232]) by sourceware.org (Postfix) with ESMTPS id 487963858C39 for ; Sun, 30 Jun 2024 19:36:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 487963858C39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 487963858C39 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::232 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1719776197; cv=none; b=iuhMSK6Dj9fr750CDaeO94DdHy0eZxYiN5OPlftsPF7A9uTLELfhG7mdi45S6vVdw+lotHU+o9KzyXfuRdtbxa8etzj6j3Nyg+5C0mGAjdv0xAPZBvd1bqg7wKK/Kno9XlJdxKnn5q3t/5qT24TfNevxHFhrMTHMa9BRQJVQlbg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1719776197; c=relaxed/simple; bh=pdONwEL2h1mrPl8lpoiGNTLm5MAKTGNd1pX0mwD6gvU=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=jpNCmklNhPI9hB6J9/QMYccEaBu7+VPM/nVwrTalFiyKXWWOjjnXovc9IrUIm0+eSVp+ifvzcaMXDrgnAXuekHZuROy4xPx9SDUUXtM+fF8VTuMWswopODi2fdOmevaUgvnlKn/Ec0dv8htmeXq2tOmSVJGmYk2r/zmgMC4a6CE= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lj1-x232.google.com with SMTP id 38308e7fff4ca-2ed5ac077f5so26137431fa.1 for ; Sun, 30 Jun 2024 12:36:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1719776192; x=1720380992; darn=sourceware.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=ce+ZKI7CYinBVe532KzkceifjYY+eFNORim5Xzgx49c=; b=alJjPJoeum4v9sCboNrFITH/Aom8lvNUKlAoPCKbj7BRT2FBJacG9z0PMquko2134D 8MyRA9699bmkWe0kvZWer+Z2+K0nqzcfL8mfEMLHWn70T56viDXIHIT8d+eVeWqAF82P 1YIBRBKsNR42o5LsK2WdebDI0l+S3Fw0UzxeQZDjUXLAQ15aKXDMNweI1dhfxQuuiWG7 p1bxarSroI2oDaYLa7nZ+wkXHF4GJ9E4u0DcWJIHiklIOlKIv36j4cF9D+vdQ+Zu5Uyz xJ9yshvddnkqQdh8wXGJ45LOMwLK+2vjXqqX1zdri8e3N5Y+L166Y2qgWQK9iXVYf0cU aNrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719776193; x=1720380993; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ce+ZKI7CYinBVe532KzkceifjYY+eFNORim5Xzgx49c=; b=tU0vwLdp8AMhIRc9e8gjVC0AVbptgkUvDP1bX4DJ+uzkqId4pquHcQWS7jIDOcPSNa TA/hYN8VBIHw8P1bCgD2FwsZzz4MHT6r9UmUTmVgxYGa0t3Xs4TfobTRMLS/PAYBPzue gZRs6SyoI+DYg6BDDUElAKFzIeYHTeSYey+oTm4H6kYbOzyH/CjJFcuW5B+acb8UMVqS j1YVVetDvKASanONoGdIM6KyzPau8l6ineVK++HP95bDGl4PAMl90ncoCEo9LmQbhAPT f0SIN/6yuIUx1fYesX9F3jLs7lKGSs1cdp7W16rWfme68E6eZcgHLrnVrQcIGvw8Ncpa FsEA== X-Gm-Message-State: AOJu0Yz7G8JHOyHSiZPTpTmKakGrPRYESDzv/txTFR4tDIrBmPykF/h2 GmopDfs6uqrGT5tVQD1dzOG4VF+ZStEImP/Q3hNZSmgQe0POKT6JC9nQqA== X-Google-Smtp-Source: AGHT+IHxd5LaoxQLfevVaLa0nLX2lRLVQWhqxKj9PdUpaZlBzAzEEkJNfEJjvaCkt+KUhxKnekFOWg== X-Received: by 2002:a2e:a984:0:b0:2ec:53ad:464 with SMTP id 38308e7fff4ca-2ee5e6cd723mr30856021fa.34.1719776192328; Sun, 30 Jun 2024 12:36:32 -0700 (PDT) Received: from localhost.localdomain (c80-217-192-209.bredband.tele2.se. [80.217.192.209]) by smtp.gmail.com with ESMTPSA id 38308e7fff4ca-2ee51695ec8sm10649911fa.130.2024.06.30.12.36.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 30 Jun 2024 12:36:31 -0700 (PDT) From: Carlos Galvez To: gdb-patches@sourceware.org Cc: Carlos Galvez Subject: [PATCH v2] [gdbsupport] Fix -Wenum-constexpr-conversion in enum-flags.h Date: Sun, 30 Jun 2024 21:36:24 +0200 Message-Id: <20240630193624.2906762-1-carlosgalvezp@gmail.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,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: 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 is not what we want here, - since that returns unsigned int even when the enum decays to signed - int. */ -template 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 -struct enum_underlying_type -{ - DIAGNOSTIC_PUSH - DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION - typedef typename - integer_for_size(T (-1) < T (0))>::type - type; - DIAGNOSTIC_POP -}; - namespace enum_flags_detail { @@ -119,10 +95,62 @@ struct zero_type; /* gdb::Requires trait helpers. */ template using EnumIsUnsigned - = std::is_unsigned::type>; + = std::is_unsigned::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 +struct EnumHasFixedUnderlyingType : std::false_type +{ + static_assert(std::is_enum::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 +struct EnumHasFixedUnderlyingType> : std::true_type +{ + static_assert(std::is_enum::value); +}; + +template +using EnumIsSafeForBitwiseComplement = std::conjunction< + EnumIsUnsigned, + EnumHasFixedUnderlyingType +>; + template -using EnumIsSigned - = std::is_signed::type>; +using EnumIsUnsafeForBitwiseComplement = std::negation>; } @@ -131,7 +159,7 @@ class enum_flags { public: typedef E enum_type; - typedef typename enum_underlying_type::type underlying_type; + typedef typename std::underlying_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 - = gdb::Requires>> + = gdb::Requires>> constexpr enum_type operator~ (enum_type e) { using underlying = typename enum_flags::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 = gdb::Requires>> + typename = gdb::Requires>> constexpr void operator~ (enum_type e) = delete; template , typename - = gdb::Requires>> + = gdb::Requires>> constexpr enum_flags operator~ (enum_flags e) { using underlying = typename enum_flags::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 = gdb::Requires>> + typename = gdb::Requires>> constexpr void operator~ (enum_flags 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