On Thu, 2021-07-01 at 14:14 -0600, Martin Sebor wrote:
> On 6/30/21 5:35 PM, David Malcolm wrote:
> > On Wed, 2021-06-30 at 13:45 -0600, Martin Sebor wrote:
> > > On 6/30/21 9:39 AM, Martin Sebor wrote:
> > > > Ping. Attached is the same patch rebased on top the latest
> > > > trunk.
> > >
> > > Please see the attached patch instead. The previous one had typo
> > > in it.
> > >
> > > >
> > > > As discussed in the review of Aldy's recent changes to the
> > > > backwards
> > > > threader, he has run into the same bug the patch fixes.
> > > > Getting this
> > > > patch set reviewed and approved would be helpful in keeping him
> > > > from
> > > > having to work around the bug.
> > > >
> > > > https://gcc.gnu.org/pipermail/gcc/2021-June/236608.html
> > > >
> > > > On 6/10/21 5:27 PM, Martin Sebor wrote:
> > > > > This diff removes the uses of %G and %K from all warning_at()
> > > > > calls
> > > > > throughout GCC front end and middle end. The inlining
> > > > > context is
> > > > > included in diagnostic output whenever it's present.
> > > >
> > >
> >
> > Thanks for writing the patch.
> >
> > I went through the full patch, though my eyes may have glazed over
> > in
> > places at all of the %G and %K removals. I *think* you got them
> > mostly
> > correct, apart from the following possible issues and nits...
> >
> > > diff --git a/gcc/expr.c b/gcc/expr.c
> > > index 025033c9ecf..b9fe1cf91d7 100644
> > > --- a/gcc/expr.c
> > > +++ b/gcc/expr.c
> >
> > [...]
> >
> > > @@ -11425,10 +11425,10 @@ expand_expr_real_1 (tree exp, rtx
> > > target, machine_mode tmode,
> > > DECL_ATTRIBUTES
> > > (fndecl))) != NULL)
> > > {
> > > const char *ident = lang_hooks.decl_printable_name
> > > (fndecl, 1);
> > > - warning_at (tree_nonartificial_location (exp),
> > > + warning_at (EXPR_LOCATION (exp),
> >
> > Are we preserving the existing behavior for
> > __attribute__((__artificial__)) here?
> > Is this behavior handled somewhere else in the patch kit?
>
> Yes. The warning infrastructure (set_inlining_locations) uses
> the location of the site into which the statement has been inlined
> regardless of whether the inlined function is artificial.
Do we have test coverage for this, though?
[...]
>
> > > @@ -90,8 +90,8 @@ NOIPA void warn_g2 (struct A *p)
> > > g2 (p);
> > > }
> > >
> > > -// { dg-message "inlined from 'g2'" "" { target *-*-* } 0 }
> > > -// { dg-message "inlined from 'warn_g2'" "" { target *-*-* } 0 }
> > > +// { dg-message "inlined from 'g2'" "note on line 93" { target
> > > *-*-* } 0 }
> > > +// { dg-message "inlined from 'warn_g2'" "note on line 94" {
> > > target *-*-* } 0 }
> >
> > You've added descriptions to disambiguate all of the various
> > directives
> > on line 0, which is good, but I don't like the use of line numbers
> > in
> > the descriptions, since it will get very confusing if the numbering
> > changes.
> >
> > Would it work to use the message text as the description e.g.
> >
> > // { dg-message "inlined from 'warn_g2'" "inlined from
> > 'warn_g2'" { target *-*-* } 0 }
> >
> > or somesuch?
>
> It would certainly work, they're just informational labels printed
> by DejaGnu when the assertions fail. I added them to help me see
> what they went with while working with the test. I'm not concerned
> about the line numbers changing. If they do and someone notices,
> they can update them, the same way they might want to if they
> rename the functions they're inlined into.
[...]
> >
> > After reading through this and trying to grok it, I see that this
> > file
> > logically can be split into several parts: the "warn*" functions,
> > then
> > the "nowarn*_ignore0" functions, then the "nowarn*_ignore_1"
> > functions
> > etc.
> >
> > Please add some kind of separator comment between each of these
> > parts
> > to make it easy for the reader to see this structure.
>
> Sure, I've added a comment.
>
Thanks.
> Attached is the revised patch for reference. Since it just removes
> the uses of the %K and %G directives made redundant by the first
> patch in the series I'll go ahead and commit it as obvious in a day
> or so after patch 1 unless someone has further questions or requests
> for changes.
Please can you look into the "__artificial__" test coverage, and
address the line number thing above.
Dave
