From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30552 invoked by alias); 4 Nov 2016 03:23:07 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 30129 invoked by uid 89); 4 Nov 2016 03:22:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=und, re, cancel, backing X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 04 Nov 2016 03:22:24 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E960FC05678F for ; Fri, 4 Nov 2016 03:22:22 +0000 (UTC) Received: from cascais.lan (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uA43MJAF017578 for ; Thu, 3 Nov 2016 23:22:22 -0400 From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 3/3] enum_flags: Fix ternary operator and remove implicit convertion to raw enum Date: Fri, 04 Nov 2016 03:23:00 -0000 Message-Id: <1478229738-24469-4-git-send-email-palves@redhat.com> In-Reply-To: <1478229738-24469-1-git-send-email-palves@redhat.com> References: <1478229738-24469-1-git-send-email-palves@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2016-11/txt/msg00080.txt.bz2 The unit tests added by the previous patch 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. There's one case in compile/compile-c-types.c where we need to call a function in a C 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. Tested with gcc 4.8, 4.9, 5.3, 7 (trunk) and clang 3.7. gdb/ChangeLog: yyyy-mm-dd Pedro Alves * common/enum-flags.h (enum_flags::operator&=): Change parameter type to enum_flags from enum_type and adjust. (enum_flags::operator|=): Likewise. (enum_flags::operator^=): Likewise. (enum_flags::operator enum_type): Delete. (enum_flags::raw): New method. (ENUM_FLAGS_GEN_BINOP): Adjust operator functions. * compile/compile-c-types.c (convert_qualified): Use enum_flags::raw. * enum-flags-selftests.c Adjust ternary operator CHECK_VALID tests. (selftests::enum_flags_tests::self_test): Add more ternary operator tests. * 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. --- gdb/btrace.c | 4 +-- gdb/common/enum-flags.h | 26 +++++++++++-------- gdb/compile/compile-c-types.c | 2 +- gdb/enum-flags-selftests.c | 60 +++++++++++++++++++++++++++++++------------ gdb/record-btrace.c | 10 ++++---- 5 files changed, 67 insertions(+), 35 deletions(-) diff --git a/gdb/btrace.c b/gdb/btrace.c index 39d537c..2717ae2 100644 --- a/gdb/btrace.c +++ b/gdb/btrace.c @@ -229,7 +229,7 @@ ftrace_new_function (struct btrace_function *prev, static void ftrace_update_caller (struct btrace_function *bfun, struct btrace_function *caller, - enum btrace_function_flag flags) + btrace_function_flags flags) { if (bfun->up != NULL) ftrace_debug (bfun, "updating caller"); @@ -246,7 +246,7 @@ ftrace_update_caller (struct btrace_function *bfun, static void ftrace_fixup_caller (struct btrace_function *bfun, struct btrace_function *caller, - enum btrace_function_flag flags) + btrace_function_flags flags) { struct btrace_function *prev, *next; diff --git a/gdb/common/enum-flags.h b/gdb/common/enum-flags.h index 9e128c3..2939a3a 100644 --- a/gdb/common/enum-flags.h +++ b/gdb/common/enum-flags.h @@ -114,24 +114,28 @@ public: : m_enum_value ((enum_type) 0) {} - enum_flags &operator&= (enum_type e) + enum_flags &operator&= (enum_flags e) { - m_enum_value = (enum_type) (m_enum_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) (m_enum_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) (m_enum_value ^ e); + m_enum_value = (enum_type) (m_enum_value ^ e.m_enum_value); return *this; } - /* Allow conversion to the enum type. */ - constexpr operator enum_type () const + /* Like raw enums, allow conversion to the underlying type. */ + constexpr operator underlying_type () const + { return m_enum_value; } + + /* Get the underlying value as a raw enum. */ + constexpr enum_type raw () const { return m_enum_value; } /* Binary operations involving some unrelated type (which would be a @@ -167,19 +171,19 @@ private: template \ constexpr typename enum_flags_type::type \ OPERATOR_OP (enum_flags e1, enum_type e2) \ - { return enum_type (e1) OP e2; } \ + { return e1.raw () OP e2; } \ \ /* enum_flags on the RHS. */ \ template \ constexpr typename enum_flags_type::type \ OPERATOR_OP (enum_type e1, enum_flags e2) \ - { return e1 OP enum_type (e2); } \ + { return e1 OP e2.raw (); } \ \ /* enum_flags on both LHS/RHS. */ \ template \ constexpr typename enum_flags_type::type \ OPERATOR_OP (enum_flags e1, enum_flags e2) \ - { return enum_type (e1) OP enum_type (e2); } \ + { return e1.raw () OP e2.raw (); } \ \ /* Delete cases involving unrelated types. */ \ \ diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c index 505bce6..6cf1dc7 100644 --- a/gdb/compile/compile-c-types.c +++ b/gdb/compile/compile-c-types.c @@ -306,7 +306,7 @@ convert_qualified (struct compile_c_instance *context, struct type *type) return C_CTX (context)->c_ops->build_qualified_type (C_CTX (context), unqual_converted, - quals); + quals.raw ()); } /* Convert a complex type to its gcc representation. */ diff --git a/gdb/enum-flags-selftests.c b/gdb/enum-flags-selftests.c index 32c8b7a..101ad56 100644 --- a/gdb/enum-flags-selftests.c +++ b/gdb/enum-flags-selftests.c @@ -146,7 +146,7 @@ CHECK_VALID (true, RE, RE (1)) CHECK_VALID (true, RE, RE (RE2 ())) CHECK_VALID (false, void, RE (EF2 ())) CHECK_VALID (true, RE, RE (RE ())) -CHECK_VALID (true, RE, RE (EF ())) +CHECK_VALID (false, void, RE (EF ())) /* other -> EF. */ @@ -268,23 +268,30 @@ CHECK_VALID (true, EF, ~EF ()) /* Check ternary operator. This exercises implicit conversions. */ -/* Invalid since each type can be converted to the other. */ -/* GCC 4.8 incorrectly fails to compile this test with: - error: operands to ?: have different types ‘enum_flags’ and ‘RE’ - Confirmed to compile/pass with gcc 4.9, 5.3 and clang 3.7. -*/ -#if GCC_VERSION >= 4009 -CHECK_VALID (false, void, true ? EF () : RE ()) -CHECK_VALID (false, void, true ? RE () : EF ()) -#endif +CHECK_VALID (true, EF, true ? EF () : RE ()) +CHECK_VALID (true, EF, true ? RE () : EF ()) + +/* Both enums should have the same underlying type. Pick any. */ +typedef std::underlying_type::type und; /* These are valid, but it's not a big deal since you won't be able to - assign the resulting int to an enum or an enum_flags without a - cast. */ -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 ()) + 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 to compile with gcc 4.8 and 4.9 at least, with: + + no known conversion from ‘enum_flags::underlying_type {aka + unsigned int}’ to ‘enum_flags_tests::RE2’ + + They've been confirmed to compile/pass with gcc 5.3, gcc 7.1 and + clang 3.7. */ +CHECK_VALID (true, und, true ? EF () : EF2 ()) +CHECK_VALID (true, und, true ? EF2 () : EF ()) +#if GCC_VERSION >= 5003 +CHECK_VALID (true, und, true ? EF () : RE2 ()) +CHECK_VALID (true, und, true ? RE2 () : EF ()) +#endif /* Unfortunately this can't work due to the way C++ computes the return type of the ternary conditional operator. int isn't @@ -487,13 +494,34 @@ self_test () } /* Check the ternary operator. */ + { + /* raw enum, raw enum */ constexpr test_flags f1 = true ? FLAG1 : FLAG2; STATIC_SELF_CHECK (f1 == FLAG1); constexpr test_flags f2 = false ? FLAG1 : FLAG2; STATIC_SELF_CHECK (f2 == FLAG2); } + { + /* enum flags, raw enum */ + constexpr test_flags src = FLAG1; + constexpr test_flags f1 = true ? src : FLAG2; + STATIC_SELF_CHECK (f1 == FLAG1); + constexpr test_flags f2 = false ? src : FLAG2; + STATIC_SELF_CHECK (f2 == FLAG2); + } + + { + /* enum flags, enum flags */ + constexpr test_flags src1 = FLAG1; + constexpr test_flags src2 = FLAG2; + constexpr test_flags f1 = true ? src1 : src2; + STATIC_SELF_CHECK (f1 == src1); + constexpr test_flags f2 = false ? src1 : src2; + STATIC_SELF_CHECK (f2 == src2); + } + /* Check that we can use operator| in switch cases, where only constants are allowed. This should work because operator| is constexpr. */ diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index 7c0e39f..f68ce3c 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -1863,7 +1863,7 @@ record_btrace_to_get_tailcall_unwinder (struct target_ops *self) /* Return a human-readable string for FLAG. */ static const char * -btrace_thread_flag_to_str (enum btrace_thread_flag flag) +btrace_thread_flag_to_str (btrace_thread_flags flag) { switch (flag) { @@ -2165,7 +2165,7 @@ record_btrace_commit_resume (struct target_ops *ops) static void record_btrace_cancel_resume (struct thread_info *tp) { - enum btrace_thread_flag flags; + btrace_thread_flags flags; flags = tp->btrace.flags & (BTHR_MOVE | BTHR_STOP); if (flags == 0) @@ -2173,7 +2173,7 @@ record_btrace_cancel_resume (struct thread_info *tp) DEBUG ("cancel resume thread %s (%s): %x (%s)", print_thread_id (tp), - target_pid_to_str (tp->ptid), flags, + target_pid_to_str (tp->ptid), (unsigned) flags, btrace_thread_flag_to_str (flags)); tp->btrace.flags &= ~(BTHR_MOVE | BTHR_STOP); @@ -2398,7 +2398,7 @@ record_btrace_step_thread (struct thread_info *tp) { struct btrace_thread_info *btinfo; struct target_waitstatus status; - enum btrace_thread_flag flags; + btrace_thread_flags flags; btinfo = &tp->btrace; @@ -2406,7 +2406,7 @@ record_btrace_step_thread (struct thread_info *tp) btinfo->flags &= ~(BTHR_MOVE | BTHR_STOP); DEBUG ("stepping thread %s (%s): %x (%s)", print_thread_id (tp), - target_pid_to_str (tp->ptid), flags, + target_pid_to_str (tp->ptid), (unsigned) flags, btrace_thread_flag_to_str (flags)); /* We can't step without an execution history. */ -- 2.5.5