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] }
