From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 123296 invoked by alias); 1 Nov 2015 15:19:45 -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 123282 invoked by uid 89); 1 Nov 2015 15:19:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-ob0-f181.google.com Received: from mail-ob0-f181.google.com (HELO mail-ob0-f181.google.com) (209.85.214.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sun, 01 Nov 2015 15:19:41 +0000 Received: by obdgf3 with SMTP id gf3so13842661obd.3 for ; Sun, 01 Nov 2015 07:19:39 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type:content-transfer-encoding; bh=CAjVZjV9HYZgHUgyfHG+U23wNQXc9Ci0HEFUnyXrhGA=; b=irjpQwwMztw2XOPguH/njCExmwsSGLiATUuxARHgkD9BTsGOmTOvV1WYhbs7sSXFF0 K9diLmSJN9A936lYDM2KJFfsScsltl9g8ilM9sJPJuDkkMP9dmQzKAtlOLAuxrDNxsBA EInF/LGCRLmO7gFANTqQY2fKkborMrl7hsu5UEicbt41Xs2mwYszx1x6fbb7ZOK+TmRJ SWWc7lPE7BYTBL2aInhOskFr96DomoNBxUDjSt3SNcHBwGPKa9mE4Nm2bAv1KogGkSQt ae4e2v7pLQ5QHM6+EmNe0dXFOfjm3wYYg6UaIpZkEzqIPYwKcNZRqsnJvV/1a0xW/cWj qa9w== X-Gm-Message-State: ALoCoQm4E4WxH35vKETIncNcZvLiZkEORO7kIL+e0goEwmpQTI/oBYX3cZL7RpQ+ZYmnXFPkI6BD X-Received: by 10.182.142.129 with SMTP id rw1mr11481922obb.28.1446391179102; Sun, 01 Nov 2015 07:19:39 -0800 (PST) MIME-Version: 1.0 Received: by 10.182.155.2 with HTTP; Sun, 1 Nov 2015 07:19:19 -0800 (PST) In-Reply-To: <1446144341-21267-1-git-send-email-palves@redhat.com> References: <1446144341-21267-1-git-send-email-palves@redhat.com> From: Patrick Palka Date: Sun, 01 Nov 2015 15:19:00 -0000 Message-ID: Subject: Re: [PATCH] Type-safe wrapper for enum flags To: Pedro Alves Cc: "gdb-patches@sourceware.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2015-11/txt/msg00004.txt.bz2 On Thu, Oct 29, 2015 at 2:45 PM, Pedro Alves wrote: > This patch fixes C++ build errors like this: > > /home/pedro/gdb/mygit/cxx-convertion/src/gdb/linux-tdep.c:1126:35: error:= invalid conversion from =E2=80=98int=E2=80=99 to =E2=80=98filterflags=E2= =80=99 [-fpermissive] > | COREFILTER_HUGETLB_PRIVATE); > ^ > > This is a case of enums used as bit flags. Unlike "regular" enums, > these values are supposed to be or'ed together. However, in C++, the > type of "(ENUM1 | ENUM2)" is int, and you then can't assign an int to > an enum variable without a cast. That means that this: > > enum foo_flags flags =3D 0; > > if (...) > flags |=3D FOO_FLAG1; > if (...) > flags |=3D FOO_FLAG2; > > ... would have to be written as: > > enum foo_flags flags =3D (enum foo_flags) 0; > > if (...) > flags =3D (enum foo_flags) (flags | FOO_FLAG1); > if (...) > flags =3D (enum foo_flags) (flags | FOO_FLAG2); > > which is ... ugly. Alternatively, we'd have to use an int for the > variable's type, which isn't ideal either. > > This patch instead adds an "enum flags" class. "enum flags" are > exactly the enums where the values are bits that are meant to be ORed > together. > > This allows writing code like the below, while with raw enums this > would fail to compile without casts to enum type at the assignments to > 'f': > > enum some_flag > { > flag_val1 =3D 1 << 1, > flag_val2 =3D 1 << 2, > flag_val3 =3D 1 << 3, > flag_val4 =3D 1 << 4, > }; > DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags) > > some_flags f =3D flag_val1 | flag_val2; > f |=3D flag_val3; > > It's also possible to assign literal zero to an enum flags variable > (meaning, no flags), dispensing either adding an awkward explicit "no > value" value to the enumeration or the cast to assignments from 0. > For example: > > some_flags f =3D 0; > f |=3D flag_val3 | flag_val4; > > Note that literal integers other than zero do fail to compile: > > some_flags f =3D 1; // error > > C is still supported -- DEF_ENUM_FLAGS_TYPE is just a typedef in that > case. > > gdb/ChangeLog: > 2015-10-29 Pedro Alves > > * btrace.h: Include common/enum_flags.h. > (btrace_insn_flags): Define. > (struct btrace_insn) : Change type. > (btrace_function_flags): Define. > (struct btrace_function) : Change type. > (btrace_thread_flags): Define. > (struct btrace_thread_info) : Change type. > * c-exp.y (token_flags): Rename to ... > (token_flag): ... this. > (token_flags): Define. > (struct token) : Change type. > * common/enum_flags.h: New file. > * compile/compile-c-types.c (convert_qualified): Change type of > 'quals' local. > * compile/compile-internal.h: Include "common/enum_flags.h". > (gcc_qualifiers_flags): Define. > * completer.c (enum reg_completer_targets): Rename to ... > (enum reg_completer_target): ... this. > (reg_completer_targets): Define. > (reg_or_group_completer_1): Change type of 'targets' parameter. > * disasm.c (do_mixed_source_and_assembly_deprecated): Change type > of 'psl_flags' local. > (do_mixed_source_and_assembly): Change type of 'psl_flags' local. > * infrun.c: Include "common/enum_flags.h". > (enum step_over_what): Rename to ... > (enum step_over_what_flag): ... this. > (step_over_what): Change type. > (start_step_over): Change type of 'step_what' local. > (thread_still_needs_step_over): Now returns a step_over_what. > Adjust. > (keep_going_pass_signal): Change type of 'step_what' local. > * linux-tdep.c: Include "common/enum_flags.h". > (enum filterflags): Rename to ... > (enum filter_flag): ... this. > (filter_flags): Define. > (dump_mapping_p): Change type of 'filterflags' parameter. > (linux_find_memory_regions_full): Change type of 'filterflags' > local. > (linux_find_memory_regions_full): Pass the address of an unsigned > int to sscanf instead of the address of an enum. > * record-btrace.c (btrace_call_history): Replace 'flags' parameter > with 'int_flags' parameter. Adjust. > (record_btrace_call_history, record_btrace_call_history_range) > (record_btrace_call_history_from): Rename 'flags' parameter to > 'int_flags'. Use record_print_flags. > * record.h: Include "common/enum_flags.h". > (record_print_flags): Define. > * source.c: Include "common/enum_flags.h". > (print_source_lines_base, print_source_lines): Change type of > flags parameter. > * symtab.h: Include "common/enum_flags.h". > (enum print_source_lines_flags): Rename to ... > (enum print_source_lines_flag): ... this. > (print_source_lines_flags): Define. > (print_source_lines): Change prototype. > --- > gdb/btrace.h | 10 +- > gdb/c-exp.y | 5 +- > gdb/common/enum_flags.h | 211 +++++++++++++++++++++++++++++++++++= ++++++ > gdb/compile/compile-c-types.c | 2 +- > gdb/compile/compile-internal.h | 4 + > gdb/completer.c | 5 +- > gdb/disasm.c | 4 +- > gdb/infrun.c | 14 +-- > gdb/linux-tdep.c | 19 ++-- > gdb/record-btrace.c | 22 +++-- > gdb/record.h | 2 + > gdb/source.c | 7 +- > gdb/symtab.h | 6 +- > 13 files changed, 275 insertions(+), 36 deletions(-) > create mode 100644 gdb/common/enum_flags.h > > diff --git a/gdb/btrace.h b/gdb/btrace.h > index f844df8..e012c1e 100644 > --- a/gdb/btrace.h > +++ b/gdb/btrace.h > @@ -28,6 +28,7 @@ > > #include "btrace-common.h" > #include "target/waitstatus.h" /* For enum target_stop_reason. */ > +#include "common/enum_flags.h" > > #if defined (HAVE_LIBIPT) > # include > @@ -58,6 +59,7 @@ enum btrace_insn_flag > /* The instruction has been executed speculatively. */ > BTRACE_INSN_FLAG_SPECULATIVE =3D (1 << 0) > }; > +DEF_ENUM_FLAGS_TYPE (enum btrace_insn_flag, btrace_insn_flags); > > /* A branch trace instruction. > > @@ -74,7 +76,7 @@ struct btrace_insn > enum btrace_insn_class iclass; > > /* A bit vector of BTRACE_INSN_FLAGS. */ > - enum btrace_insn_flag flags; > + btrace_insn_flags flags; > }; > > /* A vector of branch trace instructions. */ > @@ -100,6 +102,7 @@ enum btrace_function_flag > if bfun_up_links_to_ret is clear. */ > BFUN_UP_LINKS_TO_TAILCALL =3D (1 << 1) > }; > +DEF_ENUM_FLAGS_TYPE (enum btrace_function_flag, btrace_function_flags); > > /* Decode errors for the BTS recording format. */ > enum btrace_bts_error > @@ -181,7 +184,7 @@ struct btrace_function > int level; > > /* A bit-vector of btrace_function_flag. */ > - enum btrace_function_flag flags; > + btrace_function_flags flags; > }; > > /* A branch trace instruction iterator. */ > @@ -245,6 +248,7 @@ enum btrace_thread_flag > /* The thread is to be stopped. */ > BTHR_STOP =3D (1 << 4) > }; > +DEF_ENUM_FLAGS_TYPE (enum btrace_thread_flag, btrace_thread_flags); > > #if defined (HAVE_LIBIPT) > /* A packet. */ > @@ -342,7 +346,7 @@ struct btrace_thread_info > unsigned int ngaps; > > /* A bit-vector of btrace_thread_flag. */ > - enum btrace_thread_flag flags; > + btrace_thread_flags flags; > > /* The instruction history iterator. */ > struct btrace_insn_history *insn_history; > diff --git a/gdb/c-exp.y b/gdb/c-exp.y > index d093491..26050cb 100644 > --- a/gdb/c-exp.y > +++ b/gdb/c-exp.y > @@ -2257,7 +2257,7 @@ parse_string_or_char (const char *tokptr, const cha= r **outptr, > > /* This is used to associate some attributes with a token. */ > > -enum token_flags > +enum token_flag > { > /* If this bit is set, the token is C++-only. */ > > @@ -2269,13 +2269,14 @@ enum token_flags > > FLAG_SHADOW =3D 2 > }; > +DEF_ENUM_FLAGS_TYPE (enum token_flag, token_flags); > > struct token > { > char *oper; > int token; > enum exp_opcode opcode; > - enum token_flags flags; > + token_flags flags; > }; > > static const struct token tokentab3[] =3D > diff --git a/gdb/common/enum_flags.h b/gdb/common/enum_flags.h > new file mode 100644 > index 0000000..030fd2f > --- /dev/null > +++ b/gdb/common/enum_flags.h > @@ -0,0 +1,211 @@ > +/* Copyright (C) 2015 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see .= */ > + > +#ifndef COMMON_ENUM_FLAGS_H > +#define COMMON_ENUM_FLAGS_H > + > +/* Type-safe wrapper for enum flags. enum flags are enums where the > + values are bits that are meant to be ORed together. > + > + This allows writing code like the below, while with raw enums this > + would fail to compile without casts to enum type at the assignments > + to 'f': > + > + enum some_flag > + { > + flag_val1 =3D 1 << 1, > + flag_val2 =3D 1 << 2, > + flag_val3 =3D 1 << 3, > + flag_val4 =3D 1 << 4, > + }; > + DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags) > + > + some_flags f =3D flag_val1 | flag_val2; > + f |=3D flag_val3; > + > + It's also possible to assign literal zero to an enum flags variable > + (meaning, no flags), dispensing adding an awkward explicit "no > + value" value to the enumeration. For example: > + > + some_flags f =3D 0; > + f |=3D flag_val3 | flag_val4; > + > + Note that literal integers other than zero fail to compile: > + > + some_flags f =3D 1; // error > +*/ > + > +#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 > + 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; \ > + } > + > +/* Until we can rely on std::underlying type being universally > + available (C++11), roll our own for enums. */ > +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 > +{ > + typedef typename > + integer_for_size(T (-1) < T (0))>::type > + type; > +}; > + > +template > +class enum_flags > +{ > +public: > + typedef E enum_type; > + typedef typename enum_underlying_type::type underlying_type; > + > +private: > + /* Private type used to support initializing flag types with zero: > + > + foo_flags f =3D 0; > + > + but not other integers: > + > + foo_flags f =3D 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; > + > + underlying_type > + underlying_value () const > + { > + return enum_value; > + } > + > +public: > + /* Allow default construction, just like raw enums. */ > + enum_flags () > + {} Why not zero-initialize enum_value here? Given that enum_flags represents a bitwise OR of enum_type, it seems reasonable that its default value would be zero rather than uninitialzied. > + > + enum_flags (const enum_flags &other) > + : enum_value (other.enum_value) > + {} > + > + enum_flags &operator=3D (const enum_flags &other) > + { > + enum_value =3D other.enum_value; > + return *this; > + } > + > + /* 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) > + : enum_value (e) > + {} > + enum_flags (struct enum_flags::zero_type *zero) > + : enum_value ((enum_type) 0) > + {} > + > + enum_flags &operator&=3D (enum_type e) > + { > + enum_value =3D (enum_type) (underlying_value () & e); > + return *this; > + } > + enum_flags &operator|=3D (enum_type e) > + { > + enum_value =3D (enum_type) (underlying_value () | e); > + return *this; > + } > + enum_flags &operator^=3D (enum_type e) > + { > + enum_value =3D (enum_type) (underlying_value () ^ e); > + return *this; > + } > + > + operator enum_type () const > + { > + return 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 > + { > + return (enum_type) ~underlying_value (); > + } > + > +private: > + /* Stored as enum_type because GDB knows to print the bit flags > + neatly if the enum values look like bit flags. */ > + enum_type enum_value; > +}; > + > +/* Global operator overloads. */ > + > +template > +typename enum_flags_type::type > +operator| (enum_type e1, enum_type e2) > +{ > + return enum_flags (e1) | e2; > +} > + > +template > +typename enum_flags_type::type > +operator~ (enum_type e) > +{ > + return ~enum_flags (e); > +} What are global operator^ and operator& omitted?