This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Type-safe wrapper for enum flags


On Thu, Oct 29, 2015 at 2:45 PM, Pedro Alves <palves@redhat.com> 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 âintâ to âfilterflagsâ [-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 = 0;
>
>   if (...)
>     flags |= FOO_FLAG1;
>   if (...)
>     flags |= FOO_FLAG2;
>
> ... would have to be written as:
>
>   enum foo_flags flags = (enum foo_flags) 0;
>
>   if (...)
>     flags = (enum foo_flags) (flags | FOO_FLAG1);
>   if (...)
>     flags = (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 = 1 << 1,
>      flag_val2 = 1 << 2,
>      flag_val3 = 1 << 3,
>      flag_val4 = 1 << 4,
>   };
>   DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags)
>
>   some_flags f = flag_val1 | flag_val2;
>   f |= 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 = 0;
>   f |= flag_val3 | flag_val4;
>
> Note that literal integers other than zero do fail to compile:
>
>   some_flags f = 1; // error
>
> C is still supported -- DEF_ENUM_FLAGS_TYPE is just a typedef in that
> case.
>
> gdb/ChangeLog:
> 2015-10-29  Pedro Alves  <palves@redhat.com>
>
>         * btrace.h: Include common/enum_flags.h.
>         (btrace_insn_flags): Define.
>         (struct btrace_insn) <flags>: Change type.
>         (btrace_function_flags): Define.
>         (struct btrace_function) <flags>: Change type.
>         (btrace_thread_flags): Define.
>         (struct btrace_thread_info) <flags>: Change type.
>         * c-exp.y (token_flags): Rename to ...
>         (token_flag): ... this.
>         (token_flags): Define.
>         (struct token) <flags>: 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 <intel-pt.h>
> @@ -58,6 +59,7 @@ enum btrace_insn_flag
>    /* The instruction has been executed speculatively.  */
>    BTRACE_INSN_FLAG_SPECULATIVE = (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 = (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 = (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 char **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 = 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[] =
> 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 <http://www.gnu.org/licenses/>.  */
> +
> +#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 = 1 << 1,
> +       flag_val2 = 1 << 2,
> +       flag_val3 = 1 << 3,
> +       flag_val4 = 1 << 4,
> +    };
> +    DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags)
> +
> +    some_flags f = flag_val1 | flag_val2;
> +    f |= 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 = 0;
> +    f |= flag_val3 | flag_val4;
> +
> +   Note that literal integers other than zero fail to compile:
> +
> +    some_flags f = 1; // error
> +*/
> +
> +#ifdef __cplusplus
> +
> +/* Traits type used to prevent the global operator overloads from
> +   instantiating for non-flag enums.  */
> +template<typename T> 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<enum_type> flags_type;            \
> +  template<>                                           \
> +  struct enum_flags_type<enum_type>                    \
> +  {                                                    \
> +    typedef enum_flags<enum_type> type;                        \
> +  }
> +
> +/* Until we can rely on std::underlying type being universally
> +   available (C++11), roll our own for enums.  */
> +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
> +{
> +  typedef typename
> +    integer_for_size<sizeof (T), static_cast<bool>(T (-1) < T (0))>::type
> +    type;
> +};
> +
> +template <typename E>
> +class enum_flags
> +{
> +public:
> +  typedef E enum_type;
> +  typedef typename enum_underlying_type<enum_type>::type underlying_type;
> +
> +private:
> +  /* Private type used to support initializing flag types with zero:
> +
> +       foo_flags f = 0;
> +
> +     but not other integers:
> +
> +       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;
> +
> +  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= (const enum_flags &other)
> +  {
> +    enum_value = 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&= (enum_type e)
> +  {
> +    enum_value = (enum_type) (underlying_value () & e);
> +    return *this;
> +  }
> +  enum_flags &operator|= (enum_type e)
> +  {
> +    enum_value = (enum_type) (underlying_value () | e);
> +    return *this;
> +  }
> +  enum_flags &operator^= (enum_type e)
> +  {
> +    enum_value = (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_type>
> +typename enum_flags_type<enum_type>::type
> +operator| (enum_type e1, enum_type e2)
> +{
> +  return enum_flags<enum_type> (e1) | e2;
> +}
> +
> +template <typename enum_type>
> +typename enum_flags_type<enum_type>::type
> +operator~ (enum_type e)
> +{
> +  return ~enum_flags<enum_type> (e);
> +}

What are global operator^ and operator& omitted?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]