From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-2.mimecast.com [205.139.110.61]) by sourceware.org (Postfix) with ESMTP id B5CF538930CB for ; Wed, 29 Apr 2020 13:53:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B5CF538930CB Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-318-_LLgOM5wNVW90_MILVbr0A-1; Wed, 29 Apr 2020 09:53:01 -0400 X-MC-Unique: _LLgOM5wNVW90_MILVbr0A-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D7F88462 for ; Wed, 29 Apr 2020 13:53:00 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-112-104.ams2.redhat.com [10.36.112.104]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 469E466062; Wed, 29 Apr 2020 13:53:00 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id 03TDqweq012914; Wed, 29 Apr 2020 15:52:58 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id 03TDqvDw012913; Wed, 29 Apr 2020 15:52:57 +0200 Date: Wed, 29 Apr 2020 15:52:57 +0200 From: Jakub Jelinek To: David Malcolm Cc: Jonathan Wakely , Jason Merrill , gcc-patches@gcc.gnu.org Subject: [PATCH v2] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs Message-ID: <20200429135257.GH2424@tucnak> Reply-To: Jakub Jelinek References: <20200429124645.GG2424@tucnak> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.11.3 (2019-02-01) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Content-Disposition: inline X-Spam-Status: No, score=-16.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Apr 2020 13:53:13 -0000 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.=20 > Could these be consolidated e.g. >=20 > void maybe_emit_gcc10_param_passing_abi_warning (tree type) > { > /* something here; not sure what */ > } >=20 > 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). > > +=09 const char *url > > +=09=09=3D get_changes_url ("gcc-10/changes.html#empty_base"); >=20 > "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 > > =09 if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) > > =09=09inform (input_location, "parameter passing for argument > > of " > > =09=09=09"type %qT with %<[[no_unique_address]]%> > > members " > > -=09=09=09"changed in GCC 10.1", TYPE_MAIN_VARIANT > > (type)); > > +=09=09=09"changed in %{GCC 10.1%}", > > +=09=09=09TYPE_MAIN_VARIANT (type), url); >=20 > 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.=20 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 thin= k 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 =09* configure.ac (-with-changes-root-url): New configure option, =09defaulting to https://gcc.gnu.org/. =09* Makefile.in (CFLAGS-opts.o): Define CHANGES_ROOT_URL for =09opts.c. =09* pretty-print.c (get_end_url_string): New function. =09(pp_format): Handle %{ and %} for URLs. =09(pp_begin_url): Use pp_string instead of pp_printf. =09(pp_end_url): Use get_end_url_string. =09* opts.h (get_changes_url): Declare. =09* opts.c (get_changes_url): New function. =09* config/rs6000/rs6000-call.c: Include opts.h. =09(rs6000_discover_homogeneous_aggregate): Use %{in GCC 10.1%} instead =09of just in GCC 10.1 in diagnostics and add URL. =09* config/arm/arm.c (aapcs_vfp_is_call_or_return_candidate): Likewise. =09* config/aarch64/aarch64.c (aarch64_vfp_is_call_or_return_candidate): =09Likewise. =09* configure: Regenerated. =09* c-format.c (PP_FORMAT_CHAR_TABLE): Add %{ and %}. --- gcc/configure.ac.jj=092020-04-29 15:28:01.907010894 +0200 +++ gcc/configure.ac=092020-04-29 15:28:27.748628849 +0200 @@ -986,6 +986,20 @@ AC_ARG_WITH(documentation-root-url, ) AC_SUBST(DOCUMENTATION_ROOT_URL) =20 +# Allow overriding the default URL for GCC changes +AC_ARG_WITH(changes-root-url, + AS_HELP_STRING([--with-changes-root-url=3DURL], + [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=3D"$withval" +=09 ;; + esac], + CHANGES_ROOT_URL=3D"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=092020-04-29 15:28:01.875011367 +0200 +++ gcc/Makefile.in=092020-04-29 15:28:27.749628835 +0200 @@ -2187,6 +2187,7 @@ lto-wrapper$(exeext): $(LTO_WRAPPER_OBJS =09mv -f T$@ $@ =20 CFLAGS-opts.o +=3D -DDOCUMENTATION_ROOT_URL=3D\"@DOCUMENTATION_ROOT_URL@\" +CFLAGS-opts.o +=3D -DCHANGES_ROOT_URL=3D\"@CHANGES_ROOT_URL@\" =20 # Files used by all variants of C or by the stand-alone pre-processor. =20 --- gcc/pretty-print.c.jj=092020-04-29 15:28:01.958010139 +0200 +++ gcc/pretty-print.c=092020-04-29 15:36:51.911175267 +0200 @@ -1020,6 +1020,8 @@ pp_indent (pretty_printer *pp) pp_space (pp); } =20 +static const char *get_end_url_string (pretty_printer *); + /* The following format specifiers are recognized as being client independ= ent: %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 tru= e. @@ -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. */ =20 memset (formatters, 0, sizeof formatters); @@ -1133,6 +1137,15 @@ pp_format (pretty_printer *pp, text_info =09 p++; =09 continue; =20 +=09case '}': +=09 { +=09 const char *endurlstr =3D get_end_url_string (pp); +=09 obstack_grow (&buffer->chunk_obstack, endurlstr, +=09=09=09 strlen (endurlstr)); +=09 } +=09 p++; +=09 continue; + =09case 'R': =09 { =09 const char *colorstr =3D colorize_stop (pp_show_color (pp)); @@ -1445,6 +1458,10 @@ pp_format (pretty_printer *pp, text_info =09 } =09 break; =20 +=09case '{': +=09 pp_begin_url (pp, va_arg (*text->args_ptr, const char *)); +=09 break; + =09default: =09 { =09 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" es= cape + 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 (); + } } =20 /* 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 !=3D URL_FORMAT_NONE) + pp_string (pp, get_end_url_string (pp)); } =20 #if CHECKING_P --- gcc/opts.h.jj=092020-04-29 15:28:01.944010346 +0200 +++ gcc/opts.h=092020-04-29 15:28:27.750628820 +0200 @@ -464,6 +464,7 @@ extern void parse_options_from_collect_g =09=09=09=09=09=09 int *); =20 extern void prepend_xassembler_to_collect_as_options (const char *, obstac= k *); +extern char *get_changes_url (const char *); =20 /* Set OPTION in OPTS to VALUE if the option is not set in OPTS_SET. */ =20 --- gcc/opts.c.jj=092020-04-29 15:28:01.919010716 +0200 +++ gcc/opts.c=092020-04-29 15:30:43.172626729 +0200 @@ -3190,6 +3190,16 @@ get_option_url (diagnostic_context *, in return NULL; } =20 +/* 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 =20 namespace selftest { --- gcc/config/arm/arm.c.jj=092020-04-29 15:28:01.902010968 +0200 +++ gcc/config/arm/arm.c=092020-04-29 15:36:06.059853139 +0200 @@ -6416,6 +6416,7 @@ aapcs_vfp_is_call_or_return_candidate (e =09 && ((alt =3D aapcs_vfp_sub_candidate (type, &new_mode, NULL)) =09=09 !=3D ag_count)) =09 { +=09 char *url =3D get_changes_url ("gcc-10/changes.html#empty_base"); =09 gcc_assert (alt =3D=3D -1); =09 last_reported_type_uid =3D uid; =09 /* Use TYPE_MAIN_VARIANT to strip any redundant const @@ -6423,11 +6424,14 @@ aapcs_vfp_is_call_or_return_candidate (e =09 if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) =09=09inform (input_location, "parameter passing for argument of " =09=09=09"type %qT with %<[[no_unique_address]]%> members " -=09=09=09"changed in GCC 10.1", TYPE_MAIN_VARIANT (type)); +=09=09=09"changed %{in GCC 10.1%}", +=09=09=09TYPE_MAIN_VARIANT (type), url); =09 else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) =09=09inform (input_location, "parameter passing for argument of " =09=09=09"type %qT when C++17 is enabled changed to match " -=09=09=09"C++14 in GCC 10.1", TYPE_MAIN_VARIANT (type)); +=09=09=09"C++14 %{in GCC 10.1%}", +=09=09=09TYPE_MAIN_VARIANT (type), url); +=09 free (url); =09 } =09 *count =3D ag_count; =09} --- gcc/config/aarch64/aarch64.c.jj=092020-04-29 15:28:01.896011056 +0200 +++ gcc/config/aarch64/aarch64.c=092020-04-29 15:35:44.835166928 +0200 @@ -16883,6 +16883,7 @@ aarch64_vfp_is_call_or_return_candidate =09 && ((alt =3D aapcs_vfp_sub_candidate (type, &new_mode, NULL)) =09=09 !=3D ag_count)) =09 { +=09 char *url =3D get_changes_url ("gcc-10/changes.html#empty_base"); =09 gcc_assert (alt =3D=3D -1); =09 last_reported_type_uid =3D uid; =09 /* Use TYPE_MAIN_VARIANT to strip any redundant const @@ -16890,11 +16891,14 @@ aarch64_vfp_is_call_or_return_candidate =09 if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS) =09=09inform (input_location, "parameter passing for argument of " =09=09=09"type %qT with %<[[no_unique_address]]%> members " -=09=09=09"changed in GCC 10.1", TYPE_MAIN_VARIANT (type)); +=09=09=09"changed %{in GCC 10.1%}", +=09=09=09TYPE_MAIN_VARIANT (type), url); =09 else if (warn_psabi_flags & WARN_PSABI_EMPTY_CXX17_BASE) =09=09inform (input_location, "parameter passing for argument of " =09=09=09"type %qT when C++17 is enabled changed to match " -=09=09=09"C++14 in GCC 10.1", TYPE_MAIN_VARIANT (type)); +=09=09=09"C++14 %{in GCC 10.1%}", +=09=09=09TYPE_MAIN_VARIANT (type), url); +=09 free (url); =09 } =20 =09 if (is_ha !=3D NULL) *is_ha =3D true; --- gcc/config/rs6000/rs6000-call.c.jj=092020-04-29 15:28:01.903010953 +020= 0 +++ gcc/config/rs6000/rs6000-call.c=092020-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" =20 #include "rs6000-internal.h" =20 @@ -5747,17 +5748,20 @@ rs6000_discover_homogeneous_aggregate (m =09=09 unsigned uid =3D TYPE_UID (TYPE_MAIN_VARIANT (type)); =09=09 if (uid !=3D last_reported_type_uid) =09=09 { +=09=09 char *url +=09=09=09=3D get_changes_url ("gcc-10/changes.html#empty_base"); =09=09 if (empty_base_seen & 1) =09=09=09inform (input_location, =09=09=09=09"parameter passing for argument of type %qT " =09=09=09=09"when C++17 is enabled changed to match C++14 " -=09=09=09=09"in GCC 10.1", type); +=09=09=09=09"%{in GCC 10.1%}", type, url); =09=09 else =09=09=09inform (input_location, =09=09=09=09"parameter passing for argument of type %qT " =09=09=09=09"with %<[[no_unique_address]]%> members " -=09=09=09=09"changed in GCC 10.1", type); +=09=09=09=09"changed %{in GCC 10.1%}", type, url); =09=09 last_reported_type_uid =3D uid; +=09=09 free (url); =09=09 } =09=09} =09 return true; --- gcc/c-family/c-format.c.jj=092020-04-29 15:28:01.890011145 +0200 +++ gcc/c-family/c-format.c=092020-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, BADL= EN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN }, "", "cR", NU= LL }, \ + { "}", 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, BADL= EN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN, BADLEN }, "", "", &gc= c_diag_char_table[0] } --- gcc/configure.jj=092020-04-29 15:28:01.906010908 +0200 +++ gcc/configure=092020-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=3DURL Direct users to URL to report a bug --with-documentation-root-url=3DURL Root for documentation URLs + --with-changes-root-url=3DURL + Root for GCC changes URLs --with-multilib-list select multilibs (AArch64, SH and x86-64 only) --with-zstd=3DPATH specify prefix directory for installed zstd li= brary. Equivalent to --with-zstd-include=3DPATH/include= plus @@ -7857,6 +7861,23 @@ fi =20 =20 =20 +# Allow overriding the default URL for GCC changes + +# Check whether --with-changes-root-url was given. +if test "${with_changes_root_url+set}" =3D set; then : + withval=3D$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=3D"$withval" +=09 ;; + esac +else + CHANGES_ROOT_URL=3D"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=3D0; lt_dlno_uscore=3D1; lt_dlneed_uscore=3D2 lt_status=3D$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 18991 "configure" +#line 19012 "configure" #include "confdefs.h" =20 #if HAVE_DLFCN_H @@ -19094,7 +19115,7 @@ else lt_dlunknown=3D0; lt_dlno_uscore=3D1; lt_dlneed_uscore=3D2 lt_status=3D$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 19097 "configure" +#line 19118 "configure" #include "confdefs.h" =20 #if HAVE_DLFCN_H =09Jakub