This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]