* [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] 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 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 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).