| Message ID | 20200429124645.GG2424@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs | expand |
Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > @@ -5747,16 +5748,18 @@ rs6000_discover_homogeneous_aggregate (m > unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); > if (uid != last_reported_type_uid) > { > + const char *url > + = get_changes_url ("gcc-10/changes.html#empty_base"); > if (empty_base_seen & 1) > inform (input_location, > "parameter passing for argument of type %qT " > "when C++17 is enabled changed to match C++14 " > - "in GCC 10.1", type); > + "in %{GCC 10.1%}", type, url); > else > inform (input_location, > "parameter passing for argument of type %qT " > "with %<[[no_unique_address]]%> members " > - "changed in GCC 10.1", type); > + "changed in %{GCC 10.1%}", type, url); > last_reported_type_uid = uid; > } > } Looks like there's a missing free() for this one. Thanks, Richard
On Wed, 2020-04-29 at 14:46 +0200, Jakub Jelinek wrote: > Hi! > > The following patch attempts to use the diagnostics URL support if > available > to provide more information about the C++17 empty base and C++20 > [[no_unique_address]] empty class ABI changes in -Wpsabi diagnostics. > > GCC 10.1 at the end of the diagnostics is then in some terminals > underlined with a dotted line and points to a (to be written) anchor > in > gcc-10/changes.html which we need to write anyway. > > Ok for trunk if this passes bootstrap/regtest? Thanks for implementing this. Overall I like the idea; some nits inline below (and I think it won't bootstrap due to a const-correctness issue). Ideally we ought to have an integration test for this in DejaGnu somewhere, somehow, but I don't think we have a way to write one, alas. [...] > --- gcc/pretty-print.c.jj 2020-04-25 00:08:33.359759328 +0200 > +++ gcc/pretty-print.c 2020-04-29 14:30:46.791792554 +0200 > @@ -1020,6 +1020,8 @@ pp_indent (pretty_printer *pp) > pp_space (pp); > } > > +static const char *end_url_string (pretty_printer *); > + > /* The following format specifiers are recognized as being client > independent: > %d, %i: (signed) integer in base ten. > %u: unsigned integer in base ten. > @@ -1038,6 +1040,8 @@ pp_indent (pretty_printer *pp) > %%: '%'. > %<: opening quote. > %>: closing quote. > + %{: URL start. > + %}: URL end. I think this needs a little more explanation; perhaps something like: %{: URL start. Consumes a const char * argument for the URL. %}: URL end. Does not consume any arguments. ? [...] > > +/* Helper function for pp_end_url and pp_format, return the "close > URL" escape > + sequence string. */ > + > +static const char * > +end_url_string (pretty_printer *pp) Given that this passes in a pp, please rename this to "get_end_url_string to" avoid possible confusion that "end" might be a verb here, and thus might emit the codes to the pp. > +{ > + switch (pp->url_format) > + { > + case URL_FORMAT_NONE: > + return ""; > + case URL_FORMAT_ST: > + return "\33]8;;\33\\"; > + case URL_FORMAT_BEL: > + return "\33]8;;\a"; > + default: > + gcc_unreachable (); > + } > } > > /* If URL-printing is enabled, write a "close URL" escape sequence to > PP. */ > @@ -2191,19 +2231,8 @@ pp_begin_url (pretty_printer *pp, const > void > pp_end_url (pretty_printer *pp) > { > - switch (pp->url_format) > - { > - case URL_FORMAT_NONE: > - break; > - case URL_FORMAT_ST: > - pp_string (pp, "\33]8;;\33\\"); > - break; > - case URL_FORMAT_BEL: > - pp_string (pp, "\33]8;;\a"); > - break; > - default: > - gcc_unreachable (); > - } > + if (pp->url_format != URL_FORMAT_NONE) > + pp_string (pp, end_url_string (pp)); > } > > #if CHECKING_P [...] > --- gcc/opts.c.jj 2020-04-27 23:28:18.865609469 +0200 > +++ gcc/opts.c 2020-04-29 13:42:13.033891253 +0200 > @@ -3190,6 +3190,15 @@ get_option_url (diagnostic_context *, in > return NULL; > } > > +/* Given "gcc-10/changes.html#foobar", return that URL under > + CHANGES_ROOT_URL (see --with-changes-root-url). */ Although the return type is a "char *" rather than "const char *" I think this comment could be improved by clarifying ownership. How about: /* Given "gcc-10/changes.html#foobar", return that URL under CHANGES_ROOT_URL (see --with-changes-root-url). The caller is responsible for freeing the returned string. */ > + > +char * > +get_changes_url (const char *str) > +{ > + return concat (CHANGES_ROOT_URL, str, NULL); > +} > + > #if CHECKING_P > > namespace selftest { > --- gcc/config/arm/arm.c.jj 2020-04-29 13:12:46.736007298 +0200 > +++ gcc/config/arm/arm.c 2020-04-29 14:34:04.878864236 +0200 There's some pre-existing repetition between arm, aarch64, and rs6000, in which this patch has to make the same changes in multiple places. Could these be consolidated e.g. void maybe_emit_gcc10_param_passing_abi_warning (tree type) { /* something here; not sure what */ } That said it's a pre-existing thing. > @@ -6416,6 +6416,8 @@ aapcs_vfp_is_call_or_return_candidate (e > && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, > NULL)) > != ag_count)) > { > + const char *url > + = get_changes_url ("gcc-10/changes.html#empty_base"); "url" is declared here (and elsewhere) as a "const char *", but later freed; that seems like a violation of const-correctness. Hopefully we emit a warning about that. > gcc_assert (alt == -1); > last_reported_type_uid = uid; > /* Use TYPE_MAIN_VARIANT to strip any redundant const > @@ -6423,11 +6425,14 @@ aapcs_vfp_is_call_or_return_candidate (e > if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) > inform (input_location, "parameter passing for argument > of " > "type %qT with %<[[no_unique_address]]%> > members " > - "changed in GCC 10.1", TYPE_MAIN_VARIANT > (type)); > + "changed in %{GCC 10.1%}", > + TYPE_MAIN_VARIANT (type), url); (I really don't like the way our initial releases are *.1 rather than *.0, but that's a bikeshed issue for another day, and not something that this patch is changing) A different bikeshed: it might be better style for the hyperlink to cover the text "%{changed in GCC 10.1%}" given that the link is describing a specific change in GCC 10.1 rather than GCC 10.1 itself. I found some thoughts on this issue here: https://developers.google.com/style/link-text Not sure if that complicates translation though, and it's a relatively minor nit. > else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) > inform (input_location, "parameter passing for argument > of " > "type %qT when C++17 is enabled changed to > match " > - "C++14 in GCC 10.1", TYPE_MAIN_VARIANT (type)); > + "C++14 in %{GCC 10.1%}", > + TYPE_MAIN_VARIANT (type), url); > + free (url); > } > *count = ag_count; > } > --- gcc/config/aarch64/aarch64.c.jj 2020-04-29 13:12:46.715007609 > +0200 > +++ gcc/config/aarch64/aarch64.c 2020-04-29 14:35:06.712950146 > +0200 > @@ -16883,6 +16883,8 @@ aarch64_vfp_is_call_or_return_candidate > && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, > NULL)) > != ag_count)) > { > + const char *url > + = get_changes_url ("gcc-10/changes.html#empty_base"); > gcc_assert (alt == -1); > last_reported_type_uid = uid; > /* Use TYPE_MAIN_VARIANT to strip any redundant const > @@ -16890,11 +16892,14 @@ aarch64_vfp_is_call_or_return_candidate > if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) > inform (input_location, "parameter passing for argument > of " > "type %qT with %<[[no_unique_address]]%> > members " > - "changed in GCC 10.1", TYPE_MAIN_VARIANT > (type)); > + "changed in %{GCC 10.1%}", > + TYPE_MAIN_VARIANT (type), url); > else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) > inform (input_location, "parameter passing for argument > of " > "type %qT when C++17 is enabled changed to > match " > - "C++14 in GCC 10.1", TYPE_MAIN_VARIANT (type)); > + "C++14 in %{GCC 10.1%}", > + TYPE_MAIN_VARIANT (type), url); > + free (url); > } > > if (is_ha != NULL) *is_ha = true; > --- gcc/config/rs6000/rs6000-call.c.jj 2020-04-29 > 09:00:46.642524337 +0200 > +++ gcc/config/rs6000/rs6000-call.c 2020-04-29 13:50:45.873299042 > +0200 > @@ -68,6 +68,7 @@ > #include "tree-vrp.h" > #include "tree-ssanames.h" > #include "targhooks.h" > +#include "opts.h" > > #include "rs6000-internal.h" > > @@ -5747,16 +5748,18 @@ rs6000_discover_homogeneous_aggregate (m > unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); > if (uid != last_reported_type_uid) > { > + const char *url > + = get_changes_url ("gcc- > 10/changes.html#empty_base"); > if (empty_base_seen & 1) > inform (input_location, > "parameter passing for argument of type > %qT " > "when C++17 is enabled changed to match > C++14 " > - "in GCC 10.1", type); > + "in %{GCC 10.1%}", type, url); > else > inform (input_location, > "parameter passing for argument of type > %qT " > "with %<[[no_unique_address]]%> members > " > - "changed in GCC 10.1", type); > + "changed in %{GCC 10.1%}", type, url); > last_reported_type_uid = uid; > } > } > --- gcc/c-family/c-format.c.jj 2020-02-10 15:49:50.821300965 > +0100 > +++ gcc/c-family/c-format.c 2020-04-29 13:56:26.926250904 +0200 > @@ -757,6 +757,8 @@ static const format_char_info asm_fprint > { "<", 0, STD_C89, NOARGUMENTS, "", "<", NULL }, \ > { ">", 0, STD_C89, NOARGUMENTS, "", ">", NULL }, \ > { "'" , 0, STD_C89, NOARGUMENTS, "", "", NULL }, \ > + { "{", 1, STD_C89, { > T89_C, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN > , BADLEN, BADLEN, BADLEN, BADLEN }, "", "cR", NULL }, \ > + { "}", 0, STD_C89, NOARGUMENTS, "", "", NULL }, \ > { "R", 0, STD_C89, NOARGUMENTS, "", "\\", NULL }, \ > { "m", 0, STD_C89, NOARGUMENTS, "q", "", NULL }, \ > { "Z", 1, STD_C89, { > T89_I, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN > , BADLEN, BADLEN, BADLEN, BADLEN }, "", "", > &gcc_diag_char_table[0] }
On 29/04/20 09:24 -0400, David Malcolm wrote: >On Wed, 2020-04-29 at 14:46 +0200, Jakub Jelinek wrote: >> @@ -6416,6 +6416,8 @@ aapcs_vfp_is_call_or_return_candidate (e >> && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, >> NULL)) >> != ag_count)) >> { >> + const char *url >> + = get_changes_url ("gcc-10/changes.html#empty_base"); > >"url" is declared here (and elsewhere) as a "const char *", but later >freed; that seems like a violation of const-correctness. Hopefully we >emit a warning about that. Since this is C++, it should be an error not a warning.
On Wed, Apr 29, 2020 at 3:44 PM Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi! > > The following patch attempts to use the diagnostics URL support if available > to provide more information about the C++17 empty base and C++20 > [[no_unique_address]] empty class ABI changes in -Wpsabi diagnostics. > > GCC 10.1 at the end of the diagnostics is then in some terminals > underlined with a dotted line and points to a (to be written) anchor in > gcc-10/changes.html which we need to write anyway. > > Ok for trunk if this passes bootstrap/regtest? > > 2020-04-29 Jakub Jelinek <jakub@redhat.com> > > * configure.ac (-with-changes-root-url): New configure option, > defaulting to https://gcc.gnu.org/. > * Makefile.in (CFLAGS-opts.o): Define CHANGES_ROOT_URL for > opts.c. > * pretty-print.c (end_url_string): New function. > (pp_format): Handle %{ and %} for URLs. > (pp_begin_url): Use pp_string instead of pp_printf. > (pp_end_url): Use end_url_string. > * opts.h (get_changes_url): Declare. > * opts.c (get_changes_url): New function. > * config/rs6000/rs6000-call.c: Include opts.h. > (rs6000_discover_homogeneous_aggregate): Use %{GCC 10.1%} instead of > just GCC 10.1 in diagnostics and add URL. > * config/arm/arm.c (aapcs_vfp_is_call_or_return_candidate): Likewise. > * config/aarch64/aarch64.c (aarch64_vfp_is_call_or_return_candidate): > Likewise. > * configure: Regenerated. > > * c-format.c (PP_FORMAT_CHAR_TABLE): Add %{ and %}. > > --- gcc/configure.ac.jj 2020-04-29 10:21:25.061999873 +0200 > +++ gcc/configure.ac 2020-04-29 13:26:51.515516959 +0200 > @@ -986,6 +986,20 @@ AC_ARG_WITH(documentation-root-url, > ) > AC_SUBST(DOCUMENTATION_ROOT_URL) > > +# Allow overriding the default URL for GCC changes > +AC_ARG_WITH(changes-root-url, > + AS_HELP_STRING([--with-changes-root-url=URL], > + [Root for GCC changes URLs]), > + [case "$withval" in > + yes) AC_MSG_ERROR([changes root URL not specified]) ;; > + no) AC_MSG_ERROR([changes root URL not specified]) ;; > + *) CHANGES_ROOT_URL="$withval" > + ;; > + esac], > + CHANGES_ROOT_URL="https://gcc.gnu.org/" > +) > +AC_SUBST(CHANGES_ROOT_URL) > + > # Sanity check enable_languages in case someone does not run the toplevel > # configure # script. > AC_ARG_ENABLE(languages, > --- gcc/Makefile.in.jj 2020-02-27 09:28:46.129960135 +0100 > +++ gcc/Makefile.in 2020-04-29 13:38:42.455008718 +0200 > @@ -2187,6 +2187,7 @@ lto-wrapper$(exeext): $(LTO_WRAPPER_OBJS > mv -f T$@ $@ > > CFLAGS-opts.o += -DDOCUMENTATION_ROOT_URL=\"@DOCUMENTATION_ROOT_URL@\" > +CFLAGS-opts.o += -DCHANGES_ROOT_URL=\"@CHANGES_ROOT_URL@\" > > # Files used by all variants of C or by the stand-alone pre-processor. > > --- gcc/pretty-print.c.jj 2020-04-25 00:08:33.359759328 +0200 > +++ gcc/pretty-print.c 2020-04-29 14:30:46.791792554 +0200 > @@ -1020,6 +1020,8 @@ pp_indent (pretty_printer *pp) > pp_space (pp); > } > > +static const char *end_url_string (pretty_printer *); > + > /* The following format specifiers are recognized as being client independent: > %d, %i: (signed) integer in base ten. > %u: unsigned integer in base ten. > @@ -1038,6 +1040,8 @@ pp_indent (pretty_printer *pp) > %%: '%'. > %<: opening quote. > %>: closing quote. > + %{: URL start. > + %}: URL end. > %': apostrophe (should only be used in untranslated messages; > translations should use appropriate punctuation directly). > %@: diagnostic_event_id_ptr, for which event_id->known_p () must be true. > @@ -1051,7 +1055,7 @@ pp_indent (pretty_printer *pp) > Arguments can be used sequentially, or through %N$ resp. *N$ > notation Nth argument after the format string. If %N$ / *N$ > notation is used, it must be used for all arguments, except %m, %%, > - %<, %> and %', which may not have a number, as they do not consume > + %<, %>, %} and %', which may not have a number, as they do not consume > an argument. When %M$.*N$s is used, M must be N + 1. (This may > also be written %M$.*s, provided N is not otherwise used.) The > format string must have conversion specifiers with argument numbers > @@ -1084,7 +1088,7 @@ pp_format (pretty_printer *pp, text_info > /* Formatting phase 1: split up TEXT->format_spec into chunks in > pp_buffer (PP)->args[]. Even-numbered chunks are to be output > verbatim, odd-numbered chunks are format specifiers. > - %m, %%, %<, %>, and %' are replaced with the appropriate text at > + %m, %%, %<, %>, %} and %' are replaced with the appropriate text at > this point. */ > > memset (formatters, 0, sizeof formatters); > @@ -1133,6 +1137,15 @@ pp_format (pretty_printer *pp, text_info > p++; > continue; > > + case '}': > + { > + const char *endurlstr = end_url_string (pp); > + obstack_grow (&buffer->chunk_obstack, endurlstr, > + strlen (endurlstr)); > + } > + p++; > + continue; > + > case 'R': > { > const char *colorstr = colorize_stop (pp_show_color (pp)); > @@ -1445,6 +1458,10 @@ pp_format (pretty_printer *pp, text_info > } > break; > > + case '{': > + pp_begin_url (pp, va_arg (*text->args_ptr, const char *)); > + break; > + > default: > { > bool ok; > @@ -2172,18 +2189,41 @@ void > pp_begin_url (pretty_printer *pp, const char *url) > { > switch (pp->url_format) > - { > - case URL_FORMAT_NONE: > - break; > - case URL_FORMAT_ST: > - pp_printf (pp, "\33]8;;%s\33\\", url); > - break; > - case URL_FORMAT_BEL: > - pp_printf (pp, "\33]8;;%s\a", url); > - break; > - default: > - gcc_unreachable (); > - } > + { > + case URL_FORMAT_NONE: > + break; > + case URL_FORMAT_ST: > + pp_string (pp, "\33]8;;"); > + pp_string (pp, url); > + pp_string (pp, "\33\\"); > + break; > + case URL_FORMAT_BEL: > + pp_string (pp, "\33]8;;"); > + pp_string (pp, url); > + pp_string (pp, "\a"); > + break; > + default: > + gcc_unreachable (); > + } > +} > + > +/* Helper function for pp_end_url and pp_format, return the "close URL" escape > + sequence string. */ > + > +static const char * > +end_url_string (pretty_printer *pp) > +{ > + switch (pp->url_format) > + { > + case URL_FORMAT_NONE: > + return ""; > + case URL_FORMAT_ST: > + return "\33]8;;\33\\"; > + case URL_FORMAT_BEL: > + return "\33]8;;\a"; > + default: > + gcc_unreachable (); > + } > } > > /* If URL-printing is enabled, write a "close URL" escape sequence to PP. */ > @@ -2191,19 +2231,8 @@ pp_begin_url (pretty_printer *pp, const > void > pp_end_url (pretty_printer *pp) > { > - switch (pp->url_format) > - { > - case URL_FORMAT_NONE: > - break; > - case URL_FORMAT_ST: > - pp_string (pp, "\33]8;;\33\\"); > - break; > - case URL_FORMAT_BEL: > - pp_string (pp, "\33]8;;\a"); > - break; > - default: > - gcc_unreachable (); > - } > + if (pp->url_format != URL_FORMAT_NONE) > + pp_string (pp, end_url_string (pp)); > } > > #if CHECKING_P > --- gcc/opts.h.jj 2020-02-24 09:02:27.332710905 +0100 > +++ gcc/opts.h 2020-04-29 13:56:55.621826605 +0200 > @@ -464,6 +464,7 @@ extern void parse_options_from_collect_g > int *); > > extern void prepend_xassembler_to_collect_as_options (const char *, obstack *); > +extern char *get_changes_url (const char *); > > /* Set OPTION in OPTS to VALUE if the option is not set in OPTS_SET. */ > > --- gcc/opts.c.jj 2020-04-27 23:28:18.865609469 +0200 > +++ gcc/opts.c 2020-04-29 13:42:13.033891253 +0200 > @@ -3190,6 +3190,15 @@ get_option_url (diagnostic_context *, in > return NULL; > } > > +/* Given "gcc-10/changes.html#foobar", return that URL under > + CHANGES_ROOT_URL (see --with-changes-root-url). */ > + > +char * > +get_changes_url (const char *str) > +{ > + return concat (CHANGES_ROOT_URL, str, NULL); > +} > + > #if CHECKING_P > > namespace selftest { > --- gcc/config/arm/arm.c.jj 2020-04-29 13:12:46.736007298 +0200 > +++ gcc/config/arm/arm.c 2020-04-29 14:34:04.878864236 +0200 > @@ -6416,6 +6416,8 @@ aapcs_vfp_is_call_or_return_candidate (e > && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) > != ag_count)) > { > + const char *url > + = get_changes_url ("gcc-10/changes.html#empty_base"); This is called even when !warn_psabi_flags and I wonder if we could instead use a macro at uses, like > gcc_assert (alt == -1); > last_reported_type_uid = uid; > /* Use TYPE_MAIN_VARIANT to strip any redundant const > @@ -6423,11 +6425,14 @@ aapcs_vfp_is_call_or_return_candidate (e > if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) > inform (input_location, "parameter passing for argument of " > "type %qT with %<[[no_unique_address]]%> members " > - "changed in GCC 10.1", TYPE_MAIN_VARIANT (type)); > + "changed in %{GCC 10.1%}", > + TYPE_MAIN_VARIANT (type), url); , CHANGES_URL ("gcc-10/changes.html#empty_base"); where the macro would just use preprocessor string concatenation? Alternatively 'url' could be static I guess (and never freed) > else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) > inform (input_location, "parameter passing for argument of " > "type %qT when C++17 is enabled changed to match " > - "C++14 in GCC 10.1", TYPE_MAIN_VARIANT (type)); > + "C++14 in %{GCC 10.1%}", > + TYPE_MAIN_VARIANT (type), url); > + free (url); > } > *count = ag_count; > } > --- gcc/config/aarch64/aarch64.c.jj 2020-04-29 13:12:46.715007609 +0200 > +++ gcc/config/aarch64/aarch64.c 2020-04-29 14:35:06.712950146 +0200 > @@ -16883,6 +16883,8 @@ aarch64_vfp_is_call_or_return_candidate > && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) > != ag_count)) > { > + const char *url > + = get_changes_url ("gcc-10/changes.html#empty_base"); > gcc_assert (alt == -1); > last_reported_type_uid = uid; > /* Use TYPE_MAIN_VARIANT to strip any redundant const > @@ -16890,11 +16892,14 @@ aarch64_vfp_is_call_or_return_candidate > if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) > inform (input_location, "parameter passing for argument of " > "type %qT with %<[[no_unique_address]]%> members " > - "changed in GCC 10.1", TYPE_MAIN_VARIANT (type)); > + "changed in %{GCC 10.1%}", > + TYPE_MAIN_VARIANT (type), url); > else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) > inform (input_location, "parameter passing for argument of " > "type %qT when C++17 is enabled changed to match " > - "C++14 in GCC 10.1", TYPE_MAIN_VARIANT (type)); > + "C++14 in %{GCC 10.1%}", > + TYPE_MAIN_VARIANT (type), url); > + free (url); > } > > if (is_ha != NULL) *is_ha = true; > --- gcc/config/rs6000/rs6000-call.c.jj 2020-04-29 09:00:46.642524337 +0200 > +++ gcc/config/rs6000/rs6000-call.c 2020-04-29 13:50:45.873299042 +0200 > @@ -68,6 +68,7 @@ > #include "tree-vrp.h" > #include "tree-ssanames.h" > #include "targhooks.h" > +#include "opts.h" > > #include "rs6000-internal.h" > > @@ -5747,16 +5748,18 @@ rs6000_discover_homogeneous_aggregate (m > unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); > if (uid != last_reported_type_uid) > { > + const char *url > + = get_changes_url ("gcc-10/changes.html#empty_base"); > if (empty_base_seen & 1) > inform (input_location, > "parameter passing for argument of type %qT " > "when C++17 is enabled changed to match C++14 " > - "in GCC 10.1", type); > + "in %{GCC 10.1%}", type, url); > else > inform (input_location, > "parameter passing for argument of type %qT " > "with %<[[no_unique_address]]%> members " > - "changed in GCC 10.1", type); > + "changed in %{GCC 10.1%}", type, url); > last_reported_type_uid = uid; > } > } > --- gcc/c-family/c-format.c.jj 2020-02-10 15:49:50.821300965 +0100 > +++ gcc/c-family/c-format.c 2020-04-29 13:56:26.926250904 +0200 > @@ -757,6 +757,8 @@ static const format_char_info asm_fprint > { "<", 0, STD_C89, NOARGUMENTS, "", "<", NULL }, \ > { ">", 0, STD_C89, NOARGUMENTS, "", ">", NULL }, \ > { "'" , 0, STD_C89, NOARGUMENTS, "", "", NULL }, \ > + { "{", 1, STD_C89, { T89_C, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN }, "", "cR", NULL }, \ > + { "}", 0, STD_C89, NOARGUMENTS, "", "", NULL }, \ > { "R", 0, STD_C89, NOARGUMENTS, "", "\\", NULL }, \ > { "m", 0, STD_C89, NOARGUMENTS, "q", "", NULL }, \ > { "Z", 1, STD_C89, { T89_I, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN }, "", "", &gcc_diag_char_table[0] } > --- gcc/configure.jj 2020-04-29 10:21:25.060999888 +0200 > +++ gcc/configure 2020-04-29 13:28:07.423395043 +0200 > @@ -819,6 +819,7 @@ accel_dir_suffix > real_target_noncanonical > enable_as_accelerator > gnat_install_lib > +CHANGES_ROOT_URL > DOCUMENTATION_ROOT_URL > REPORT_BUGS_TEXI > REPORT_BUGS_TO > @@ -967,6 +968,7 @@ with_specs > with_pkgversion > with_bugurl > with_documentation_root_url > +with_changes_root_url > enable_languages > with_multilib_list > with_zstd > @@ -1803,6 +1805,8 @@ Optional Packages: > --with-bugurl=URL Direct users to URL to report a bug > --with-documentation-root-url=URL > Root for documentation URLs > + --with-changes-root-url=URL > + Root for GCC changes URLs > --with-multilib-list select multilibs (AArch64, SH and x86-64 only) > --with-zstd=PATH specify prefix directory for installed zstd library. > Equivalent to --with-zstd-include=PATH/include plus > @@ -7857,6 +7861,23 @@ fi > > > > +# Allow overriding the default URL for GCC changes > + > +# Check whether --with-changes-root-url was given. > +if test "${with_changes_root_url+set}" = set; then : > + withval=$with_changes_root_url; case "$withval" in > + yes) as_fn_error $? "changes root URL not specified" "$LINENO" 5 ;; > + no) as_fn_error $? "changes root URL not specified" "$LINENO" 5 ;; > + *) CHANGES_ROOT_URL="$withval" > + ;; > + esac > +else > + CHANGES_ROOT_URL="https://gcc.gnu.org/" > + > +fi > + > + > + > # Sanity check enable_languages in case someone does not run the toplevel > # configure # script. > # Check whether --enable-languages was given. > @@ -18988,7 +19009,7 @@ else > lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 > lt_status=$lt_dlunknown > cat > conftest.$ac_ext <<_LT_EOF > -#line 18991 "configure" > +#line 19012 "configure" > #include "confdefs.h" > > #if HAVE_DLFCN_H > @@ -19094,7 +19115,7 @@ else > lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 > lt_status=$lt_dlunknown > cat > conftest.$ac_ext <<_LT_EOF > -#line 19097 "configure" > +#line 19118 "configure" > #include "confdefs.h" > > #if HAVE_DLFCN_H > > Jakub >
On Thu, Apr 30, 2020 at 08:42:39AM +0200, Richard Biener wrote: > > --- gcc/config/arm/arm.c.jj 2020-04-29 13:12:46.736007298 +0200 > > +++ gcc/config/arm/arm.c 2020-04-29 14:34:04.878864236 +0200 > > @@ -6416,6 +6416,8 @@ aapcs_vfp_is_call_or_return_candidate (e > > && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) > > != ag_count)) > > { > > + const char *url > > + = get_changes_url ("gcc-10/changes.html#empty_base"); > > This is called even when !warn_psabi_flags and I wonder if we could > instead use a macro at uses, like It is not, because the 3 lines above the snippet are: if (warn_psabi && warn_psabi_flags && uid != last_reported_type_uid > > > gcc_assert (alt == -1); > > last_reported_type_uid = uid; > > /* Use TYPE_MAIN_VARIANT to strip any redundant const > > @@ -6423,11 +6425,14 @@ aapcs_vfp_is_call_or_return_candidate (e > > if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) > > inform (input_location, "parameter passing for argument of " > > "type %qT with %<[[no_unique_address]]%> members " > > - "changed in GCC 10.1", TYPE_MAIN_VARIANT (type)); > > + "changed in %{GCC 10.1%}", > > + TYPE_MAIN_VARIANT (type), url); > > , CHANGES_URL ("gcc-10/changes.html#empty_base"); > > where the macro would just use preprocessor string concatenation? > > Alternatively 'url' could be static I guess (and never freed) The reason was that DOCUMENTATION_ROOT_URL is a macro added through CFLAGS-opts.o += -DDOCUMENTATION_ROOT_URL=\"@DOCUMENTATION_ROOT_URL@\" and so it is available in opts.o only and I've followed that. I didn't want to add a long -D option to every *.o file out there. But, thinking about it now, there is no reason why DOCUMENTATION_ROOT_URL and CHANGES_ROOT_URL couldn't be in config.h, i.e. AC_DEFINE rather than AC_SUBST, and then we can indeed use string concatenation. I've already checked the patch in, so I'll post an incremental patch (together with the doc/ changes requested by Joseph). Jakub
--- gcc/configure.ac.jj 2020-04-29 10:21:25.061999873 +0200 +++ gcc/configure.ac 2020-04-29 13:26:51.515516959 +0200 @@ -986,6 +986,20 @@ AC_ARG_WITH(documentation-root-url, ) AC_SUBST(DOCUMENTATION_ROOT_URL) +# Allow overriding the default URL for GCC changes +AC_ARG_WITH(changes-root-url, + AS_HELP_STRING([--with-changes-root-url=URL], + [Root for GCC changes URLs]), + [case "$withval" in + yes) AC_MSG_ERROR([changes root URL not specified]) ;; + no) AC_MSG_ERROR([changes root URL not specified]) ;; + *) CHANGES_ROOT_URL="$withval" + ;; + esac], + CHANGES_ROOT_URL="https://gcc.gnu.org/" +) +AC_SUBST(CHANGES_ROOT_URL) + # Sanity check enable_languages in case someone does not run the toplevel # configure # script. AC_ARG_ENABLE(languages, --- gcc/Makefile.in.jj 2020-02-27 09:28:46.129960135 +0100 +++ gcc/Makefile.in 2020-04-29 13:38:42.455008718 +0200 @@ -2187,6 +2187,7 @@ lto-wrapper$(exeext): $(LTO_WRAPPER_OBJS mv -f T$@ $@ CFLAGS-opts.o += -DDOCUMENTATION_ROOT_URL=\"@DOCUMENTATION_ROOT_URL@\" +CFLAGS-opts.o += -DCHANGES_ROOT_URL=\"@CHANGES_ROOT_URL@\" # Files used by all variants of C or by the stand-alone pre-processor. --- gcc/pretty-print.c.jj 2020-04-25 00:08:33.359759328 +0200 +++ gcc/pretty-print.c 2020-04-29 14:30:46.791792554 +0200 @@ -1020,6 +1020,8 @@ pp_indent (pretty_printer *pp) pp_space (pp); } +static const char *end_url_string (pretty_printer *); + /* The following format specifiers are recognized as being client independent: %d, %i: (signed) integer in base ten. %u: unsigned integer in base ten. @@ -1038,6 +1040,8 @@ pp_indent (pretty_printer *pp) %%: '%'. %<: opening quote. %>: closing quote. + %{: URL start. + %}: URL end. %': apostrophe (should only be used in untranslated messages; translations should use appropriate punctuation directly). %@: diagnostic_event_id_ptr, for which event_id->known_p () must be true. @@ -1051,7 +1055,7 @@ pp_indent (pretty_printer *pp) Arguments can be used sequentially, or through %N$ resp. *N$ notation Nth argument after the format string. If %N$ / *N$ notation is used, it must be used for all arguments, except %m, %%, - %<, %> and %', which may not have a number, as they do not consume + %<, %>, %} and %', which may not have a number, as they do not consume an argument. When %M$.*N$s is used, M must be N + 1. (This may also be written %M$.*s, provided N is not otherwise used.) The format string must have conversion specifiers with argument numbers @@ -1084,7 +1088,7 @@ pp_format (pretty_printer *pp, text_info /* Formatting phase 1: split up TEXT->format_spec into chunks in pp_buffer (PP)->args[]. Even-numbered chunks are to be output verbatim, odd-numbered chunks are format specifiers. - %m, %%, %<, %>, and %' are replaced with the appropriate text at + %m, %%, %<, %>, %} and %' are replaced with the appropriate text at this point. */ memset (formatters, 0, sizeof formatters); @@ -1133,6 +1137,15 @@ pp_format (pretty_printer *pp, text_info p++; continue; + case '}': + { + const char *endurlstr = end_url_string (pp); + obstack_grow (&buffer->chunk_obstack, endurlstr, + strlen (endurlstr)); + } + p++; + continue; + case 'R': { const char *colorstr = colorize_stop (pp_show_color (pp)); @@ -1445,6 +1458,10 @@ pp_format (pretty_printer *pp, text_info } break; + case '{': + pp_begin_url (pp, va_arg (*text->args_ptr, const char *)); + break; + default: { bool ok; @@ -2172,18 +2189,41 @@ void pp_begin_url (pretty_printer *pp, const char *url) { switch (pp->url_format) - { - case URL_FORMAT_NONE: - break; - case URL_FORMAT_ST: - pp_printf (pp, "\33]8;;%s\33\\", url); - break; - case URL_FORMAT_BEL: - pp_printf (pp, "\33]8;;%s\a", url); - break; - default: - gcc_unreachable (); - } + { + case URL_FORMAT_NONE: + break; + case URL_FORMAT_ST: + pp_string (pp, "\33]8;;"); + pp_string (pp, url); + pp_string (pp, "\33\\"); + break; + case URL_FORMAT_BEL: + pp_string (pp, "\33]8;;"); + pp_string (pp, url); + pp_string (pp, "\a"); + break; + default: + gcc_unreachable (); + } +} + +/* Helper function for pp_end_url and pp_format, return the "close URL" escape + sequence string. */ + +static const char * +end_url_string (pretty_printer *pp) +{ + switch (pp->url_format) + { + case URL_FORMAT_NONE: + return ""; + case URL_FORMAT_ST: + return "\33]8;;\33\\"; + case URL_FORMAT_BEL: + return "\33]8;;\a"; + default: + gcc_unreachable (); + } } /* If URL-printing is enabled, write a "close URL" escape sequence to PP. */ @@ -2191,19 +2231,8 @@ pp_begin_url (pretty_printer *pp, const void pp_end_url (pretty_printer *pp) { - switch (pp->url_format) - { - case URL_FORMAT_NONE: - break; - case URL_FORMAT_ST: - pp_string (pp, "\33]8;;\33\\"); - break; - case URL_FORMAT_BEL: - pp_string (pp, "\33]8;;\a"); - break; - default: - gcc_unreachable (); - } + if (pp->url_format != URL_FORMAT_NONE) + pp_string (pp, end_url_string (pp)); } #if CHECKING_P --- gcc/opts.h.jj 2020-02-24 09:02:27.332710905 +0100 +++ gcc/opts.h 2020-04-29 13:56:55.621826605 +0200 @@ -464,6 +464,7 @@ extern void parse_options_from_collect_g int *); extern void prepend_xassembler_to_collect_as_options (const char *, obstack *); +extern char *get_changes_url (const char *); /* Set OPTION in OPTS to VALUE if the option is not set in OPTS_SET. */ --- gcc/opts.c.jj 2020-04-27 23:28:18.865609469 +0200 +++ gcc/opts.c 2020-04-29 13:42:13.033891253 +0200 @@ -3190,6 +3190,15 @@ get_option_url (diagnostic_context *, in return NULL; } +/* Given "gcc-10/changes.html#foobar", return that URL under + CHANGES_ROOT_URL (see --with-changes-root-url). */ + +char * +get_changes_url (const char *str) +{ + return concat (CHANGES_ROOT_URL, str, NULL); +} + #if CHECKING_P namespace selftest { --- gcc/config/arm/arm.c.jj 2020-04-29 13:12:46.736007298 +0200 +++ gcc/config/arm/arm.c 2020-04-29 14:34:04.878864236 +0200 @@ -6416,6 +6416,8 @@ aapcs_vfp_is_call_or_return_candidate (e && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) != ag_count)) { + const char *url + = get_changes_url ("gcc-10/changes.html#empty_base"); gcc_assert (alt == -1); last_reported_type_uid = uid; /* Use TYPE_MAIN_VARIANT to strip any redundant const @@ -6423,11 +6425,14 @@ aapcs_vfp_is_call_or_return_candidate (e if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) inform (input_location, "parameter passing for argument of " "type %qT with %<[[no_unique_address]]%> members " - "changed in GCC 10.1", TYPE_MAIN_VARIANT (type)); + "changed in %{GCC 10.1%}", + TYPE_MAIN_VARIANT (type), url); else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) inform (input_location, "parameter passing for argument of " "type %qT when C++17 is enabled changed to match " - "C++14 in GCC 10.1", TYPE_MAIN_VARIANT (type)); + "C++14 in %{GCC 10.1%}", + TYPE_MAIN_VARIANT (type), url); + free (url); } *count = ag_count; } --- gcc/config/aarch64/aarch64.c.jj 2020-04-29 13:12:46.715007609 +0200 +++ gcc/config/aarch64/aarch64.c 2020-04-29 14:35:06.712950146 +0200 @@ -16883,6 +16883,8 @@ aarch64_vfp_is_call_or_return_candidate && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) != ag_count)) { + const char *url + = get_changes_url ("gcc-10/changes.html#empty_base"); gcc_assert (alt == -1); last_reported_type_uid = uid; /* Use TYPE_MAIN_VARIANT to strip any redundant const @@ -16890,11 +16892,14 @@ aarch64_vfp_is_call_or_return_candidate if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) inform (input_location, "parameter passing for argument of " "type %qT with %<[[no_unique_address]]%> members " - "changed in GCC 10.1", TYPE_MAIN_VARIANT (type)); + "changed in %{GCC 10.1%}", + TYPE_MAIN_VARIANT (type), url); else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) inform (input_location, "parameter passing for argument of " "type %qT when C++17 is enabled changed to match " - "C++14 in GCC 10.1", TYPE_MAIN_VARIANT (type)); + "C++14 in %{GCC 10.1%}", + TYPE_MAIN_VARIANT (type), url); + free (url); } if (is_ha != NULL) *is_ha = true; --- gcc/config/rs6000/rs6000-call.c.jj 2020-04-29 09:00:46.642524337 +0200 +++ gcc/config/rs6000/rs6000-call.c 2020-04-29 13:50:45.873299042 +0200 @@ -68,6 +68,7 @@ #include "tree-vrp.h" #include "tree-ssanames.h" #include "targhooks.h" +#include "opts.h" #include "rs6000-internal.h" @@ -5747,16 +5748,18 @@ rs6000_discover_homogeneous_aggregate (m unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); if (uid != last_reported_type_uid) { + const char *url + = get_changes_url ("gcc-10/changes.html#empty_base"); if (empty_base_seen & 1) inform (input_location, "parameter passing for argument of type %qT " "when C++17 is enabled changed to match C++14 " - "in GCC 10.1", type); + "in %{GCC 10.1%}", type, url); else inform (input_location, "parameter passing for argument of type %qT " "with %<[[no_unique_address]]%> members " - "changed in GCC 10.1", type); + "changed in %{GCC 10.1%}", type, url); last_reported_type_uid = uid; } } --- gcc/c-family/c-format.c.jj 2020-02-10 15:49:50.821300965 +0100 +++ gcc/c-family/c-format.c 2020-04-29 13:56:26.926250904 +0200 @@ -757,6 +757,8 @@ static const format_char_info asm_fprint { "<", 0, STD_C89, NOARGUMENTS, "", "<", NULL }, \ { ">", 0, STD_C89, NOARGUMENTS, "", ">", NULL }, \ { "'" , 0, STD_C89, NOARGUMENTS, "", "", NULL }, \ + { "{", 1, STD_C89, { T89_C, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN }, "", "cR", NULL }, \ + { "}", 0, STD_C89, NOARGUMENTS, "", "", NULL }, \ { "R", 0, STD_C89, NOARGUMENTS, "", "\\", NULL }, \ { "m", 0, STD_C89, NOARGUMENTS, "q", "", NULL }, \ { "Z", 1, STD_C89, { T89_I, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN }, "", "", &gcc_diag_char_table[0] } --- gcc/configure.jj 2020-04-29 10:21:25.060999888 +0200 +++ gcc/configure 2020-04-29 13:28:07.423395043 +0200 @@ -819,6 +819,7 @@ accel_dir_suffix real_target_noncanonical enable_as_accelerator gnat_install_lib +CHANGES_ROOT_URL DOCUMENTATION_ROOT_URL REPORT_BUGS_TEXI REPORT_BUGS_TO @@ -967,6 +968,7 @@ with_specs with_pkgversion with_bugurl with_documentation_root_url +with_changes_root_url enable_languages with_multilib_list with_zstd @@ -1803,6 +1805,8 @@ Optional Packages: --with-bugurl=URL Direct users to URL to report a bug --with-documentation-root-url=URL Root for documentation URLs + --with-changes-root-url=URL + Root for GCC changes URLs --with-multilib-list select multilibs (AArch64, SH and x86-64 only) --with-zstd=PATH specify prefix directory for installed zstd library. Equivalent to --with-zstd-include=PATH/include plus @@ -7857,6 +7861,23 @@ fi +# Allow overriding the default URL for GCC changes + +# Check whether --with-changes-root-url was given. +if test "${with_changes_root_url+set}" = set; then : + withval=$with_changes_root_url; case "$withval" in + yes) as_fn_error $? "changes root URL not specified" "$LINENO" 5 ;; + no) as_fn_error $? "changes root URL not specified" "$LINENO" 5 ;; + *) CHANGES_ROOT_URL="$withval" + ;; + esac +else + CHANGES_ROOT_URL="https://gcc.gnu.org/" + +fi + + + # Sanity check enable_languages in case someone does not run the toplevel # configure # script. # Check whether --enable-languages was given. @@ -18988,7 +19009,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 18991 "configure" +#line 19012 "configure" #include "confdefs.h" #if HAVE_DLFCN_H @@ -19094,7 +19115,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 19097 "configure" +#line 19118 "configure" #include "confdefs.h" #if HAVE_DLFCN_H
Hi! The following patch attempts to use the diagnostics URL support if available to provide more information about the C++17 empty base and C++20 [[no_unique_address]] empty class ABI changes in -Wpsabi diagnostics. GCC 10.1 at the end of the diagnostics is then in some terminals underlined with a dotted line and points to a (to be written) anchor in gcc-10/changes.html which we need to write anyway. Ok for trunk if this passes bootstrap/regtest? 2020-04-29 Jakub Jelinek <jakub@redhat.com> * configure.ac (-with-changes-root-url): New configure option, defaulting to https://gcc.gnu.org/. * Makefile.in (CFLAGS-opts.o): Define CHANGES_ROOT_URL for opts.c. * pretty-print.c (end_url_string): New function. (pp_format): Handle %{ and %} for URLs. (pp_begin_url): Use pp_string instead of pp_printf. (pp_end_url): Use end_url_string. * opts.h (get_changes_url): Declare. * opts.c (get_changes_url): New function. * config/rs6000/rs6000-call.c: Include opts.h. (rs6000_discover_homogeneous_aggregate): Use %{GCC 10.1%} instead of just GCC 10.1 in diagnostics and add URL. * config/arm/arm.c (aapcs_vfp_is_call_or_return_candidate): Likewise. * config/aarch64/aarch64.c (aarch64_vfp_is_call_or_return_candidate): Likewise. * configure: Regenerated. * c-format.c (PP_FORMAT_CHAR_TABLE): Add %{ and %}. Jakub