From 32ef0ecca5d8cc875453541da5be5443839e9768 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 21 Aug 2020 15:36:37 +0100 Subject: Rewrite enum_flags, add unit tests, fix problems This patch started by adding comprehensive unit tests for enum_flags. For the testing part, it adds: - tests of normal expected uses of the API. - checks that _invalid_ uses of the API would fail to compile. I.e., it validates that enum_flags really is a strong type, and that incorrect mixing of enum types would be caught at compile time. It pulls that off making use of SFINEA and C++11's decltype/constexpr. This revealed many holes in the enum_flags API. For example, the f1 assignment below currently incorrectly fails to compile: enum_flags f1 = FLAG1; enum_flags f2 = FLAG2 | f1; The unit tests also revealed that this useful use case doesn't work: enum flag { FLAG1 = 1, FLAG2 = 2 }; enum_flags src = FLAG1; enum_flags f1 = condition ? src : FLAG2; It fails to compile because enum_flags and flag are convertible to each other. Turns out that making enum_flags be implicitly convertible to the backing raw enum type was not a good idea. If we make it convertible to the underlying type instead, we fix that ternary operator use case, and, we find cases throughout the codebase that should be using the enum_flags but were using the raw backing enum instead. So it's a good change overall. Also, several operators were missing. These holes and more are plugged by this patch, by reworking how the enum_flags operators are implemented, and making use of C++11's feature of being able to delete methods/functions. There are cases in gdb/compile/ where we need to call a function in a C plugin API that expects the raw enum. To address cases like that, this adds a "raw()" method to enum_flags. This way we can keep using the safer enum_flags to construct the value, and then be explicit when we need to get at the raw enum. This makes most of the enum_flags operators constexpr. Beyond enabling more compiler optimizations and enabling the new unit tests, this has other advantages, like making it possible to use operator| with enum_flags values in switch cases, where only compile-time constants are allowed: enum_flags f = FLAG1 | FLAG2; switch (f) { case FLAG1 | FLAG2: break; } Currently that fails to compile. It also switches to a different mechanism of enabling the global operators. The current mechanism isn't namespace friendly, the new one is. It also switches to C++11-style SFINAE -- instead of wrapping the return type in a SFINAE-friently structure, we use an unnamed template parameter. I.e., this: template > enum_type operator& (enum_type e1, enum_type e2) instead of: template typename enum_flags_type::type operator& (enum_type e1, enum_type e2) Note that the static_assert inside operator~() was converted to a couple overloads (signed vs unsigned), because static_assert is too late for SFINAE-based tests, which is important for the CHECK_VALID unit tests. Tested with gcc {4.8, 7.1, 9.3} and clang {5.0.2, 10.0.0}. gdb/ChangeLog: * Makefile.in (SELFTESTS_SRCS): Add unittests/enum-flags-selftests.c. * btrace.c (ftrace_update_caller, ftrace_fixup_calle): Use btrace_function_flags instead of enum btrace_function_flag. * compile/compile-c-types.c (convert_qualified): Use enum_flags::raw. * compile/compile-cplus-symbols.c (convert_one_symbol) (convert_symbol_bmsym): * compile/compile-cplus-types.c (compile_cplus_convert_method) (compile_cplus_convert_struct_or_union_methods) (compile_cplus_instance::convert_qualified_base): * go-exp.y (parse_string_or_char): Add cast to int. * unittests/enum-flags-selftests.c: New file. * record-btrace.c (btrace_thread_flag_to_str): Change parameter's type to btrace_thread_flags from btrace_thread_flag. (record_btrace_cancel_resume, record_btrace_step_thread): Change local's type to btrace_thread_flags from btrace_thread_flag. Add cast in DEBUG call. gdbsupport/ChangeLog: * common/enum-flags.h: Include "traits.h". (DEF_ENUM_FLAGS_TYPE): Declare a function instead of defining a structure. (enum_underlying_type): Update comment. (namespace enum_flags_detail): New. Move struct zero_type here. (EnumIsUnsigned, EnumIsSigned): New. (class enum_flags): Make most methods constexpr. (operator&=, operator|=, operator^=): Take an enum_flags instead of an enum_type. (operator enum_type()): Delete. (operator&, operator|, operator^, operator~): Delete, moved out of class. (raw()): New method. (is_enum_flags_enum_type_t): Declare. (ENUM_FLAGS_GEN_BINOP, ENUM_FLAGS_GEN_COMPOUND_ASSIGN) (ENUM_FLAGS_GEN_COMP): New. Use them to reimplement global operators. (operator~): Now constexpr and reimplemented. (operator<<, operator>>): New deleted functions. * valid-expr.h (CHECK_VALID_EXPR_5, CHECK_VALID_EXPR_6): New. --- gdbsupport/enum-flags.h | 366 +++++++++++++++++++++++++++++++++++++----------- gdbsupport/valid-expr.h | 15 ++ 2 files changed, 299 insertions(+), 82 deletions(-) (limited to 'gdbsupport') diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h index 825ff4f..b3e317e 100644 --- a/gdbsupport/enum-flags.h +++ b/gdbsupport/enum-flags.h @@ -18,6 +18,8 @@ #ifndef COMMON_ENUM_FLAGS_H #define COMMON_ENUM_FLAGS_H +#include "traits.h" + /* Type-safe wrapper for enum flags. enum flags are enums where the values are bits that are meant to be ORed together. @@ -51,23 +53,31 @@ #ifdef __cplusplus -/* Traits type used to prevent the global operator overloads from - instantiating for non-flag enums. */ -template struct enum_flags_type {}; - -/* Use this to mark an enum as flags enum. It defines FLAGS as +/* Use this to mark an enum as flags enum. It defines FLAGS_TYPE as enum_flags wrapper class for ENUM, and enables the global operator overloads for ENUM. */ #define DEF_ENUM_FLAGS_TYPE(enum_type, flags_type) \ typedef enum_flags flags_type; \ - template<> \ - struct enum_flags_type \ - { \ - typedef enum_flags type; \ - } + void is_enum_flags_enum_type (enum_type *) + +/* To enable the global enum_flags operators for enum, declare an + "is_enum_flags_enum_type" overload that has exactly one parameter, + of type a pointer to that enum class. E.g.,: + + void is_enum_flags_enum_type (enum some_flag *); + + The function does not need to be defined, only declared. + DEF_ENUM_FLAGS_TYPE declares this. + + A function declaration is preferred over a traits type, because the + former allows calling the DEF_ENUM_FLAGS_TYPE macro inside a + namespace to define the corresponding enum flags type in that + namespace. The compiler finds the corresponding + is_enum_flags_enum_type function via ADL. */ -/* Until we can rely on std::underlying type being universally - available (C++11), roll our own for enums. */ +/* 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; }; @@ -86,128 +96,320 @@ struct enum_underlying_type type; }; -template -class enum_flags +namespace enum_flags_detail { -public: - typedef E enum_type; - typedef typename enum_underlying_type::type underlying_type; -private: - /* Private type used to support initializing flag types with zero: +/* Private type used to support initializing flag types with zero: - foo_flags f = 0; + foo_flags f = 0; - but not other integers: + but not other integers: - foo_flags f = 1; + foo_flags f = 1; - The way this works is that we define an implicit constructor that - takes a pointer to this private type. Since nothing can - instantiate an object of this type, the only possible pointer to - pass to the constructor is the NULL pointer, or, zero. */ - struct zero_type; + The way this works is that we define an implicit constructor that + takes a pointer to this private type. Since nothing can + instantiate an object of this type, the only possible pointer to + pass to the constructor is the NULL pointer, or, zero. */ +struct zero_type; - underlying_type - underlying_value () const - { - return m_enum_value; - } +/* gdb::Requires trait helpers. */ +template +using EnumIsUnsigned + = std::is_unsigned::type>; +template +using EnumIsSigned + = std::is_signed::type>; + +} + +template +class enum_flags +{ +public: + typedef E enum_type; + typedef typename enum_underlying_type::type underlying_type; public: /* Allow default construction. */ - enum_flags () + constexpr enum_flags () : m_enum_value ((enum_type) 0) {} + /* The default move/copy ctor/assignment do the right thing. */ + /* If you get an error saying these two overloads are ambiguous, then you tried to mix values of different enum types. */ - enum_flags (enum_type e) + constexpr enum_flags (enum_type e) : m_enum_value (e) {} - enum_flags (struct enum_flags::zero_type *zero) + constexpr enum_flags (enum_flags_detail::zero_type *zero) : m_enum_value ((enum_type) 0) {} - enum_flags &operator&= (enum_type e) + enum_flags &operator&= (enum_flags e) { - m_enum_value = (enum_type) (underlying_value () & e); + m_enum_value = (enum_type) (m_enum_value & e.m_enum_value); return *this; } - enum_flags &operator|= (enum_type e) + enum_flags &operator|= (enum_flags e) { - m_enum_value = (enum_type) (underlying_value () | e); + m_enum_value = (enum_type) (m_enum_value | e.m_enum_value); return *this; } - enum_flags &operator^= (enum_type e) + enum_flags &operator^= (enum_flags e) { - m_enum_value = (enum_type) (underlying_value () ^ e); + m_enum_value = (enum_type) (m_enum_value ^ e.m_enum_value); return *this; } - operator enum_type () const + /* Like raw enums, allow conversion to the underlying type. */ + constexpr operator underlying_type () const { return m_enum_value; } - enum_flags operator& (enum_type e) const - { - return (enum_type) (underlying_value () & e); - } - enum_flags operator| (enum_type e) const - { - return (enum_type) (underlying_value () | e); - } - enum_flags operator^ (enum_type e) const - { - return (enum_type) (underlying_value () ^ e); - } - enum_flags operator~ () const + /* Get the underlying value as a raw enum. */ + constexpr enum_type raw () const { - // We only the underlying type to be unsigned when actually using - // operator~ -- if it were not unsigned, undefined behavior could - // result. However, asserting this in the class itself would - // require too many unnecessary changes to otherwise ok enum - // types. - gdb_static_assert (std::is_unsigned::value); - return (enum_type) ~underlying_value (); + return m_enum_value; } + /* Binary operations involving some unrelated type (which would be a + bug) are implemented as non-members, and deleted. */ + private: /* Stored as enum_type because GDB knows to print the bit flags neatly if the enum values look like bit flags. */ enum_type m_enum_value; }; +template +using is_enum_flags_enum_type_t + = decltype (is_enum_flags_enum_type (std::declval ())); + /* Global operator overloads. */ -template -typename enum_flags_type::type -operator& (enum_type e1, enum_type e2) +/* Generate binary operators. */ + +#define ENUM_FLAGS_GEN_BINOP(OPERATOR_OP, OP) \ + \ + /* Raw enum on both LHS/RHS. Returns raw enum type. */ \ + template > \ + constexpr enum_type \ + OPERATOR_OP (enum_type e1, enum_type e2) \ + { \ + using underlying = typename enum_flags::underlying_type; \ + return (enum_type) (underlying (e1) OP underlying (e2)); \ + } \ + \ + /* enum_flags on the LHS. */ \ + template > \ + constexpr enum_flags \ + OPERATOR_OP (enum_flags e1, enum_type e2) \ + { return e1.raw () OP e2; } \ + \ + /* enum_flags on the RHS. */ \ + template > \ + constexpr enum_flags \ + OPERATOR_OP (enum_type e1, enum_flags e2) \ + { return e1 OP e2.raw (); } \ + \ + /* enum_flags on both LHS/RHS. */ \ + template > \ + constexpr enum_flags \ + OPERATOR_OP (enum_flags e1, enum_flags e2) \ + { return e1.raw () OP e2.raw (); } \ + \ + /* Delete cases involving unrelated types. */ \ + \ + template > \ + constexpr enum_flags \ + OPERATOR_OP (enum_type e1, unrelated_type e2) = delete; \ + \ + template > \ + constexpr enum_flags \ + OPERATOR_OP (unrelated_type e1, enum_type e2) = delete; \ + \ + template > \ + constexpr enum_flags \ + OPERATOR_OP (enum_flags e1, unrelated_type e2) = delete; \ + \ + template > \ + constexpr enum_flags \ + OPERATOR_OP (unrelated_type e1, enum_flags e2) = delete; + +/* Generate non-member compound assignment operators. Only the raw + enum versions are defined here. The enum_flags versions are + defined as member functions, simply because it's less code that + way. + + Note we delete operators that would allow e.g., + + "enum_type | 1" or "enum_type1 | enum_type2" + + because that would allow a mistake like : + enum flags1 { F1_FLAGS1 = 1 }; + enum flags2 { F2_FLAGS2 = 2 }; + enum flags1 val; + switch (val) { + case F1_FLAGS1 | F2_FLAGS2: + ... + + If you really need to 'or' enumerators of different flag types, + cast to integer first. +*/ +#define ENUM_FLAGS_GEN_COMPOUND_ASSIGN(OPERATOR_OP, OP) \ + /* lval reference version. */ \ + template > \ + constexpr enum_type & \ + OPERATOR_OP (enum_type &e1, enum_type e2) \ + { return e1 = e1 OP e2; } \ + \ + /* rval reference version. */ \ + template > \ + constexpr enum_type & \ + OPERATOR_OP (enum_type &&e1, enum_type e2) \ + { return e1 = e1 OP e2; } \ + \ + /* Delete compound assignment from unrelated types. */ \ + \ + template > \ + constexpr enum_type & \ + OPERATOR_OP (enum_type &e1, other_enum_type e2) = delete; \ + \ + template > \ + constexpr enum_type & \ + OPERATOR_OP (enum_type &&e1, other_enum_type e2) = delete; + +ENUM_FLAGS_GEN_BINOP (operator|, |) +ENUM_FLAGS_GEN_BINOP (operator&, &) +ENUM_FLAGS_GEN_BINOP (operator^, ^) + +ENUM_FLAGS_GEN_COMPOUND_ASSIGN (operator|=, |) +ENUM_FLAGS_GEN_COMPOUND_ASSIGN (operator&=, &) +ENUM_FLAGS_GEN_COMPOUND_ASSIGN (operator^=, ^) + +/* Allow comparison with enum_flags, raw enum, and integers, only. + The latter case allows "== 0". As side effect, it allows comparing + with integer variables too, but that's not a common mistake to + make. It's important to disable comparison with unrelated types to + prevent accidentally comparing with unrelated enum values, which + are convertible to integer, and thus coupled with enum_flags + convertion to underlying type too, would trigger the built-in 'bool + operator==(unsigned, int)' operator. */ + +#define ENUM_FLAGS_GEN_COMP(OPERATOR_OP, OP) \ + \ + /* enum_flags OP enum_flags */ \ + \ + template \ + constexpr bool \ + OPERATOR_OP (enum_flags lhs, enum_flags rhs) \ + { return lhs.raw () OP rhs.raw (); } \ + \ + /* enum_flags OP other */ \ + \ + template \ + constexpr bool \ + OPERATOR_OP (enum_flags lhs, enum_type rhs) \ + { return lhs.raw () OP rhs; } \ + \ + template \ + constexpr bool \ + OPERATOR_OP (enum_flags lhs, int rhs) \ + { return lhs.raw () OP rhs; } \ + \ + template \ + constexpr bool \ + OPERATOR_OP (enum_flags lhs, U rhs) = delete; \ + \ + /* other OP enum_flags */ \ + \ + template \ + constexpr bool \ + OPERATOR_OP (enum_type lhs, enum_flags rhs) \ + { return lhs OP rhs.raw (); } \ + \ + template \ + constexpr bool \ + OPERATOR_OP (int lhs, enum_flags rhs) \ + { return lhs OP rhs.raw (); } \ + \ + template \ + constexpr bool \ + OPERATOR_OP (U lhs, enum_flags rhs) = delete; + +ENUM_FLAGS_GEN_COMP (operator==, ==) +ENUM_FLAGS_GEN_COMP (operator!=, !=) + +/* Unary operators for the raw flags enum. */ + +/* We require underlying type to be unsigned when using operator~ -- + if it were not unsigned, undefined behavior could result. However, + asserting this in the class itself would require too many + unnecessary changes to usages of otherwise OK enum types. */ +template , + typename + = gdb::Requires>> +constexpr enum_type +operator~ (enum_type e) { - return enum_flags (e1) & e2; + using underlying = typename enum_flags::underlying_type; + return (enum_type) ~underlying (e); } -template -typename enum_flags_type::type -operator| (enum_type e1, enum_type e2) +template , + typename = gdb::Requires>> +constexpr void operator~ (enum_type e) = delete; + +template , + typename + = gdb::Requires>> +constexpr enum_flags +operator~ (enum_flags e) { - return enum_flags (e1) | e2; + using underlying = typename enum_flags::underlying_type; + return (enum_type) ~underlying (e); } -template -typename enum_flags_type::type -operator^ (enum_type e1, enum_type e2) -{ - return enum_flags (e1) ^ e2; -} +template , + typename = gdb::Requires>> +constexpr void operator~ (enum_flags e) = delete; -template -typename enum_flags_type::type -operator~ (enum_type e) -{ - return ~enum_flags (e); -} +/* Delete operator<< and operator>>. */ + +template > +void operator<< (const enum_type &, const any_type &) = delete; + +template > +void operator<< (const enum_flags &, const any_type &) = delete; + +template > +void operator>> (const enum_type &, const any_type &) = delete; + +template > +void operator>> (const enum_flags &, const any_type &) = delete; #else /* __cplusplus */ diff --git a/gdbsupport/valid-expr.h b/gdbsupport/valid-expr.h index a22fa61..459de17 100644 --- a/gdbsupport/valid-expr.h +++ b/gdbsupport/valid-expr.h @@ -91,4 +91,19 @@ ESC_PARENS (T1, T2, T3, T4), \ VALID, EXPR_TYPE, EXPR) +#define CHECK_VALID_EXPR_5(T1, T2, T3, T4, T5, VALID, EXPR_TYPE, EXPR) \ + CHECK_VALID_EXPR_INT (ESC_PARENS (typename T1, typename T2, \ + typename T3, typename T4, \ + typename T5), \ + ESC_PARENS (T1, T2, T3, T4, T5), \ + VALID, EXPR_TYPE, EXPR) + +#define CHECK_VALID_EXPR_6(T1, T2, T3, T4, T5, T6, \ + VALID, EXPR_TYPE, EXPR) \ + CHECK_VALID_EXPR_INT (ESC_PARENS (typename T1, typename T2, \ + typename T3, typename T4, \ + typename T5, typename T6), \ + ESC_PARENS (T1, T2, T3, T4, T5, T6), \ + VALID, EXPR_TYPE, EXPR) + #endif /* COMMON_VALID_EXPR_H */ -- cgit v1.1