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

[David Malcolm <dmalcolm at redhat dot com>]

On Tue, 2018-12-11 at 10:35 -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?
> 
> 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.
> 
> See:	
> https://gcc.gnu.org/onlinedocs/gccint/Guidelines-for-Diagnostics.html
> #Group-logically-related-diagnostics

For bonus points, these could offer fix-it hints, so that an IDE can
offer to delete the duplicate qualifier token.  The above code could
be:

  if (volatile_loc)
    complain_about_duplicate_asm_qualifier (token->value, loc,
                                            volatile_loc);
  else
    volatile_loc = loc;


void
complain_about_duplicate_asm_qualifier (tree value,
                                        location_t duplicate_loc,
                                        location_t first_loc)
{
   auto_diagnostic_group d;
   gcc_rich_location richloc (duplicate_loc);
   richloc.add_fixit_remove ();
   error_at (&richloc, "duplicate asm qualifier %qE", value);
   inform (first_loc, "first seen here");
}

or somesuch, where rich_location::add_fixit_remove adds a fix-it hint
suggesting the removal of all of "loc", the duplicate token; given that
it's 5 lines at this point, a subroutine seems justified, to eliminate
duplication at the 6 sites it's done.

Caveat: haven't tried to compile the above.

Dave


> > +	  else
> > +	    volatile_loc = loc;
> >  	  c_parser_consume_token (parser);
> >  	  continue;
> >  
> >  	case RID_INLINE:
> > -	  if (is_inline)
> > -	    break;
> > -	  is_inline = true;
> > +	  if (inline_loc)
> > +	    {
> > +	      error_at (loc, "duplicate asm qualifier %qE", token-
> > > value);
> > 
> > +	      inform (inline_loc, "first seen here");
> 
> Likewise.
> 
> > +	    }
> > +	  else
> > +	    inline_loc = loc;
> >  	  c_parser_consume_token (parser);
> >  	  continue;
> >  
> >  	case RID_GOTO:
> > -	  if (is_goto)
> > -	    break;
> > -	  is_goto = true;
> > +	  if (goto_loc)
> > +	    {
> > +	      error_at (loc, "duplicate asm qualifier %qE", token-
> > > value);
> > 
> > +	      inform (goto_loc, "first seen here");
> > +	    }
> 
> Likewise.
> 
> > +	  else
> > +	    goto_loc = loc;
> >  	  c_parser_consume_token (parser);
> >  	  continue;
> 
> [...]
>  
> > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> > index 1cc34ba..06a6bb0 100644
> > --- a/gcc/cp/parser.c
> > +++ b/gcc/cp/parser.c
> > @@ -19649,29 +19646,50 @@ cp_parser_asm_definition (cp_parser*
> > parser)
> >      }
> >  
> >    /* Handle the asm-qualifier-list.  */
> > +  location_t volatile_loc = UNKNOWN_LOCATION;
> > +  location_t inline_loc = UNKNOWN_LOCATION;
> > +  location_t goto_loc = UNKNOWN_LOCATION;
> >    if (cp_parser_allow_gnu_extensions_p (parser))
> >      for (;;)
> >        {
> > +	cp_token *token = cp_lexer_peek_token (parser->lexer);
> > +	location_t loc = token->location;
> >  	switch (cp_lexer_peek_token (parser->lexer)->keyword)
> >  	  {
> >  	  case RID_VOLATILE:
> > -	    if (volatile_p)
> > -	      break;
> > -	    volatile_p = true;
> > +	    if (volatile_loc)
> > +	      {
> > +		error_at (loc, "duplicate asm qualifier %qT",
> > token-
> > > u.value);
> > 
> > +		inform (volatile_loc, "first seen here");
> 
> Likewise.
> 
> > +	      }
> > +	    else
> > +	      volatile_loc = loc;
> >  	    cp_lexer_consume_token (parser->lexer);
> >  	    continue;
> >  
> >  	  case RID_INLINE:
> > -	    if (inline_p || !parser->in_function_body)
> > +	    if (!parser->in_function_body)
> >  	      break;
> > -	    inline_p = true;
> > +	    if (inline_loc)
> > +	      {
> > +		error_at (loc, "duplicate asm qualifier %qT",
> > token-
> > > u.value);
> > 
> > +		inform (inline_loc, "first seen here");
> 
> Likewise.
> 
> > +	      }
> > +	    else
> > +	      inline_loc = loc;
> >  	    cp_lexer_consume_token (parser->lexer);
> >  	    continue;
> >  
> >  	  case RID_GOTO:
> > -	    if (goto_p || !parser->in_function_body)
> > +	    if (!parser->in_function_body)
> >  	      break;
> > -	    goto_p = true;
> > +	    if (goto_loc)
> > +	      {
> > +		error_at (loc, "duplicate asm qualifier %qT",
> > token-
> > > u.value);
> > 
> > +		inform (goto_loc, "first seen here");
> 
> Likewise.
> 
> > +	      }
> > +	    else
> > +	      goto_loc = loc;
> >  	    cp_lexer_consume_token (parser->lexer);
> >  	    continue;
> >  
> 
> [...]
> 
> 
> Dave