Re: [PATCH 5/5] Formatted printing for dump_* in the middle-end
On Tue, 31 Jul 2018, David Malcolm wrote: > I didn't exhaustively check every callsite to the changed calls; I'm > assuming that -Wformat during bootstrap has effectively checked that > for me. Though now I think about it, I note that we use > HOST_WIDE_INT_PRINT_DEC in many places: is this guaranteed to be a > valid input to pp_format on all of our configurations? HOST_WIDE_INT_PRINT_DEC should not be considered safe with pp_format (although since r197049 may have effectively stopped using %I64 on MinGW hosts, I'm not sure if there are current cases where it won't work). Rather, it is the job of pp_format to map the 'w' length specifier to HOST_WIDE_INT_PRINT_DEC etc. I think it clearly makes for cleaner code to limit use of HOST_WIDE_INT_PRINT_* to as few places as possible and to prefer use of internal printf-like functions that accept formats such as %wd where possible. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH 5/5] Formatted printing for dump_* in the middle-end
On Tue, Jul 31, 2018 at 4:21 PM Richard Biener
wrote:
>
> On Tue, Jul 31, 2018 at 4:19 PM David Malcolm wrote:
> >
> > On Tue, 2018-07-31 at 15:03 +0200, Richard Biener wrote:
> > > On Fri, Jul 27, 2018 at 11:49 PM David Malcolm
> > > wrote:
> > > >
> > > > This patch converts dump_print and dump_printf_loc from using
> > > > printf (and thus ATTRIBUTE_PRINTF) to using a new pretty-printer
> > > > based on pp_format, which supports formatting middle-end types.
> > > >
> > > > In particular, the following codes are implemented (in addition
> > > > to the standard pretty_printer ones):
> > > >
> > > >%E: gimple *:
> > > >Equivalent to: dump_gimple_expr (MSG_*, TDF_SLIM, stmt, 0)
> > > >%G: gimple *:
> > > >Equivalent to: dump_gimple_stmt (MSG_*, TDF_SLIM, stmt, 0)
> > > >%T: tree:
> > > >Equivalent to: dump_generic_expr (MSG_*, arg, TDF_SLIM).
> > > >
> > > > Hence it becomes possible to convert e.g.:
> > > >
> > > > if (dump_enabled_p ())
> > > > {
> > > > dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> > > >"not vectorized: different sized vector "
> > > >"types in statement, ");
> > > > dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> > > > vectype);
> > > > dump_printf (MSG_MISSED_OPTIMIZATION, " and ");
> > > > dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> > > > nunits_vectype);
> > > > dump_printf (MSG_MISSED_OPTIMIZATION, "\n");
> > > > }
> > > >
> > > > into a one-liner:
> > > >
> > > > if (dump_enabled_p ())
> > > > dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> > > > "not vectorized: different sized vector "
> > > > "types in statement, %T and %T\n",
> > > > vectype, nunits_vectype);
> > > >
> > > > Unlike regular pretty-printers, this one captures optinfo_item
> > > > instances for the formatted chunks as appropriate, so that when
> > > > written out to a JSON optimization record, the relevant parts of
> > > > the message are labelled by type, and by source location (so that
> > > > e.g. %G is entirely equivalent to using dump_gimple_stmt).
> > > >
> > > > dump_printf and dump_printf_loc become marked with
> > > > ATTRIBUTE_GCC_DUMP_PRINTF, which the patch also implements.
> > > >
> > > > gcc/c-family/ChangeLog:
> > > > * c-format.c (enum format_type): Add
> > > > gcc_dump_printf_format_type.
> > > > (local_gimple_ptr_node): New decl.
> > > > (gcc_dump_printf_length_specs): New.
> > > > (gcc_dump_printf_flag_pairs): New.
> > > > (gcc_dump_printf_flag_specs): New.
> > > > (gcc_dump_printf_char_table): New.
> > > > (format_types_orig): Add entry for "gcc_dump_printf".
> > > > (init_dynamic_diag_info): Create local_gimple_ptr_node.
> > > > Set up length_char_specs and conversion_specs for
> > > > gcc_dump_printf_format_type.
> > > > (handle_format_attribute): Handle
> > > > gcc_dump_printf_format_type.
> > > > * c-format.h (T89_GIMPLE): New macro.
> > >
> > > Iff the c-family changes are neccessary (are they?) then how does
> > > this
> > > work for non-c-family languages which do not link c-family/c-
> > > format.o?
> >
> > The c-family changes are necessary for bootstrap, so that -Wformat
> > works cleanly after changing dump_printf_loc etc from
> > ATTRIBUTE_PRINTF_3;
> > to
> > ATTRIBUTE_GCC_DUMP_PRINTF (3, 0);
> > i.e. they're just the changes to -Wformat to teach it how to verify the
> > new:
> > __attribute__ ((__format__ (__gcc_dump_printf__, m ,n)))
> > (hence the cleanups to c-format.c earlier in the patch kit, to avoid
> > yet more copy-and-paste there for the new format decoder callback).
>
> Ah, thanks for clarifying.
>
> > The implementation itself is all within dumpfile.c, hence the non-c-
> > family languages ought to work. My testing was with:
> > --enable-languages=c,c++,objc,obj-c++,fortran,ada,go,lto,jit,brig
> > (and with cloog and isl, fwiw).
> >
> > (I kept the alphabetization of the ChangeLog files from my generate-
> > changelog.py script, which put the gcc/c-family/ChangeLog before the
> > gcc/ChangeLog and thus may have made this confusing to read, sorry).
> >
> > I didn't exhaustively check every callsite to the changed calls; I'm
> > assuming that -Wformat during bootstrap has effectively checked that
> > for me. Though now I think about it, I note that we use
> > HOST_WIDE_INT_PRINT_DEC in many places: is this guaranteed to be a
> > valid input to pp_format on all of our configurations?
>
> I hope so ... returning to the patch now.
The patch is OK if C family maintainers agree on their parts.
Thanks,
Richard.
> Richard.
>
> > Dave
> >
> > >
> > > > gcc/ChangeLog:
> > > > * dump-context.h: Include "dumpfile.h".
> > > > (dump_context::dump_printf_va): Convert final param from
> > > > va_list
> > > > to va_list
Re: [PATCH 5/5] Formatted printing for dump_* in the middle-end
On Tue, Jul 31, 2018 at 4:19 PM David Malcolm wrote:
>
> On Tue, 2018-07-31 at 15:03 +0200, Richard Biener wrote:
> > On Fri, Jul 27, 2018 at 11:49 PM David Malcolm
> > wrote:
> > >
> > > This patch converts dump_print and dump_printf_loc from using
> > > printf (and thus ATTRIBUTE_PRINTF) to using a new pretty-printer
> > > based on pp_format, which supports formatting middle-end types.
> > >
> > > In particular, the following codes are implemented (in addition
> > > to the standard pretty_printer ones):
> > >
> > >%E: gimple *:
> > >Equivalent to: dump_gimple_expr (MSG_*, TDF_SLIM, stmt, 0)
> > >%G: gimple *:
> > >Equivalent to: dump_gimple_stmt (MSG_*, TDF_SLIM, stmt, 0)
> > >%T: tree:
> > >Equivalent to: dump_generic_expr (MSG_*, arg, TDF_SLIM).
> > >
> > > Hence it becomes possible to convert e.g.:
> > >
> > > if (dump_enabled_p ())
> > > {
> > > dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> > >"not vectorized: different sized vector "
> > >"types in statement, ");
> > > dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> > > vectype);
> > > dump_printf (MSG_MISSED_OPTIMIZATION, " and ");
> > > dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> > > nunits_vectype);
> > > dump_printf (MSG_MISSED_OPTIMIZATION, "\n");
> > > }
> > >
> > > into a one-liner:
> > >
> > > if (dump_enabled_p ())
> > > dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> > > "not vectorized: different sized vector "
> > > "types in statement, %T and %T\n",
> > > vectype, nunits_vectype);
> > >
> > > Unlike regular pretty-printers, this one captures optinfo_item
> > > instances for the formatted chunks as appropriate, so that when
> > > written out to a JSON optimization record, the relevant parts of
> > > the message are labelled by type, and by source location (so that
> > > e.g. %G is entirely equivalent to using dump_gimple_stmt).
> > >
> > > dump_printf and dump_printf_loc become marked with
> > > ATTRIBUTE_GCC_DUMP_PRINTF, which the patch also implements.
> > >
> > > gcc/c-family/ChangeLog:
> > > * c-format.c (enum format_type): Add
> > > gcc_dump_printf_format_type.
> > > (local_gimple_ptr_node): New decl.
> > > (gcc_dump_printf_length_specs): New.
> > > (gcc_dump_printf_flag_pairs): New.
> > > (gcc_dump_printf_flag_specs): New.
> > > (gcc_dump_printf_char_table): New.
> > > (format_types_orig): Add entry for "gcc_dump_printf".
> > > (init_dynamic_diag_info): Create local_gimple_ptr_node.
> > > Set up length_char_specs and conversion_specs for
> > > gcc_dump_printf_format_type.
> > > (handle_format_attribute): Handle
> > > gcc_dump_printf_format_type.
> > > * c-format.h (T89_GIMPLE): New macro.
> >
> > Iff the c-family changes are neccessary (are they?) then how does
> > this
> > work for non-c-family languages which do not link c-family/c-
> > format.o?
>
> The c-family changes are necessary for bootstrap, so that -Wformat
> works cleanly after changing dump_printf_loc etc from
> ATTRIBUTE_PRINTF_3;
> to
> ATTRIBUTE_GCC_DUMP_PRINTF (3, 0);
> i.e. they're just the changes to -Wformat to teach it how to verify the
> new:
> __attribute__ ((__format__ (__gcc_dump_printf__, m ,n)))
> (hence the cleanups to c-format.c earlier in the patch kit, to avoid
> yet more copy-and-paste there for the new format decoder callback).
Ah, thanks for clarifying.
> The implementation itself is all within dumpfile.c, hence the non-c-
> family languages ought to work. My testing was with:
> --enable-languages=c,c++,objc,obj-c++,fortran,ada,go,lto,jit,brig
> (and with cloog and isl, fwiw).
>
> (I kept the alphabetization of the ChangeLog files from my generate-
> changelog.py script, which put the gcc/c-family/ChangeLog before the
> gcc/ChangeLog and thus may have made this confusing to read, sorry).
>
> I didn't exhaustively check every callsite to the changed calls; I'm
> assuming that -Wformat during bootstrap has effectively checked that
> for me. Though now I think about it, I note that we use
> HOST_WIDE_INT_PRINT_DEC in many places: is this guaranteed to be a
> valid input to pp_format on all of our configurations?
I hope so ... returning to the patch now.
Richard.
> Dave
>
> >
> > > gcc/ChangeLog:
> > > * dump-context.h: Include "dumpfile.h".
> > > (dump_context::dump_printf_va): Convert final param from
> > > va_list
> > > to va_list *. Convert from ATTRIBUTE_PRINTF to
> > > ATTRIBUTE_GCC_DUMP_PRINTF.
> > > (dump_context::dump_printf_loc_va): Likewise.
> > > * dumpfile.c: Include "stringpool.h".
> > > (make_item_for_dump_printf_va): Delete.
> > > (make_item_for_dump_printf): Delete.
> > > (class dump_pretty_printer): New class.
> > >
Re: [PATCH 5/5] Formatted printing for dump_* in the middle-end
On Tue, 2018-07-31 at 15:03 +0200, Richard Biener wrote:
> On Fri, Jul 27, 2018 at 11:49 PM David Malcolm
> wrote:
> >
> > This patch converts dump_print and dump_printf_loc from using
> > printf (and thus ATTRIBUTE_PRINTF) to using a new pretty-printer
> > based on pp_format, which supports formatting middle-end types.
> >
> > In particular, the following codes are implemented (in addition
> > to the standard pretty_printer ones):
> >
> >%E: gimple *:
> >Equivalent to: dump_gimple_expr (MSG_*, TDF_SLIM, stmt, 0)
> >%G: gimple *:
> >Equivalent to: dump_gimple_stmt (MSG_*, TDF_SLIM, stmt, 0)
> >%T: tree:
> >Equivalent to: dump_generic_expr (MSG_*, arg, TDF_SLIM).
> >
> > Hence it becomes possible to convert e.g.:
> >
> > if (dump_enabled_p ())
> > {
> > dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> >"not vectorized: different sized vector "
> >"types in statement, ");
> > dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> > vectype);
> > dump_printf (MSG_MISSED_OPTIMIZATION, " and ");
> > dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> > nunits_vectype);
> > dump_printf (MSG_MISSED_OPTIMIZATION, "\n");
> > }
> >
> > into a one-liner:
> >
> > if (dump_enabled_p ())
> > dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> > "not vectorized: different sized vector "
> > "types in statement, %T and %T\n",
> > vectype, nunits_vectype);
> >
> > Unlike regular pretty-printers, this one captures optinfo_item
> > instances for the formatted chunks as appropriate, so that when
> > written out to a JSON optimization record, the relevant parts of
> > the message are labelled by type, and by source location (so that
> > e.g. %G is entirely equivalent to using dump_gimple_stmt).
> >
> > dump_printf and dump_printf_loc become marked with
> > ATTRIBUTE_GCC_DUMP_PRINTF, which the patch also implements.
> >
> > gcc/c-family/ChangeLog:
> > * c-format.c (enum format_type): Add
> > gcc_dump_printf_format_type.
> > (local_gimple_ptr_node): New decl.
> > (gcc_dump_printf_length_specs): New.
> > (gcc_dump_printf_flag_pairs): New.
> > (gcc_dump_printf_flag_specs): New.
> > (gcc_dump_printf_char_table): New.
> > (format_types_orig): Add entry for "gcc_dump_printf".
> > (init_dynamic_diag_info): Create local_gimple_ptr_node.
> > Set up length_char_specs and conversion_specs for
> > gcc_dump_printf_format_type.
> > (handle_format_attribute): Handle
> > gcc_dump_printf_format_type.
> > * c-format.h (T89_GIMPLE): New macro.
>
> Iff the c-family changes are neccessary (are they?) then how does
> this
> work for non-c-family languages which do not link c-family/c-
> format.o?
The c-family changes are necessary for bootstrap, so that -Wformat
works cleanly after changing dump_printf_loc etc from
ATTRIBUTE_PRINTF_3;
to
ATTRIBUTE_GCC_DUMP_PRINTF (3, 0);
i.e. they're just the changes to -Wformat to teach it how to verify the
new:
__attribute__ ((__format__ (__gcc_dump_printf__, m ,n)))
(hence the cleanups to c-format.c earlier in the patch kit, to avoid
yet more copy-and-paste there for the new format decoder callback).
The implementation itself is all within dumpfile.c, hence the non-c-
family languages ought to work. My testing was with:
--enable-languages=c,c++,objc,obj-c++,fortran,ada,go,lto,jit,brig
(and with cloog and isl, fwiw).
(I kept the alphabetization of the ChangeLog files from my generate-
changelog.py script, which put the gcc/c-family/ChangeLog before the
gcc/ChangeLog and thus may have made this confusing to read, sorry).
I didn't exhaustively check every callsite to the changed calls; I'm
assuming that -Wformat during bootstrap has effectively checked that
for me. Though now I think about it, I note that we use
HOST_WIDE_INT_PRINT_DEC in many places: is this guaranteed to be a
valid input to pp_format on all of our configurations?
Dave
>
> > gcc/ChangeLog:
> > * dump-context.h: Include "dumpfile.h".
> > (dump_context::dump_printf_va): Convert final param from
> > va_list
> > to va_list *. Convert from ATTRIBUTE_PRINTF to
> > ATTRIBUTE_GCC_DUMP_PRINTF.
> > (dump_context::dump_printf_loc_va): Likewise.
> > * dumpfile.c: Include "stringpool.h".
> > (make_item_for_dump_printf_va): Delete.
> > (make_item_for_dump_printf): Delete.
> > (class dump_pretty_printer): New class.
> > (dump_pretty_printer::dump_pretty_printer): New ctor.
> > (dump_pretty_printer::emit_items): New member function.
> > (dump_pretty_printer::emit_any_pending_textual_chunks): New
> > member
> > function.
> > (dump_pretty_printer::emit_item): New member function.
> > (dump_pretty_printer::stash_it
Re: [PATCH 5/5] Formatted printing for dump_* in the middle-end
On Fri, Jul 27, 2018 at 11:49 PM David Malcolm wrote:
>
> This patch converts dump_print and dump_printf_loc from using
> printf (and thus ATTRIBUTE_PRINTF) to using a new pretty-printer
> based on pp_format, which supports formatting middle-end types.
>
> In particular, the following codes are implemented (in addition
> to the standard pretty_printer ones):
>
>%E: gimple *:
>Equivalent to: dump_gimple_expr (MSG_*, TDF_SLIM, stmt, 0)
>%G: gimple *:
>Equivalent to: dump_gimple_stmt (MSG_*, TDF_SLIM, stmt, 0)
>%T: tree:
>Equivalent to: dump_generic_expr (MSG_*, arg, TDF_SLIM).
>
> Hence it becomes possible to convert e.g.:
>
> if (dump_enabled_p ())
> {
> dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
>"not vectorized: different sized vector "
>"types in statement, ");
> dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM, vectype);
> dump_printf (MSG_MISSED_OPTIMIZATION, " and ");
> dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM, nunits_vectype);
> dump_printf (MSG_MISSED_OPTIMIZATION, "\n");
> }
>
> into a one-liner:
>
> if (dump_enabled_p ())
> dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> "not vectorized: different sized vector "
> "types in statement, %T and %T\n",
> vectype, nunits_vectype);
>
> Unlike regular pretty-printers, this one captures optinfo_item
> instances for the formatted chunks as appropriate, so that when
> written out to a JSON optimization record, the relevant parts of
> the message are labelled by type, and by source location (so that
> e.g. %G is entirely equivalent to using dump_gimple_stmt).
>
> dump_printf and dump_printf_loc become marked with
> ATTRIBUTE_GCC_DUMP_PRINTF, which the patch also implements.
>
> gcc/c-family/ChangeLog:
> * c-format.c (enum format_type): Add gcc_dump_printf_format_type.
> (local_gimple_ptr_node): New decl.
> (gcc_dump_printf_length_specs): New.
> (gcc_dump_printf_flag_pairs): New.
> (gcc_dump_printf_flag_specs): New.
> (gcc_dump_printf_char_table): New.
> (format_types_orig): Add entry for "gcc_dump_printf".
> (init_dynamic_diag_info): Create local_gimple_ptr_node.
> Set up length_char_specs and conversion_specs for
> gcc_dump_printf_format_type.
> (handle_format_attribute): Handle gcc_dump_printf_format_type.
> * c-format.h (T89_GIMPLE): New macro.
Iff the c-family changes are neccessary (are they?) then how does this
work for non-c-family languages which do not link c-family/c-format.o?
> gcc/ChangeLog:
> * dump-context.h: Include "dumpfile.h".
> (dump_context::dump_printf_va): Convert final param from va_list
> to va_list *. Convert from ATTRIBUTE_PRINTF to
> ATTRIBUTE_GCC_DUMP_PRINTF.
> (dump_context::dump_printf_loc_va): Likewise.
> * dumpfile.c: Include "stringpool.h".
> (make_item_for_dump_printf_va): Delete.
> (make_item_for_dump_printf): Delete.
> (class dump_pretty_printer): New class.
> (dump_pretty_printer::dump_pretty_printer): New ctor.
> (dump_pretty_printer::emit_items): New member function.
> (dump_pretty_printer::emit_any_pending_textual_chunks): New member
> function.
> (dump_pretty_printer::emit_item): New member function.
> (dump_pretty_printer::stash_item): New member function.
> (dump_pretty_printer::format_decoder_cb): New member function.
> (dump_pretty_printer::decode_format): New member function.
> (dump_context::dump_printf_va): Reimplement in terms of
> dump_pretty_printer.
> (dump_context::dump_printf_loc_va): Convert final param from va_list
> to va_list *.
> (dump_context::begin_scope): Reimplement call to
> make_item_for_dump_printf.
> (dump_printf): Update for change to dump_printf_va.
> (dump_printf_loc): Likewise.
> (selftest::test_capture_of_dump_calls): Convert "stmt" from
> greturn * to gimple *. Add a test_decl. Add tests of dump_printf
> with %T, %E, and %G.
> * dumpfile.h (ATTRIBUTE_GCC_DUMP_PRINTF): New macro.
> (dump_printf): Replace ATTRIBUTE_PRINTF_2 with
> ATTRIBUTE_GCC_DUMP_PRINTF (2, 3).
> (dump_printf_loc): Replace ATTRIBUTE_PRINTF_3 with
> ATTRIBUTE_GCC_DUMP_PRINTF (3, 0).
>
> gcc/testsuite/ChangeLog:
> * gcc.dg/format/gcc_diag-1.c: Fix typo. Add test coverage for
> gcc_dump_printf.
> * gcc.dg/format/gcc_diag-10.c: Add gimple typedef. Add test
> coverage for gcc_dump_printf.
> ---
> gcc/c-family/c-format.c | 60 -
> gcc/c-family/c-format.h | 1 +
> gcc/dump-context.h| 7 +-
> gcc/dumpfile.c|
