[v2] diagnostics: Add %{...%} pretty-format support for URLs and use it in -Wpsabi diagnostcs

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 %}.



	Jakub

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

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

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

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

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

This is missing documentation for the new configure option in 
install.texi.

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