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 <ja...@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
