On Wed, 2018-12-12 at 11:40 -0600, Segher Boessenkool wrote:
> On Tue, Dec 11, 2018 at 10:35:00AM -0500, David Malcolm wrote:
> > On Mon, 2018-12-10 at 22:47 +0000, Segher Boessenkool wrote:
> >
> > [...]
> >
> > > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> > > index 121a91c..652e53c 100644
> > > --- a/gcc/c/c-parser.c
> > > +++ b/gcc/c/c-parser.c
> > > @@ -6360,41 +6360,54 @@ c_parser_for_statement (c_parser *parser,
> > > bool ivdep, unsigned short unroll,
> > > static tree
> > > c_parser_asm_statement (c_parser *parser)
> > > {
> > > - tree quals, str, outputs, inputs, clobbers, labels, ret;
> > > - bool simple, is_volatile, is_inline, is_goto;
> > > + tree str, outputs, inputs, clobbers, labels, ret;
> > > + bool simple;
> > > location_t asm_loc = c_parser_peek_token (parser)->location;
> > > int section, nsections;
> > >
> > > gcc_assert (c_parser_next_token_is_keyword (parser, RID_ASM));
> > > c_parser_consume_token (parser);
> > >
> > > - quals = NULL_TREE;
> > > - is_volatile = false;
> > > - is_inline = false;
> > > - is_goto = false;
> > > + /* Handle the asm-qualifier-list. */
> > > + location_t volatile_loc = UNKNOWN_LOCATION;
> > > + location_t inline_loc = UNKNOWN_LOCATION;
> > > + location_t goto_loc = UNKNOWN_LOCATION;
> > > for (;;)
> > > {
> > > - switch (c_parser_peek_token (parser)->keyword)
> > > + c_token *token = c_parser_peek_token (parser);
> > > + location_t loc = token->location;
> > > + switch (token->keyword)
> > > {
> > > case RID_VOLATILE:
> > > - if (is_volatile)
> > > - break;
> > > - is_volatile = true;
> > > - quals = c_parser_peek_token (parser)->value;
> > > + if (volatile_loc)
> > > + {
> > > + error_at (loc, "duplicate asm qualifier %qE",
> > > token-
> > > > value);
> > >
> > > + inform (volatile_loc, "first seen here");
> > > + }
> >
> > Thanks for the improvements.
> >
> > Is there test coverage for these errors and notes?
>
> Yes, in this same patch. The error, that is; I have no idea how to
> test
> the note, and that belongs in a different test anyhow. Dejagnu
> ignores
> notes normally.
You can use dg-message to test for a note. The ignoring of notes is
done by prune.exp, which strips out any notes that are unmatched after
the dg-message runs.
There are numerous examples in the testsuite, see e.g
gcc.dg/floatn-errs.c
which has:
extern float a; /* { dg-message "previous declaration" } */
extern _Float32 a; /* { dg-error "conflicting" } */
> > A diagnostic nit (new with gcc 9): please add an:
> > auto_diagnostic_group d;
> > to the start of the guarded block, so that the "error" and "note"
> > are
> > known to be related.
>
> How would that work for
>
> asm volatile goto volatile goto ("uh-oh" :::: lab); lab :;
>
> There are two groups, overlapping, but not nested. I suppose
> something
> could be done with new() but that is too ugly for words. Is there a
> procedural interface, too? Something that does not depend on C++
> magic.
If I'm understanding the patch correctly, I'd expect it to output
something like:
error: duplicate asm qualifier 'volatile'
asm volatile goto volatile goto ("uh-oh" :::: lab); lab :;
^~~~~~~~
note: first seen here
asm volatile goto volatile goto ("uh-oh" :::: lab); lab :;
^~~~~~~~
error: duplicate asm qualifier 'goto'
asm volatile goto volatile goto ("uh-oh" :::: lab); lab :;
^~~~
note: first seen here
asm volatile goto volatile goto ("uh-oh" :::: lab); lab :;
^~~~
(with appropriate column numbers)
where each error/note pair are in their own diagnostic group. I don't
think it matters that the locations "overlap" here.
There isn't a procedural interface for diagnostic groups - though there
could be if needed. I think the existing auto_diagnostic_group
ctor/dtor ought to work here. We definitely don't want/need to be
using new, I agree that that would be wrong.
Let me know if you want me to update the patch for you.
Hope this is constructive
Dave
