Re: Sharing gdb's enum-flags.h with gcc? (was Re: [PATCH 01/10] Convert dump and optgroup flags to enums)
On June 5, 2018 4:49:21 PM GMT+02:00, David Malcolm wrote:
>On Tue, 2018-06-05 at 04:40 -0400, Trevor Saunders wrote:
>> On Fri, Jun 01, 2018 at 12:00:09PM +0200, Richard Biener wrote:
>> > On Tue, May 29, 2018 at 10:32 PM David Malcolm > > > wrote:
>> > >
>> > > The dump machinery uses "int" in a few places, for two different
>> > > sets of bitmasks.
>> > >
>> > > This patch makes things more self-documenting and type-safe by
>> > > using
>> > > a new pair of enums: one for the dump_flags_t and another for the
>> > > optgroup_flags.
>> >
>> > Great! This should also make them accessible in gdb w/o using -g3.
>> >
>> > > This requires adding some overloaded bit operations to the enums
>> > > in question, which, in this patch is done for each enum . If the
>> > > basic
>> > > idea is OK, should I add a template for this? (with some kind of
>> > > magic to express that bitmasking operations are only supported on
>> > > certain opt-in enums).
>>
>> You may want to look at gdb's enum-flags.h which I think already
>> implements what your doing here.
>
>Aha! Thanks!
>
>Browsing the git web UI, that gdb header was introduced by Pedro Alves
>in:
>https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=8d297bbf604c8318ffc72d5a7b3db654409c5ed9
>and the current state is here:
>https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/common/enum-flags.h;hb=HEAD
>
>I'll try this out with GCC; hopefully it works with C++98.
>
>Presumably it would be good to share this header between GCC and GDB;
>CCing Pedro; Pedro: hi! Does this sound sane?
>(for reference, the GCC patch we're discussing here is:
> https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01685.html )
>
>Presumably gcc's copy should live in the gcc's top-level "include"
>subdirectory?
>
>Would we need to change that "This file is part of GDB." comment, and
>the include guards' "COMMON_ENUM_FLAGS_H"?
>
>> > Does C++ allow > int enums? I think we want some way of knowing
>> > when either enum exceeds int (dump_flags_t was already uint64_t
>> > but you now make it effectively int again). That is, general
>> > wrapping
>> > for enum ops should chose an appropriate unsigned integer for
>> > the operation. So yes, a common implementation looks useful to me.
>>
>> I don't remember very well, but istr C++ will actually use a 8 byte
>> integer if the enum contains constants larger than 2^32. Testing
>> sizeof enum x { A =0x4 }; gives the desired thing for me, but
>> it
>> would still be good to check the standard.
>
>FWIW C++11 onwards has a std::underlying_type for enums:
> http://en.cppreference.com/w/cpp/types/underlying_type
>(GCC is on C++98). The gdb header seems to emulate this via the use of
>sizeof(T) to select an appropriate integer_for_size specialization and
>thus the appropriate struct enum_underlying_type specialization (if I'm
>reading it right).
>
>> Trev
>>
>>
>> >
>> > I think this patch is independently useful.
>
>Richard: by this, did you mean that the patch is OK for trunk as-is?
Yes.
Richard.
>(keeping a more general bitflags enum patch as followup work) Note to
>self: still needs bootstrap-and-testing.
>
>> > Thanks,
>> > Richard.
>
>Thanks
>Dave
Re: Sharing gdb's enum-flags.h with gcc? (was Re: [PATCH 01/10] Convert dump and optgroup flags to enums)
On 06/05/2018 03:49 PM, David Malcolm wrote: > On Tue, 2018-06-05 at 04:40 -0400, Trevor Saunders wrote: >> You may want to look at gdb's enum-flags.h which I think already >> implements what your doing here. > > Aha! Thanks! > > Browsing the git web UI, that gdb header was introduced by Pedro Alves > in: > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=8d297bbf604c8318ffc72d5a7b3db654409c5ed9 > and the current state is here: > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/common/enum-flags.h;hb=HEAD > > I'll try this out with GCC; hopefully it works with C++98. It should -- it was written/added while GDB still required C++98. Note I have a WIP series here: https://github.com/palves/gdb/commits/palves/cxx11-enum-flags that fixes a few things, and adds a bunch of unit tests. In the process, it uses C++11 constructs (hence the branch's name), but I think that can be reverted to still work with C++98. I had seen some noises recently about GCC maybe considering requiring C++11. Is that reasonable to expect in this cycle? (GDB requires GCC 4.8 or later, FWIW.) Getting that branch into master had fallen lower on my TODO, but if there's interest, I can try to finish it off soon, so we can share a better baseline. (I posted it once, but found some issues which I fixed since but never managed to repost.) > > Presumably it would be good to share this header between GCC and GDB; > CCing Pedro; Pedro: hi! Does this sound sane? > (for reference, the GCC patch we're discussing here is: > https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01685.html ) Sure! Thanks, Pedro Alves
Sharing gdb's enum-flags.h with gcc? (was Re: [PATCH 01/10] Convert dump and optgroup flags to enums)
On Tue, 2018-06-05 at 04:40 -0400, Trevor Saunders wrote:
> On Fri, Jun 01, 2018 at 12:00:09PM +0200, Richard Biener wrote:
> > On Tue, May 29, 2018 at 10:32 PM David Malcolm > > wrote:
> > >
> > > The dump machinery uses "int" in a few places, for two different
> > > sets of bitmasks.
> > >
> > > This patch makes things more self-documenting and type-safe by
> > > using
> > > a new pair of enums: one for the dump_flags_t and another for the
> > > optgroup_flags.
> >
> > Great! This should also make them accessible in gdb w/o using -g3.
> >
> > > This requires adding some overloaded bit operations to the enums
> > > in question, which, in this patch is done for each enum . If the
> > > basic
> > > idea is OK, should I add a template for this? (with some kind of
> > > magic to express that bitmasking operations are only supported on
> > > certain opt-in enums).
>
> You may want to look at gdb's enum-flags.h which I think already
> implements what your doing here.
Aha! Thanks!
Browsing the git web UI, that gdb header was introduced by Pedro Alves
in:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=8d297bbf604c8318ffc72d5a7b3db654409c5ed9
and the current state is here:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/common/enum-flags.h;hb=HEAD
I'll try this out with GCC; hopefully it works with C++98.
Presumably it would be good to share this header between GCC and GDB;
CCing Pedro; Pedro: hi! Does this sound sane?
(for reference, the GCC patch we're discussing here is:
https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01685.html )
Presumably gcc's copy should live in the gcc's top-level "include"
subdirectory?
Would we need to change that "This file is part of GDB." comment, and
the include guards' "COMMON_ENUM_FLAGS_H"?
> > Does C++ allow > int enums? I think we want some way of knowing
> > when either enum exceeds int (dump_flags_t was already uint64_t
> > but you now make it effectively int again). That is, general
> > wrapping
> > for enum ops should chose an appropriate unsigned integer for
> > the operation. So yes, a common implementation looks useful to me.
>
> I don't remember very well, but istr C++ will actually use a 8 byte
> integer if the enum contains constants larger than 2^32. Testing
> sizeof enum x { A =0x4 }; gives the desired thing for me, but
> it
> would still be good to check the standard.
FWIW C++11 onwards has a std::underlying_type for enums:
http://en.cppreference.com/w/cpp/types/underlying_type
(GCC is on C++98). The gdb header seems to emulate this via the use of
sizeof(T) to select an appropriate integer_for_size specialization and
thus the appropriate struct enum_underlying_type specialization (if I'm
reading it right).
> Trev
>
>
> >
> > I think this patch is independently useful.
Richard: by this, did you mean that the patch is OK for trunk as-is?
(keeping a more general bitflags enum patch as followup work) Note to
self: still needs bootstrap-and-testing.
> > Thanks,
> > Richard.
Thanks
Dave
Re: [PATCH 01/10] Convert dump and optgroup flags to enums
On Fri, Jun 01, 2018 at 12:00:09PM +0200, Richard Biener wrote:
> On Tue, May 29, 2018 at 10:32 PM David Malcolm wrote:
> >
> > The dump machinery uses "int" in a few places, for two different
> > sets of bitmasks.
> >
> > This patch makes things more self-documenting and type-safe by using
> > a new pair of enums: one for the dump_flags_t and another for the
> > optgroup_flags.
>
> Great! This should also make them accessible in gdb w/o using -g3.
>
> > This requires adding some overloaded bit operations to the enums
> > in question, which, in this patch is done for each enum . If the basic
> > idea is OK, should I add a template for this? (with some kind of
> > magic to express that bitmasking operations are only supported on
> > certain opt-in enums).
You may want to look at gdb's enum-flags.h which I think already
implements what your doing here.
>
> Does C++ allow > int enums? I think we want some way of knowing
> when either enum exceeds int (dump_flags_t was already uint64_t
> but you now make it effectively int again). That is, general wrapping
> for enum ops should chose an appropriate unsigned integer for
> the operation. So yes, a common implementation looks useful to me.
I don't remember very well, but istr C++ will actually use a 8 byte
integer if the enum contains constants larger than 2^32. Testing
sizeof enum x { A =0x4 }; gives the desired thing for me, but it
would still be good to check the standard.
Trev
>
> I think this patch is independently useful.
>
> Thanks,
> Richard.
>
> >
Re: [PATCH 01/10] Convert dump and optgroup flags to enums
On Tue, May 29, 2018 at 10:32 PM David Malcolm wrote: > > The dump machinery uses "int" in a few places, for two different > sets of bitmasks. > > This patch makes things more self-documenting and type-safe by using > a new pair of enums: one for the dump_flags_t and another for the > optgroup_flags. Great! This should also make them accessible in gdb w/o using -g3. > This requires adding some overloaded bit operations to the enums > in question, which, in this patch is done for each enum . If the basic > idea is OK, should I add a template for this? (with some kind of > magic to express that bitmasking operations are only supported on > certain opt-in enums). Does C++ allow > int enums? I think we want some way of knowing when either enum exceeds int (dump_flags_t was already uint64_t but you now make it effectively int again). That is, general wrapping for enum ops should chose an appropriate unsigned integer for the operation. So yes, a common implementation looks useful to me. I think this patch is independently useful. Thanks, Richard. > > gcc/c-family/ChangeLog: > * c-pretty-print.c (c_pretty_printer::statement): Use TDF_NONE > rather than 0. > > gcc/ChangeLog: > * cfg.c (debug): Use TDF_NONE rather than 0. > * cfghooks.c (debug): Likewise. > * dumpfile.c (DUMP_FILE_INFO): Likewise; also for OPTGROUP. > (struct dump_option_value_info): Convert to... > (struct kv_pair): ...this template type. > (dump_options): Convert to kv_pair; use TDF_NONE > rather than 0. > (optinfo_verbosity_options): Likewise. > (optgroup_options): Convert to kv_pair; use > OPTGROUP_NONE. > (gcc::dump_manager::dump_register): Use optgroup_flags_t rather > than int for "optgroup_flags" param. > (dump_generic_expr_loc): Use dump_flags_t rather than int for > "dump_kind" param. > (dump_finish): Use TDF_NONE rather than 0. > (gcc::dump_manager::opt_info_enable_passes): Use optgroup_flags_t > rather than int for "optgroup_flags" param. Use TDF_NONE rather > than 0. Update for change to option_ptr. > (opt_info_switch_p_1): Convert "optgroup_flags" param from int * > to optgroup_flags_t *. Use TDF_NONE and OPTGROUP_NONE rather than > 0. Update for changes to optinfo_verbosity_options and > optgroup_options. > (opt_info_switch_p): Convert optgroup_flags from int to > optgroup_flags_t. > * dumpfile.h (TDF_ADDRESS, TDF_SLIM, TDF_RAW, TDF_DETAILS, > TDF_STATS, TDF_BLOCKS, TDF_VOPS, TDF_LINENO, TDF_UID) > TDF_STMTADDR, TDF_GRAPH, TDF_MEMSYMS, TDF_RHS_ONLY, TDF_ASMNAME, > TDF_EH, TDF_NOUID, TDF_ALIAS, TDF_ENUMERATE_LOCALS, TDF_CSELIB, > TDF_SCEV, TDF_GIMPLE, TDF_FOLDING, MSG_OPTIMIZED_LOCATIONS, > MSG_MISSED_OPTIMIZATION, MSG_NOTE, MSG_ALL, TDF_COMPARE_DEBUG, > TDF_NONE): Convert from macros to... > (enum dump_flag): ...this new enum. > (dump_flags_t): Update to use enum. > (operator|, operator&, operator~, operator|=, operator&=): > Implement for dump_flags_t. > (OPTGROUP_NONE, OPTGROUP_IPA, OPTGROUP_LOOP, OPTGROUP_INLINE, > OPTGROUP_OMP, OPTGROUP_VEC, OPTGROUP_OTHER, OPTGROUP_ALL): > Convert from macros to... > (enum optgroup_flag): ...this new enum. > (optgroup_flags_t): New typedef. > (operator|, operator|=): Implement for optgroup_flags_t. > (struct dump_file_info): Convert field "alt_flags" to > dump_flags_t. Convert field "optgroup_flags" to > optgroup_flags_t. > (dump_register): Convert param "optgroup_flags" to > optgroup_flags_t. > (opt_info_enable_passes): Likewise. > * early-remat.c (early_remat::dump_edge_list): Use TDF_NONE rather > than 0. > * gimple-pretty-print.c (debug): Likewise. > * gimple-ssa-store-merging.c (bswap_replace): Likewise. > (merged_store_group::apply_stores): Likewise. > * gimple-ssa-strength-reduction.c (insert_initializers): Likewise. > * gimple.c (verify_gimple_pp): Likewise. > * passes.c (pass_manager::register_one_dump_file): Convert > local "optgroup_flags" to optgroup_flags_t. > * print-tree.c (print_node): Use TDF_NONE rather than 0. > (debug): Likewise. > (debug_body): Likewise. > * tree-pass.h (struct pass_data): Convert field "optgroup_flags" > to optgroup_flags_t. > * tree-pretty-print.c (print_struct_decl): Use TDF_NONE rather > than 0. > * tree-ssa-math-opts.c (convert_mult_to_fma_1): Likewise. > (convert_mult_to_fma): Likewise. > * tree-ssa-reassoc.c (undistribute_ops_list): Likewise. > * tree-ssa-sccvn.c (vn_eliminate): Likewise. > * tree-vect-data-refs.c (dump_lower_bound): Convert param > "dump_kind" to
[PATCH 01/10] Convert dump and optgroup flags to enums
The dump machinery uses "int" in a few places, for two different sets of bitmasks. This patch makes things more self-documenting and type-safe by using a new pair of enums: one for the dump_flags_t and another for the optgroup_flags. This requires adding some overloaded bit operations to the enums in question, which, in this patch is done for each enum . If the basic idea is OK, should I add a template for this? (with some kind of magic to express that bitmasking operations are only supported on certain opt-in enums). gcc/c-family/ChangeLog: * c-pretty-print.c (c_pretty_printer::statement): Use TDF_NONE rather than 0. gcc/ChangeLog: * cfg.c (debug): Use TDF_NONE rather than 0. * cfghooks.c (debug): Likewise. * dumpfile.c (DUMP_FILE_INFO): Likewise; also for OPTGROUP. (struct dump_option_value_info): Convert to... (struct kv_pair): ...this template type. (dump_options): Convert to kv_pair; use TDF_NONE rather than 0. (optinfo_verbosity_options): Likewise. (optgroup_options): Convert to kv_pair; use OPTGROUP_NONE. (gcc::dump_manager::dump_register): Use optgroup_flags_t rather than int for "optgroup_flags" param. (dump_generic_expr_loc): Use dump_flags_t rather than int for "dump_kind" param. (dump_finish): Use TDF_NONE rather than 0. (gcc::dump_manager::opt_info_enable_passes): Use optgroup_flags_t rather than int for "optgroup_flags" param. Use TDF_NONE rather than 0. Update for change to option_ptr. (opt_info_switch_p_1): Convert "optgroup_flags" param from int * to optgroup_flags_t *. Use TDF_NONE and OPTGROUP_NONE rather than 0. Update for changes to optinfo_verbosity_options and optgroup_options. (opt_info_switch_p): Convert optgroup_flags from int to optgroup_flags_t. * dumpfile.h (TDF_ADDRESS, TDF_SLIM, TDF_RAW, TDF_DETAILS, TDF_STATS, TDF_BLOCKS, TDF_VOPS, TDF_LINENO, TDF_UID) TDF_STMTADDR, TDF_GRAPH, TDF_MEMSYMS, TDF_RHS_ONLY, TDF_ASMNAME, TDF_EH, TDF_NOUID, TDF_ALIAS, TDF_ENUMERATE_LOCALS, TDF_CSELIB, TDF_SCEV, TDF_GIMPLE, TDF_FOLDING, MSG_OPTIMIZED_LOCATIONS, MSG_MISSED_OPTIMIZATION, MSG_NOTE, MSG_ALL, TDF_COMPARE_DEBUG, TDF_NONE): Convert from macros to... (enum dump_flag): ...this new enum. (dump_flags_t): Update to use enum. (operator|, operator&, operator~, operator|=, operator&=): Implement for dump_flags_t. (OPTGROUP_NONE, OPTGROUP_IPA, OPTGROUP_LOOP, OPTGROUP_INLINE, OPTGROUP_OMP, OPTGROUP_VEC, OPTGROUP_OTHER, OPTGROUP_ALL): Convert from macros to... (enum optgroup_flag): ...this new enum. (optgroup_flags_t): New typedef. (operator|, operator|=): Implement for optgroup_flags_t. (struct dump_file_info): Convert field "alt_flags" to dump_flags_t. Convert field "optgroup_flags" to optgroup_flags_t. (dump_register): Convert param "optgroup_flags" to optgroup_flags_t. (opt_info_enable_passes): Likewise. * early-remat.c (early_remat::dump_edge_list): Use TDF_NONE rather than 0. * gimple-pretty-print.c (debug): Likewise. * gimple-ssa-store-merging.c (bswap_replace): Likewise. (merged_store_group::apply_stores): Likewise. * gimple-ssa-strength-reduction.c (insert_initializers): Likewise. * gimple.c (verify_gimple_pp): Likewise. * passes.c (pass_manager::register_one_dump_file): Convert local "optgroup_flags" to optgroup_flags_t. * print-tree.c (print_node): Use TDF_NONE rather than 0. (debug): Likewise. (debug_body): Likewise. * tree-pass.h (struct pass_data): Convert field "optgroup_flags" to optgroup_flags_t. * tree-pretty-print.c (print_struct_decl): Use TDF_NONE rather than 0. * tree-ssa-math-opts.c (convert_mult_to_fma_1): Likewise. (convert_mult_to_fma): Likewise. * tree-ssa-reassoc.c (undistribute_ops_list): Likewise. * tree-ssa-sccvn.c (vn_eliminate): Likewise. * tree-vect-data-refs.c (dump_lower_bound): Convert param "dump_kind" to dump_flags_t. --- gcc/c-family/c-pretty-print.c | 2 +- gcc/cfg.c | 4 +- gcc/cfghooks.c | 2 +- gcc/dumpfile.c | 56 - gcc/dumpfile.h | 227 +++- gcc/early-remat.c | 2 +- gcc/gimple-pretty-print.c | 2 +- gcc/gimple-ssa-store-merging.c | 6 +- gcc/gimple-ssa-strength-reduction.c | 2 +- gcc/gimple.c| 2 +- gcc/passes.c| 2 +- gcc/print-tree.c| 7 +- gcc/tree-pass.h | 2 +- gcc/tree-pretty-print.c | 2
