* [PATCH] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs @ 2020-04-29 12:46 Jakub Jelinek 2020-04-29 12:57 ` Richard Sandiford ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Jakub Jelinek @ 2020-04-29 12:46 UTC (permalink / raw) To: David Malcolm, Jonathan Wakely, Jason Merrill; +Cc: gcc-patches 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"); 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 Jakub ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 12:46 [PATCH] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs Jakub Jelinek @ 2020-04-29 12:57 ` Richard Sandiford 2020-04-29 13:24 ` David Malcolm 2020-04-30 6:42 ` Richard Biener 2 siblings, 0 replies; 16+ messages in thread From: Richard Sandiford @ 2020-04-29 12:57 UTC (permalink / raw) To: Jakub Jelinek via Gcc-patches Cc: David Malcolm, Jonathan Wakely, Jason Merrill, Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 12:46 [PATCH] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs Jakub Jelinek 2020-04-29 12:57 ` Richard Sandiford @ 2020-04-29 13:24 ` David Malcolm 2020-04-29 13:52 ` [PATCH v2] " Jakub Jelinek 2020-04-29 15:13 ` [PATCH] " Jonathan Wakely 2020-04-30 6:42 ` Richard Biener 2 siblings, 2 replies; 16+ messages in thread From: David Malcolm @ 2020-04-29 13:24 UTC (permalink / raw) To: Jakub Jelinek, Jonathan Wakely, Jason Merrill; +Cc: gcc-patches 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] } ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 13:24 ` David Malcolm @ 2020-04-29 13:52 ` Jakub Jelinek 2020-04-29 14:25 ` David Malcolm 2020-04-29 21:42 ` Joseph Myers 2020-04-29 15:13 ` [PATCH] " Jonathan Wakely 1 sibling, 2 replies; 16+ messages in thread From: Jakub Jelinek @ 2020-04-29 13:52 UTC (permalink / raw) To: David Malcolm; +Cc: Jonathan Wakely, Jason Merrill, gcc-patches On Wed, Apr 29, 2020 at 09:24:09AM -0400, David Malcolm wrote: > Overall I like the idea; some nits inline below (and I think it won't > bootstrap due to a const-correctness issue). I'll start a bootstrap soon. > 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. All we have right now is g++.target/ tests that check the wording, but those test have from the common code the URLs disabled. > I think this needs a little more explanation; perhaps something like: Ack, added. > 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. Ok. > Although the return type is a "char *" rather than "const char *" I > think this comment could be improved by clarifying ownership. How > about: Ok. > 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. I'd prefer not to at this point, all that could be commonized are the two inform calls perhaps with a bool or enum arg which one it is, but usually the backends prefer to keep their -Wpsabi stuff visible there. Yes, it affects 4 targets (s390 too; haven't touched that one, because there is a pending patch with two alternatives). > > + 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. Changed to char *, one could use free (CONST_CAST (char *, url)) too though. > > @@ -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); > > 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 think I can't do that, because there are two inform calls and while one has the changed in GCC 10.1 in that order, the other doesn't, as it has changed to match C++14 in GCC 10.1. Additionally, for translations, I think some translators will need to reorder the words in various ways, and am not sure what they would do if e.g. the translated changed word is way appart from in GCC 10.1. One could use ... type %1$qT ... %2${changed%} ... %2${in GCC 10.1%} perhaps, which would mean two separate underlined words, but not sure if they'd figure it out. Is just %{in GCC 10.1%} good enough (in the patch below)? I've also included the missing free (url); in rs6000-call.c Richard Sandiford reported. 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 (get_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 get_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 %{in GCC 10.1%} instead of just in 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 15:28:01.907010894 +0200 +++ gcc/configure.ac 2020-04-29 15:28:27.748628849 +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-04-29 15:28:01.875011367 +0200 +++ gcc/Makefile.in 2020-04-29 15:28:27.749628835 +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-29 15:28:01.958010139 +0200 +++ gcc/pretty-print.c 2020-04-29 15:36:51.911175267 +0200 @@ -1020,6 +1020,8 @@ pp_indent (pretty_printer *pp) pp_space (pp); } +static const char *get_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. Consumes a const char * argument for the URL. + %}: URL end. Does not consume any arguments. %': 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 = get_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 * +get_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, get_end_url_string (pp)); } #if CHECKING_P --- gcc/opts.h.jj 2020-04-29 15:28:01.944010346 +0200 +++ gcc/opts.h 2020-04-29 15:28:27.750628820 +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-29 15:28:01.919010716 +0200 +++ gcc/opts.c 2020-04-29 15:30:43.172626729 +0200 @@ -3190,6 +3190,16 @@ 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). + 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 15:28:01.902010968 +0200 +++ gcc/config/arm/arm.c 2020-04-29 15:36:06.059853139 +0200 @@ -6416,6 +6416,7 @@ aapcs_vfp_is_call_or_return_candidate (e && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) != ag_count)) { + 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 +6424,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 15:28:01.896011056 +0200 +++ gcc/config/aarch64/aarch64.c 2020-04-29 15:35:44.835166928 +0200 @@ -16883,6 +16883,7 @@ aarch64_vfp_is_call_or_return_candidate && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) != ag_count)) { + 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 +16891,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 15:28:01.903010953 +0200 +++ gcc/config/rs6000/rs6000-call.c 2020-04-29 15:35:02.507792697 +0200 @@ -68,6 +68,7 @@ #include "tree-vrp.h" #include "tree-ssanames.h" #include "targhooks.h" +#include "opts.h" #include "rs6000-internal.h" @@ -5747,17 +5748,20 @@ rs6000_discover_homogeneous_aggregate (m unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); if (uid != last_reported_type_uid) { + 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; + free (url); } } return true; --- gcc/c-family/c-format.c.jj 2020-04-29 15:28:01.890011145 +0200 +++ gcc/c-family/c-format.c 2020-04-29 15:28:27.760628672 +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 15:28:01.906010908 +0200 +++ gcc/configure 2020-04-29 15:28:27.762628642 +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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 13:52 ` [PATCH v2] " Jakub Jelinek @ 2020-04-29 14:25 ` David Malcolm 2020-04-29 14:42 ` Jakub Jelinek 2020-04-29 21:42 ` Joseph Myers 1 sibling, 1 reply; 16+ messages in thread From: David Malcolm @ 2020-04-29 14:25 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Jonathan Wakely, Jason Merrill, gcc-patches On Wed, 2020-04-29 at 15:52 +0200, Jakub Jelinek wrote: > On Wed, Apr 29, 2020 at 09:24:09AM -0400, David Malcolm wrote: [...] > > 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. > > I'd prefer not to at this point, all that could be commonized are > the two inform calls perhaps with a bool or enum arg which one it is, > but usually the backends prefer to keep their -Wpsabi stuff visible > there. > Yes, it affects 4 targets (s390 too; haven't touched that one, > because > there is a pending patch with two alternatives). One other benefit is to move the allocation/free of the URL string so that it's guarded by the same condition as the "inform" call. Is there a chance this patch could be doing a bunch of extra allocation and freeing of URL strings that never get used? How often does this code get called? Idea: introduce a static allocation for this const char * get_url_for_gcc_10_empty_base () { static const char *url; if (!url) url = get_changes_url ("gcc-10/changes.html#empty_base"); return url; } or somesuch? > > > + 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. > > Changed to char *, one could use free (CONST_CAST (char *, url)) too > though. > > > > @@ -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); > > > > 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 think I can't do that, because there are two inform calls and while > one > has the changed in GCC 10.1 in that order, the other doesn't, as it > has > changed to match C++14 in GCC 10.1. Additionally, for translations, > I think > some translators will need to reorder the words in various ways, and > am not > sure what they would do if e.g. the translated changed word is way > appart > from in GCC 10.1. One could use > ... type %1$qT ... %2${changed%} ... %2${in GCC 10.1%} perhaps, which > would mean two separate underlined words, but not sure if they'd > figure it > out. > Is just %{in GCC 10.1%} good enough (in the patch below)? Yes. > I've also included the missing free (url); in rs6000-call.c Richard > Sandiford reported. Thanks. > 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 (get_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 get_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 %{in GCC 10.1%} > instead > of just in 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 %}. > [...] LGTM, with the caveat about allocation noted above. Thanks Dave ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 14:25 ` David Malcolm @ 2020-04-29 14:42 ` Jakub Jelinek 2020-04-29 15:34 ` David Malcolm 0 siblings, 1 reply; 16+ messages in thread From: Jakub Jelinek @ 2020-04-29 14:42 UTC (permalink / raw) To: David Malcolm; +Cc: Jonathan Wakely, Jason Merrill, gcc-patches On Wed, Apr 29, 2020 at 10:25:38AM -0400, David Malcolm wrote: > > I'd prefer not to at this point, all that could be commonized are > > the two inform calls perhaps with a bool or enum arg which one it is, > > but usually the backends prefer to keep their -Wpsabi stuff visible > > there. > > Yes, it affects 4 targets (s390 too; haven't touched that one, > > because > > there is a pending patch with two alternatives). > > One other benefit is to move the allocation/free of the URL string so > that it's guarded by the same condition as the "inform" call. > > Is there a chance this patch could be doing a bunch of extra allocation > and freeing of URL strings that never get used? How often does this > code get called? I think it will be called infrequently, and emitting a diagnostic is already slow. E.g. the URLs for warnings are also allocated and freed during the warning{,_at} call. So, not sure it is worth wasting a static variable on it, especially for all the other 52 or how many backends that don't need it. But if you feel strongly about it, can do it that way, though not really sure what is the best spot to place it, calls.[ch] ? Jakub ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 14:42 ` Jakub Jelinek @ 2020-04-29 15:34 ` David Malcolm 2020-04-29 15:49 ` Jakub Jelinek 0 siblings, 1 reply; 16+ messages in thread From: David Malcolm @ 2020-04-29 15:34 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Jonathan Wakely, Jason Merrill, gcc-patches On Wed, 2020-04-29 at 16:42 +0200, Jakub Jelinek wrote: > On Wed, Apr 29, 2020 at 10:25:38AM -0400, David Malcolm wrote: > > > I'd prefer not to at this point, all that could be commonized are > > > the two inform calls perhaps with a bool or enum arg which one it > > > is, > > > but usually the backends prefer to keep their -Wpsabi stuff > > > visible > > > there. > > > Yes, it affects 4 targets (s390 too; haven't touched that one, > > > because > > > there is a pending patch with two alternatives). > > > > One other benefit is to move the allocation/free of the URL string > > so > > that it's guarded by the same condition as the "inform" call. > > > > Is there a chance this patch could be doing a bunch of extra > > allocation > > and freeing of URL strings that never get used? How often does > > this > > code get called? > > I think it will be called infrequently, and emitting a diagnostic is > already > slow. I was more concerned about the case for which the url is built but then the "inform" is not called - I was worried it might happen per field of a struct - but if you think that code path is infrequent, then I trust your judgment. > E.g. the URLs for warnings are also allocated and freed during the > warning{,_at} call. I believe I fixed that in abb4852434b3dee26301cc406472ac9450c621aa so that it only builds the URLs if they're going to be used. > So, not sure it is worth wasting a static variable on > it, especially for all the other 52 or how many backends that don't > need it. > > But if you feel strongly about it, can do it that way, though not > really > sure what is the best spot to place it, calls.[ch] ? I don't feel strongly about it. Thanks Dave ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 15:34 ` David Malcolm @ 2020-04-29 15:49 ` Jakub Jelinek 2020-04-29 15:53 ` David Malcolm 0 siblings, 1 reply; 16+ messages in thread From: Jakub Jelinek @ 2020-04-29 15:49 UTC (permalink / raw) To: David Malcolm; +Cc: Jonathan Wakely, Jason Merrill, gcc-patches On Wed, Apr 29, 2020 at 11:34:06AM -0400, David Malcolm wrote: > > I think it will be called infrequently, and emitting a diagnostic is > > already > > slow. > > I was more concerned about the case for which the url is built but then > the "inform" is not called - I was worried it might happen per field of > a struct - but if you think that code path is infrequent, then I trust > your judgment. When get_changes_url is called, then inform is also always called, and I think inform even doesn't do any -Wsystem-headers disabling. On rs6000 it should be clear from the patch itself, on aarch64/arm it is less so, but it does: if (... && warn_psabi && warn_psabi_flags && ...) { char *url = get_changes_url ("..."); if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) inform (..., url); else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) inform (..., url); } and the two bits are the only ones ever set in warn_psabi_flags. So, this is not called e.g. with -Wno-psabi or not called again if it is the same type as reported last time. BTW, the patch successfully passed bootstrap/regtest on {x86_64,i686,powerpc64le}-linux. Jakub ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 15:49 ` Jakub Jelinek @ 2020-04-29 15:53 ` David Malcolm 0 siblings, 0 replies; 16+ messages in thread From: David Malcolm @ 2020-04-29 15:53 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Jonathan Wakely, Jason Merrill, gcc-patches On Wed, 2020-04-29 at 17:49 +0200, Jakub Jelinek wrote: > On Wed, Apr 29, 2020 at 11:34:06AM -0400, David Malcolm wrote: > > > I think it will be called infrequently, and emitting a diagnostic > > > is > > > already > > > slow. > > > > I was more concerned about the case for which the url is built but > > then > > the "inform" is not called - I was worried it might happen per > > field of > > a struct - but if you think that code path is infrequent, then I > > trust > > your judgment. > > When get_changes_url is called, then inform is also always called, > and > I think inform even doesn't do any -Wsystem-headers disabling. > On rs6000 it should be clear from the patch itself, on aarch64/arm it > is less so, > but it does: > if (... && warn_psabi && warn_psabi_flags && ...) > { > char *url = get_changes_url ("..."); > if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) > inform (..., url); > else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) > inform (..., url); > } > and the two bits are the only ones ever set in warn_psabi_flags. > So, this is not called e.g. with -Wno-psabi or not called again if it > is the > same type as reported last time. Aha - thanks for clarifying. > BTW, the patch successfully passed bootstrap/regtest on > {x86_64,i686,powerpc64le}-linux. The v2 patch looks good to me. Thanks Dave ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 13:52 ` [PATCH v2] " Jakub Jelinek 2020-04-29 14:25 ` David Malcolm @ 2020-04-29 21:42 ` Joseph Myers 2020-04-29 21:45 ` Jakub Jelinek 1 sibling, 1 reply; 16+ messages in thread From: Joseph Myers @ 2020-04-29 21:42 UTC (permalink / raw) To: Jakub Jelinek; +Cc: David Malcolm, Jonathan Wakely, gcc-patches This is missing documentation for the new configure option in install.texi. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 21:42 ` Joseph Myers @ 2020-04-29 21:45 ` Jakub Jelinek 0 siblings, 0 replies; 16+ messages in thread From: Jakub Jelinek @ 2020-04-29 21:45 UTC (permalink / raw) To: Joseph Myers; +Cc: David Malcolm, Jonathan Wakely, gcc-patches On Wed, Apr 29, 2020 at 09:42:59PM +0000, Joseph Myers wrote: > This is missing documentation for the new configure option in > install.texi. I was considering it, but then didn't find any docs for the --with-documentation-root= option either, so punted on that. I'll try to write something for both. Jakub ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 13:24 ` David Malcolm 2020-04-29 13:52 ` [PATCH v2] " Jakub Jelinek @ 2020-04-29 15:13 ` Jonathan Wakely 1 sibling, 0 replies; 16+ messages in thread From: Jonathan Wakely @ 2020-04-29 15:13 UTC (permalink / raw) To: David Malcolm; +Cc: Jakub Jelinek, Jason Merrill, gcc-patches 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-29 12:46 [PATCH] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs Jakub Jelinek 2020-04-29 12:57 ` Richard Sandiford 2020-04-29 13:24 ` David Malcolm @ 2020-04-30 6:42 ` Richard Biener 2020-04-30 7:15 ` Jakub Jelinek 2020-04-30 8:44 ` [PATCH] --with-{documentation,changes}-root-url tweaks Jakub Jelinek 2 siblings, 2 replies; 16+ messages in thread From: Richard Biener @ 2020-04-30 6:42 UTC (permalink / raw) To: Jakub Jelinek; +Cc: David Malcolm, Jonathan Wakely, Jason Merrill, GCC Patches 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs 2020-04-30 6:42 ` Richard Biener @ 2020-04-30 7:15 ` Jakub Jelinek 2020-04-30 8:44 ` [PATCH] --with-{documentation,changes}-root-url tweaks Jakub Jelinek 1 sibling, 0 replies; 16+ messages in thread From: Jakub Jelinek @ 2020-04-30 7:15 UTC (permalink / raw) To: Richard Biener; +Cc: David Malcolm, Jonathan Wakely, Jason Merrill, GCC Patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] --with-{documentation,changes}-root-url tweaks 2020-04-30 6:42 ` Richard Biener 2020-04-30 7:15 ` Jakub Jelinek @ 2020-04-30 8:44 ` Jakub Jelinek 2020-04-30 9:45 ` Richard Biener 1 sibling, 1 reply; 16+ messages in thread From: Jakub Jelinek @ 2020-04-30 8:44 UTC (permalink / raw) To: Richard Biener, David Malcolm, Joseph S. Myers; +Cc: gcc-patches Hi! On Thu, Apr 30, 2020 at 08:42:39AM +0200, Richard Biener wrote: > , CHANGES_URL ("gcc-10/changes.html#empty_base"); > > where the macro would just use preprocessor string concatenation? Ok, the following patch implements it (doesn't introduce a separate macro and just uses CHANGES_ROOT_URL "gcc-10/changes.html#empty_base"), in addition adds the documentation Joseph requested. Tested on x86_64-linux and with crosses to s390x-linux and powerpc64le-linux, ok for trunk? 2020-04-30 Jakub Jelinek <jakub@redhat.com> * configure.ac (--with-documentation-root-url, --with-changes-root-url): Diagnose URL not ending with /, use AC_DEFINE_UNQUOTED instead of AC_SUBST. * opts.h (get_changes_url): Remove. * opts.c (get_changes_url): Remove. * Makefile.in (CFLAGS-opts.o): Don't add -DDOCUMENTATION_ROOT_URL or -DCHANGES_ROOT_URL. * doc/install.texi (--with-documentation-root-url, --with-changes-root-url): Document. * config/arm/arm.c (aapcs_vfp_is_call_or_return_candidate): Don't call get_changes_url and free, change url variable type to const char * and set it to CHANGES_ROOT_URL "gcc-10/changes.html#empty_base". * config/s390/s390.c (s390_function_arg_vector, s390_function_arg_float): Likewise. * config/aarch64/aarch64.c (aarch64_vfp_is_call_or_return_candidate): Likewise. * config/rs6000/rs6000-call.c (rs6000_discover_homogeneous_aggregate): Likewise. * config.in: Regenerate. * configure: Regenerate. --- gcc/configure.ac.jj 2020-04-29 22:41:05.086585150 +0200 +++ gcc/configure.ac 2020-04-30 09:35:29.800894085 +0200 @@ -979,12 +979,13 @@ AC_ARG_WITH(documentation-root-url, [case "$withval" in yes) AC_MSG_ERROR([documentation root URL not specified]) ;; no) AC_MSG_ERROR([documentation root URL not specified]) ;; - *) DOCUMENTATION_ROOT_URL="$withval" - ;; + */) DOCUMENTATION_ROOT_URL="$withval" ;; + *) AC_MSG_ERROR([documentation root URL does not end with /]) ;; esac], DOCUMENTATION_ROOT_URL="https://gcc.gnu.org/onlinedocs/" ) -AC_SUBST(DOCUMENTATION_ROOT_URL) +AC_DEFINE_UNQUOTED(DOCUMENTATION_ROOT_URL,"$DOCUMENTATION_ROOT_URL", + [Define to the root for documentation URLs.]) # Allow overriding the default URL for GCC changes AC_ARG_WITH(changes-root-url, @@ -993,12 +994,13 @@ AC_ARG_WITH(changes-root-url, [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" - ;; + */) CHANGES_ROOT_URL="$withval" ;; + *) AC_MSG_ERROR([changes root URL does not end with /]) ;; esac], CHANGES_ROOT_URL="https://gcc.gnu.org/" ) -AC_SUBST(CHANGES_ROOT_URL) +AC_DEFINE_UNQUOTED(CHANGES_ROOT_URL,"$CHANGES_ROOT_URL", + [Define to the root for URLs about GCC changes.]) # Sanity check enable_languages in case someone does not run the toplevel # configure # script. --- gcc/opts.h.jj 2020-04-29 22:41:05.089585106 +0200 +++ gcc/opts.h 2020-04-30 09:38:20.110367236 +0200 @@ -464,7 +464,6 @@ 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-29 22:41:05.090585091 +0200 +++ gcc/opts.c 2020-04-30 09:37:17.039303011 +0200 @@ -3190,16 +3190,6 @@ 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). - 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/Makefile.in.jj 2020-04-29 22:41:05.088585120 +0200 +++ gcc/Makefile.in 2020-04-30 09:36:14.201235329 +0200 @@ -2186,9 +2186,6 @@ lto-wrapper$(exeext): $(LTO_WRAPPER_OBJS $(LTO_WRAPPER_OBJS) libcommon-target.a $(LIBS) 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. CFLAGS-c-family/c-opts.o += @TARGET_SYSTEM_ROOT_DEFINE@ --- gcc/doc/install.texi.jj 2020-04-23 14:42:07.614123663 +0200 +++ gcc/doc/install.texi 2020-04-30 10:13:09.500365247 +0200 @@ -684,6 +684,19 @@ if you determine that they are not bugs The default value refers to the FSF's GCC bug tracker. +@item --with-documentation-root-url=@var{url} +Specify the URL root that contains GCC option documentation. The @var{url} +should end with a @code{/} character. + +The default value is @uref{https://gcc.gnu.org/onlinedocs/,,https://gcc.gnu.org/onlinedocs/}. + +@item --with-changes-root-url=@var{url} +Specify the URL root that contains information about changes in GCC +releases like @code{gcc-@var{version}/changes.html}. +The @var{url} should end with a @code{/} character. + +The default value is @uref{https://gcc.gnu.org/,,https://gcc.gnu.org/}. + @end table @heading Target specification --- gcc/config/arm/arm.c.jj 2020-04-29 22:41:05.096585003 +0200 +++ gcc/config/arm/arm.c 2020-04-30 09:39:24.493412004 +0200 @@ -6416,7 +6416,8 @@ aapcs_vfp_is_call_or_return_candidate (e && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) != ag_count)) { - char *url = get_changes_url ("gcc-10/changes.html#empty_base"); + const char *url + = CHANGES_ROOT_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 @@ -6431,7 +6432,6 @@ aapcs_vfp_is_call_or_return_candidate (e "type %qT when C++17 is enabled changed to match " "C++14 %{in GCC 10.1%}", TYPE_MAIN_VARIANT (type), url); - free (url); } *count = ag_count; } --- gcc/config/s390/s390.c.jj 2020-04-29 22:41:05.101584929 +0200 +++ gcc/config/s390/s390.c 2020-04-30 09:41:23.234650255 +0200 @@ -11960,7 +11960,7 @@ s390_function_arg_vector (machine_mode m unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type)); if (uid != last_reported_type_uid) { - char *url = get_changes_url ("gcc-10/changes.html#empty_base"); + const char *url = CHANGES_ROOT_URL "gcc-10/changes.html#empty_base"; last_reported_type_uid = uid; if (empty_base_seen & 1) inform (input_location, @@ -11972,7 +11972,6 @@ s390_function_arg_vector (machine_mode m "parameter passing for argument of type %qT with " "%<[[no_unique_address]]%> members changed " "%{in GCC 10.1%}", orig_type, url); - free (url); } } return true; @@ -12038,7 +12037,7 @@ s390_function_arg_float (machine_mode mo unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type)); if (uid != last_reported_type_uid) { - char *url = get_changes_url ("gcc-10/changes.html#empty_base"); + const char *url = CHANGES_ROOT_URL "gcc-10/changes.html#empty_base"; last_reported_type_uid = uid; if (empty_base_seen & 1) inform (input_location, @@ -12050,7 +12049,6 @@ s390_function_arg_float (machine_mode mo "parameter passing for argument of type %qT with " "%<[[no_unique_address]]%> members changed " "%{in GCC 10.1%}", orig_type, url); - free (url); } } --- gcc/config/aarch64/aarch64.c.jj 2020-04-29 22:41:05.098584973 +0200 +++ gcc/config/aarch64/aarch64.c 2020-04-30 09:40:07.694771031 +0200 @@ -16883,7 +16883,8 @@ aarch64_vfp_is_call_or_return_candidate && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) != ag_count)) { - char *url = get_changes_url ("gcc-10/changes.html#empty_base"); + const char *url + = CHANGES_ROOT_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 @@ -16898,7 +16899,6 @@ aarch64_vfp_is_call_or_return_candidate "type %qT when C++17 is enabled changed to match " "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 22:41:05.099584959 +0200 +++ gcc/config/rs6000/rs6000-call.c 2020-04-30 09:40:46.140200621 +0200 @@ -5748,8 +5748,8 @@ rs6000_discover_homogeneous_aggregate (m unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); if (uid != last_reported_type_uid) { - char *url - = get_changes_url ("gcc-10/changes.html#empty_base"); + const char *url + = CHANGES_ROOT_URL "gcc-10/changes.html#empty_base"; if (empty_base_seen & 1) inform (input_location, "parameter passing for argument of type %qT " @@ -5761,7 +5761,6 @@ rs6000_discover_homogeneous_aggregate (m "with %<[[no_unique_address]]%> members " "changed %{in GCC 10.1%}", type, url); last_reported_type_uid = uid; - free (url); } } return true; --- gcc/config.in.jj 2020-02-15 12:51:17.422090432 +0100 +++ gcc/config.in 2020-04-30 09:35:40.289738417 +0200 @@ -24,6 +24,12 @@ #endif +/* Define to the root for URLs about GCC changes. */ +#ifndef USED_FOR_TARGET +#undef CHANGES_ROOT_URL +#endif + + /* Define as the number of bits in a byte, if `limits.h' doesn't. */ #ifndef USED_FOR_TARGET #undef CHAR_BIT @@ -82,6 +88,12 @@ #endif +/* Define to the root for documentation URLs. */ +#ifndef USED_FOR_TARGET +#undef DOCUMENTATION_ROOT_URL +#endif + + /* Define 0/1 if static analyzer feature is enabled. */ #ifndef USED_FOR_TARGET #undef ENABLE_ANALYZER --- gcc/configure.jj 2020-04-29 22:41:05.539578490 +0200 +++ gcc/configure 2020-04-30 09:35:37.772775809 +0200 @@ -819,8 +819,6 @@ accel_dir_suffix real_target_noncanonical enable_as_accelerator gnat_install_lib -CHANGES_ROOT_URL -DOCUMENTATION_ROOT_URL REPORT_BUGS_TEXI REPORT_BUGS_TO PKGVERSION @@ -7851,8 +7849,8 @@ if test "${with_documentation_root_url+s withval=$with_documentation_root_url; case "$withval" in yes) as_fn_error $? "documentation root URL not specified" "$LINENO" 5 ;; no) as_fn_error $? "documentation root URL not specified" "$LINENO" 5 ;; - *) DOCUMENTATION_ROOT_URL="$withval" - ;; + */) DOCUMENTATION_ROOT_URL="$withval" ;; + *) as_fn_error $? "documentation root URL does not end with /" "$LINENO" 5 ;; esac else DOCUMENTATION_ROOT_URL="https://gcc.gnu.org/onlinedocs/" @@ -7860,6 +7858,10 @@ else fi +cat >>confdefs.h <<_ACEOF +#define DOCUMENTATION_ROOT_URL "$DOCUMENTATION_ROOT_URL" +_ACEOF + # Allow overriding the default URL for GCC changes @@ -7868,8 +7870,8 @@ if test "${with_changes_root_url+set}" = 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" - ;; + */) CHANGES_ROOT_URL="$withval" ;; + *) as_fn_error $? "changes root URL does not end with /" "$LINENO" 5 ;; esac else CHANGES_ROOT_URL="https://gcc.gnu.org/" @@ -7877,6 +7879,10 @@ else fi +cat >>confdefs.h <<_ACEOF +#define CHANGES_ROOT_URL "$CHANGES_ROOT_URL" +_ACEOF + # Sanity check enable_languages in case someone does not run the toplevel # configure # script. @@ -19009,7 +19015,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 19012 "configure" +#line 19018 "configure" #include "confdefs.h" #if HAVE_DLFCN_H @@ -19115,7 +19121,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 19118 "configure" +#line 19124 "configure" #include "confdefs.h" #if HAVE_DLFCN_H Jakub ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] --with-{documentation,changes}-root-url tweaks 2020-04-30 8:44 ` [PATCH] --with-{documentation,changes}-root-url tweaks Jakub Jelinek @ 2020-04-30 9:45 ` Richard Biener 0 siblings, 0 replies; 16+ messages in thread From: Richard Biener @ 2020-04-30 9:45 UTC (permalink / raw) To: Jakub Jelinek; +Cc: David Malcolm, Joseph S. Myers, GCC Patches On Thu, Apr 30, 2020 at 10:44 AM Jakub Jelinek <jakub@redhat.com> wrote: > > Hi! > On Thu, Apr 30, 2020 at 08:42:39AM +0200, Richard Biener wrote: > > , CHANGES_URL ("gcc-10/changes.html#empty_base"); > > > > where the macro would just use preprocessor string concatenation? > > Ok, the following patch implements it (doesn't introduce a separate > macro and just uses CHANGES_ROOT_URL "gcc-10/changes.html#empty_base"), > in addition adds the documentation Joseph requested. > > Tested on x86_64-linux and with crosses to s390x-linux and > powerpc64le-linux, ok for trunk? OK. Richard. > 2020-04-30 Jakub Jelinek <jakub@redhat.com> > > * configure.ac (--with-documentation-root-url, > --with-changes-root-url): Diagnose URL not ending with /, > use AC_DEFINE_UNQUOTED instead of AC_SUBST. > * opts.h (get_changes_url): Remove. > * opts.c (get_changes_url): Remove. > * Makefile.in (CFLAGS-opts.o): Don't add -DDOCUMENTATION_ROOT_URL > or -DCHANGES_ROOT_URL. > * doc/install.texi (--with-documentation-root-url, > --with-changes-root-url): Document. > * config/arm/arm.c (aapcs_vfp_is_call_or_return_candidate): Don't call > get_changes_url and free, change url variable type to const char * and > set it to CHANGES_ROOT_URL "gcc-10/changes.html#empty_base". > * config/s390/s390.c (s390_function_arg_vector, > s390_function_arg_float): Likewise. > * config/aarch64/aarch64.c (aarch64_vfp_is_call_or_return_candidate): > Likewise. > * config/rs6000/rs6000-call.c (rs6000_discover_homogeneous_aggregate): > Likewise. > * config.in: Regenerate. > * configure: Regenerate. > > --- gcc/configure.ac.jj 2020-04-29 22:41:05.086585150 +0200 > +++ gcc/configure.ac 2020-04-30 09:35:29.800894085 +0200 > @@ -979,12 +979,13 @@ AC_ARG_WITH(documentation-root-url, > [case "$withval" in > yes) AC_MSG_ERROR([documentation root URL not specified]) ;; > no) AC_MSG_ERROR([documentation root URL not specified]) ;; > - *) DOCUMENTATION_ROOT_URL="$withval" > - ;; > + */) DOCUMENTATION_ROOT_URL="$withval" ;; > + *) AC_MSG_ERROR([documentation root URL does not end with /]) ;; > esac], > DOCUMENTATION_ROOT_URL="https://gcc.gnu.org/onlinedocs/" > ) > -AC_SUBST(DOCUMENTATION_ROOT_URL) > +AC_DEFINE_UNQUOTED(DOCUMENTATION_ROOT_URL,"$DOCUMENTATION_ROOT_URL", > + [Define to the root for documentation URLs.]) > > # Allow overriding the default URL for GCC changes > AC_ARG_WITH(changes-root-url, > @@ -993,12 +994,13 @@ AC_ARG_WITH(changes-root-url, > [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" > - ;; > + */) CHANGES_ROOT_URL="$withval" ;; > + *) AC_MSG_ERROR([changes root URL does not end with /]) ;; > esac], > CHANGES_ROOT_URL="https://gcc.gnu.org/" > ) > -AC_SUBST(CHANGES_ROOT_URL) > +AC_DEFINE_UNQUOTED(CHANGES_ROOT_URL,"$CHANGES_ROOT_URL", > + [Define to the root for URLs about GCC changes.]) > > # Sanity check enable_languages in case someone does not run the toplevel > # configure # script. > --- gcc/opts.h.jj 2020-04-29 22:41:05.089585106 +0200 > +++ gcc/opts.h 2020-04-30 09:38:20.110367236 +0200 > @@ -464,7 +464,6 @@ 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-29 22:41:05.090585091 +0200 > +++ gcc/opts.c 2020-04-30 09:37:17.039303011 +0200 > @@ -3190,16 +3190,6 @@ 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). > - 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/Makefile.in.jj 2020-04-29 22:41:05.088585120 +0200 > +++ gcc/Makefile.in 2020-04-30 09:36:14.201235329 +0200 > @@ -2186,9 +2186,6 @@ lto-wrapper$(exeext): $(LTO_WRAPPER_OBJS > $(LTO_WRAPPER_OBJS) libcommon-target.a $(LIBS) > 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. > > CFLAGS-c-family/c-opts.o += @TARGET_SYSTEM_ROOT_DEFINE@ > --- gcc/doc/install.texi.jj 2020-04-23 14:42:07.614123663 +0200 > +++ gcc/doc/install.texi 2020-04-30 10:13:09.500365247 +0200 > @@ -684,6 +684,19 @@ if you determine that they are not bugs > > The default value refers to the FSF's GCC bug tracker. > > +@item --with-documentation-root-url=@var{url} > +Specify the URL root that contains GCC option documentation. The @var{url} > +should end with a @code{/} character. > + > +The default value is @uref{https://gcc.gnu.org/onlinedocs/,,https://gcc.gnu.org/onlinedocs/}. > + > +@item --with-changes-root-url=@var{url} > +Specify the URL root that contains information about changes in GCC > +releases like @code{gcc-@var{version}/changes.html}. > +The @var{url} should end with a @code{/} character. > + > +The default value is @uref{https://gcc.gnu.org/,,https://gcc.gnu.org/}. > + > @end table > > @heading Target specification > --- gcc/config/arm/arm.c.jj 2020-04-29 22:41:05.096585003 +0200 > +++ gcc/config/arm/arm.c 2020-04-30 09:39:24.493412004 +0200 > @@ -6416,7 +6416,8 @@ aapcs_vfp_is_call_or_return_candidate (e > && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) > != ag_count)) > { > - char *url = get_changes_url ("gcc-10/changes.html#empty_base"); > + const char *url > + = CHANGES_ROOT_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 > @@ -6431,7 +6432,6 @@ aapcs_vfp_is_call_or_return_candidate (e > "type %qT when C++17 is enabled changed to match " > "C++14 %{in GCC 10.1%}", > TYPE_MAIN_VARIANT (type), url); > - free (url); > } > *count = ag_count; > } > --- gcc/config/s390/s390.c.jj 2020-04-29 22:41:05.101584929 +0200 > +++ gcc/config/s390/s390.c 2020-04-30 09:41:23.234650255 +0200 > @@ -11960,7 +11960,7 @@ s390_function_arg_vector (machine_mode m > unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type)); > if (uid != last_reported_type_uid) > { > - char *url = get_changes_url ("gcc-10/changes.html#empty_base"); > + const char *url = CHANGES_ROOT_URL "gcc-10/changes.html#empty_base"; > last_reported_type_uid = uid; > if (empty_base_seen & 1) > inform (input_location, > @@ -11972,7 +11972,6 @@ s390_function_arg_vector (machine_mode m > "parameter passing for argument of type %qT with " > "%<[[no_unique_address]]%> members changed " > "%{in GCC 10.1%}", orig_type, url); > - free (url); > } > } > return true; > @@ -12038,7 +12037,7 @@ s390_function_arg_float (machine_mode mo > unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type)); > if (uid != last_reported_type_uid) > { > - char *url = get_changes_url ("gcc-10/changes.html#empty_base"); > + const char *url = CHANGES_ROOT_URL "gcc-10/changes.html#empty_base"; > last_reported_type_uid = uid; > if (empty_base_seen & 1) > inform (input_location, > @@ -12050,7 +12049,6 @@ s390_function_arg_float (machine_mode mo > "parameter passing for argument of type %qT with " > "%<[[no_unique_address]]%> members changed " > "%{in GCC 10.1%}", orig_type, url); > - free (url); > } > } > > --- gcc/config/aarch64/aarch64.c.jj 2020-04-29 22:41:05.098584973 +0200 > +++ gcc/config/aarch64/aarch64.c 2020-04-30 09:40:07.694771031 +0200 > @@ -16883,7 +16883,8 @@ aarch64_vfp_is_call_or_return_candidate > && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) > != ag_count)) > { > - char *url = get_changes_url ("gcc-10/changes.html#empty_base"); > + const char *url > + = CHANGES_ROOT_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 > @@ -16898,7 +16899,6 @@ aarch64_vfp_is_call_or_return_candidate > "type %qT when C++17 is enabled changed to match " > "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 22:41:05.099584959 +0200 > +++ gcc/config/rs6000/rs6000-call.c 2020-04-30 09:40:46.140200621 +0200 > @@ -5748,8 +5748,8 @@ rs6000_discover_homogeneous_aggregate (m > unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); > if (uid != last_reported_type_uid) > { > - char *url > - = get_changes_url ("gcc-10/changes.html#empty_base"); > + const char *url > + = CHANGES_ROOT_URL "gcc-10/changes.html#empty_base"; > if (empty_base_seen & 1) > inform (input_location, > "parameter passing for argument of type %qT " > @@ -5761,7 +5761,6 @@ rs6000_discover_homogeneous_aggregate (m > "with %<[[no_unique_address]]%> members " > "changed %{in GCC 10.1%}", type, url); > last_reported_type_uid = uid; > - free (url); > } > } > return true; > --- gcc/config.in.jj 2020-02-15 12:51:17.422090432 +0100 > +++ gcc/config.in 2020-04-30 09:35:40.289738417 +0200 > @@ -24,6 +24,12 @@ > #endif > > > +/* Define to the root for URLs about GCC changes. */ > +#ifndef USED_FOR_TARGET > +#undef CHANGES_ROOT_URL > +#endif > + > + > /* Define as the number of bits in a byte, if `limits.h' doesn't. */ > #ifndef USED_FOR_TARGET > #undef CHAR_BIT > @@ -82,6 +88,12 @@ > #endif > > > +/* Define to the root for documentation URLs. */ > +#ifndef USED_FOR_TARGET > +#undef DOCUMENTATION_ROOT_URL > +#endif > + > + > /* Define 0/1 if static analyzer feature is enabled. */ > #ifndef USED_FOR_TARGET > #undef ENABLE_ANALYZER > --- gcc/configure.jj 2020-04-29 22:41:05.539578490 +0200 > +++ gcc/configure 2020-04-30 09:35:37.772775809 +0200 > @@ -819,8 +819,6 @@ accel_dir_suffix > real_target_noncanonical > enable_as_accelerator > gnat_install_lib > -CHANGES_ROOT_URL > -DOCUMENTATION_ROOT_URL > REPORT_BUGS_TEXI > REPORT_BUGS_TO > PKGVERSION > @@ -7851,8 +7849,8 @@ if test "${with_documentation_root_url+s > withval=$with_documentation_root_url; case "$withval" in > yes) as_fn_error $? "documentation root URL not specified" "$LINENO" 5 ;; > no) as_fn_error $? "documentation root URL not specified" "$LINENO" 5 ;; > - *) DOCUMENTATION_ROOT_URL="$withval" > - ;; > + */) DOCUMENTATION_ROOT_URL="$withval" ;; > + *) as_fn_error $? "documentation root URL does not end with /" "$LINENO" 5 ;; > esac > else > DOCUMENTATION_ROOT_URL="https://gcc.gnu.org/onlinedocs/" > @@ -7860,6 +7858,10 @@ else > fi > > > +cat >>confdefs.h <<_ACEOF > +#define DOCUMENTATION_ROOT_URL "$DOCUMENTATION_ROOT_URL" > +_ACEOF > + > > # Allow overriding the default URL for GCC changes > > @@ -7868,8 +7870,8 @@ if test "${with_changes_root_url+set}" = > 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" > - ;; > + */) CHANGES_ROOT_URL="$withval" ;; > + *) as_fn_error $? "changes root URL does not end with /" "$LINENO" 5 ;; > esac > else > CHANGES_ROOT_URL="https://gcc.gnu.org/" > @@ -7877,6 +7879,10 @@ else > fi > > > +cat >>confdefs.h <<_ACEOF > +#define CHANGES_ROOT_URL "$CHANGES_ROOT_URL" > +_ACEOF > + > > # Sanity check enable_languages in case someone does not run the toplevel > # configure # script. > @@ -19009,7 +19015,7 @@ else > lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 > lt_status=$lt_dlunknown > cat > conftest.$ac_ext <<_LT_EOF > -#line 19012 "configure" > +#line 19018 "configure" > #include "confdefs.h" > > #if HAVE_DLFCN_H > @@ -19115,7 +19121,7 @@ else > lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 > lt_status=$lt_dlunknown > cat > conftest.$ac_ext <<_LT_EOF > -#line 19118 "configure" > +#line 19124 "configure" > #include "confdefs.h" > > #if HAVE_DLFCN_H > > > Jakub > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-04-30 9:45 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-29 12:46 [PATCH] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs Jakub Jelinek 2020-04-29 12:57 ` Richard Sandiford 2020-04-29 13:24 ` David Malcolm 2020-04-29 13:52 ` [PATCH v2] " Jakub Jelinek 2020-04-29 14:25 ` David Malcolm 2020-04-29 14:42 ` Jakub Jelinek 2020-04-29 15:34 ` David Malcolm 2020-04-29 15:49 ` Jakub Jelinek 2020-04-29 15:53 ` David Malcolm 2020-04-29 21:42 ` Joseph Myers 2020-04-29 21:45 ` Jakub Jelinek 2020-04-29 15:13 ` [PATCH] " Jonathan Wakely 2020-04-30 6:42 ` Richard Biener 2020-04-30 7:15 ` Jakub Jelinek 2020-04-30 8:44 ` [PATCH] --with-{documentation,changes}-root-url tweaks Jakub Jelinek 2020-04-30 9:45 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).