[1/2] asm qualifiers (PR55681)

PR55681 observes that currently only one qualifier is allowed for
inline asm, so that e.g. "volatile asm" is allowed, "const asm" is also
okay (with a warning), but "const volatile asm" gives an error.  Also
"const const asm" is an error (while "const const int" is okay for C),
"goto" has to be last, and "_Atomic" isn't handled at all.

This patch fixes all these.  It allows any order of qualifiers (and
goto), allows them all for C, allows duplications for C.  For C++ it
still allows only a single volatile and single goto, but in any order.


2018-10-30  Segher Boessenkool  <segher@kernel.crashing.org>

gcc/c/
	PR inline-asm/55681
	* c-parser.c (c_parser_for_statement): Update grammar.  Allow any
	combination of type-qualifiers and goto in any order, with repetitions
	allowed.

gcc/cp/
	PR inline-asm/55681
	* parser.c (cp_parser_using_directive): Update grammar.  Allow any
	combination of volatile and goto in any order, without repetitions.

---
 gcc/c/c-parser.c | 66 +++++++++++++++++++++++++++---------------------
 gcc/cp/parser.c  | 77 +++++++++++++++++++++++++++++++++++++-------------------
 2 files changed, 89 insertions(+), 54 deletions(-)

+cc: C and C++ maintainers.  Sorry I forgot before :-/

On Tue, Oct 30, 2018 at 05:30:33PM +0000, Segher Boessenkool wrote:
> PR55681 observes that currently only one qualifier is allowed for
> inline asm, so that e.g. "volatile asm" is allowed, "const asm" is also
> okay (with a warning), but "const volatile asm" gives an error.  Also
> "const const asm" is an error (while "const const int" is okay for C),
> "goto" has to be last, and "_Atomic" isn't handled at all.
> 
> This patch fixes all these.  It allows any order of qualifiers (and
> goto), allows them all for C, allows duplications for C.  For C++ it
> still allows only a single volatile and single goto, but in any order.
> 
> 
> 2018-10-30  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> gcc/c/
> 	PR inline-asm/55681
> 	* c-parser.c (c_parser_for_statement): Update grammar.  Allow any
> 	combination of type-qualifiers and goto in any order, with repetitions
> 	allowed.
> 
> gcc/cp/
> 	PR inline-asm/55681
> 	* parser.c (cp_parser_using_directive): Update grammar.  Allow any
> 	combination of volatile and goto in any order, without repetitions.
> 
> ---
>  gcc/c/c-parser.c | 66 +++++++++++++++++++++++++++---------------------
>  gcc/cp/parser.c  | 77 +++++++++++++++++++++++++++++++++++++-------------------
>  2 files changed, 89 insertions(+), 54 deletions(-)
> 
> diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> index ee66ce8..ce9921e 100644
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -6283,23 +6283,31 @@ c_parser_for_statement (c_parser *parser, bool ivdep, unsigned short unroll,
>  }
>  
>  /* Parse an asm statement, a GNU extension.  This is a full-blown asm
> -   statement with inputs, outputs, clobbers, and volatile tag
> +   statement with inputs, outputs, clobbers, and volatile and goto tag
>     allowed.
>  
> +   asm-qualifier:
> +     type-qualifier
> +     goto
> +
> +   asm-qualifier-list:
> +     asm-qualifier-list asm-qualifier
> +     asm-qualifier
> +
>     asm-statement:
> -     asm type-qualifier[opt] ( asm-argument ) ;
> -     asm type-qualifier[opt] goto ( asm-goto-argument ) ;
> +     asm asm-qualifier-list[opt] ( asm-argument ) ;
>  
>     asm-argument:
>       asm-string-literal
>       asm-string-literal : asm-operands[opt]
>       asm-string-literal : asm-operands[opt] : asm-operands[opt]
> -     asm-string-literal : asm-operands[opt] : asm-operands[opt] : asm-clobbers[opt]
> -
> -   asm-goto-argument:
> +     asm-string-literal : asm-operands[opt] : asm-operands[opt] \
> +       : asm-clobbers[opt]
>       asm-string-literal : : asm-operands[opt] : asm-clobbers[opt] \
>         : asm-goto-operands
>  
> +   The form with asm-goto-operands is valid if and only if the
> +   asm-qualifier-list contains goto, and is the only allowed form in that case.
>     Qualifiers other than volatile are accepted in the syntax but
>     warned for.  */
>  
> @@ -6313,30 +6321,32 @@ c_parser_asm_statement (c_parser *parser)
>  
>    gcc_assert (c_parser_next_token_is_keyword (parser, RID_ASM));
>    c_parser_consume_token (parser);
> -  if (c_parser_next_token_is_keyword (parser, RID_VOLATILE))
> -    {
> -      quals = c_parser_peek_token (parser)->value;
> -      c_parser_consume_token (parser);
> -    }
> -  else if (c_parser_next_token_is_keyword (parser, RID_CONST)
> -	   || c_parser_next_token_is_keyword (parser, RID_RESTRICT))
> -    {
> -      warning_at (c_parser_peek_token (parser)->location,
> -		  0,
> -		  "%E qualifier ignored on asm",
> -		  c_parser_peek_token (parser)->value);
> -      quals = NULL_TREE;
> -      c_parser_consume_token (parser);
> -    }
> -  else
> -    quals = NULL_TREE;
>  
> +  quals = NULL_TREE;
>    is_goto = false;
> -  if (c_parser_next_token_is_keyword (parser, RID_GOTO))
> -    {
> -      c_parser_consume_token (parser);
> -      is_goto = true;
> -    }
> +  for (bool done = false; !done; )
> +    switch (c_parser_peek_token (parser)->keyword)
> +      {
> +      case RID_VOLATILE:
> +	quals = c_parser_peek_token (parser)->value;
> +	c_parser_consume_token (parser);
> +	break;
> +      case RID_CONST:
> +      case RID_RESTRICT:
> +      case RID_ATOMIC:
> +	warning_at (c_parser_peek_token (parser)->location,
> +		    0,
> +		    "%E qualifier ignored on asm",
> +		    c_parser_peek_token (parser)->value);
> +	c_parser_consume_token (parser);
> +	break;
> +      case RID_GOTO:
> +	is_goto = true;
> +	c_parser_consume_token (parser);
> +	break;
> +      default:
> +	done = true;
> +      }
>  
>    /* ??? Follow the C++ parser rather than using the
>       lex_untranslated_string kludge.  */
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index ebe326e..d44fd4d 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -19196,22 +19196,34 @@ cp_parser_using_directive (cp_parser* parser)
>  
>  /* Parse an asm-definition.
>  
> +  asm-qualifier:
> +    volatile
> +    goto
> +
> +  asm-qualifier-list:
> +    asm-qualifier
> +    asm-qualifier-list asm-qualifier
> +
>     asm-definition:
>       asm ( string-literal ) ;
>  
>     GNU Extension:
>  
>     asm-definition:
> -     asm volatile [opt] ( string-literal ) ;
> -     asm volatile [opt] ( string-literal : asm-operand-list [opt] ) ;
> -     asm volatile [opt] ( string-literal : asm-operand-list [opt]
> -			  : asm-operand-list [opt] ) ;
> -     asm volatile [opt] ( string-literal : asm-operand-list [opt]
> -			  : asm-operand-list [opt]
> +     asm asm-qualifier-list [opt] ( string-literal ) ;
> +     asm asm-qualifier-list [opt] ( string-literal : asm-operand-list [opt] ) ;
> +     asm asm-qualifier-list [opt] ( string-literal : asm-operand-list [opt]
> +				    : asm-operand-list [opt] ) ;
> +     asm asm-qualifier-list [opt] ( string-literal : asm-operand-list [opt]
> +				    : asm-operand-list [opt]
>  			  : asm-clobber-list [opt] ) ;
> -     asm volatile [opt] goto ( string-literal : : asm-operand-list [opt]
> -			       : asm-clobber-list [opt]
> -			       : asm-goto-list ) ;  */
> +     asm asm-qualifier-list [opt] ( string-literal : : asm-operand-list [opt]
> +				    : asm-clobber-list [opt]
> +				    : asm-goto-list ) ;
> +
> +  The form with asm-goto-list is valid if and only if the asm-qualifier-list
> +  contains goto, and is the only allowed form in that case.  No duplicates are
> +  allowed in an asm-qualifier-list.  */
>  
>  static void
>  cp_parser_asm_definition (cp_parser* parser)
> @@ -19240,23 +19252,36 @@ cp_parser_asm_definition (cp_parser* parser)
>      }
>  
>    /* See if the next token is `volatile'.  */
> -  if (cp_parser_allow_gnu_extensions_p (parser)
> -      && cp_lexer_next_token_is_keyword (parser->lexer, RID_VOLATILE))
> -    {
> -      /* Remember that we saw the `volatile' keyword.  */
> -      volatile_p = true;
> -      /* Consume the token.  */
> -      cp_lexer_consume_token (parser->lexer);
> -    }
> -  if (cp_parser_allow_gnu_extensions_p (parser)
> -      && parser->in_function_body
> -      && cp_lexer_next_token_is_keyword (parser->lexer, RID_GOTO))
> -    {
> -      /* Remember that we saw the `goto' keyword.  */
> -      goto_p = true;
> -      /* Consume the token.  */
> -      cp_lexer_consume_token (parser->lexer);
> -    }
> +  if (cp_parser_allow_gnu_extensions_p (parser))
> +    for (bool done = false; !done ; )
> +      switch (cp_lexer_peek_token (parser->lexer)->keyword)
> +	{
> +	case RID_VOLATILE:
> +	  if (!volatile_p)
> +	    {
> +	      /* Remember that we saw the `volatile' keyword.  */
> +	      volatile_p = true;
> +	      /* Consume the token.  */
> +	      cp_lexer_consume_token (parser->lexer);
> +	    }
> +	  else
> +	    done = true;
> +	  break;
> +	case RID_GOTO:
> +	  if (!goto_p && parser->in_function_body)
> +	    {
> +	      /* Remember that we saw the `goto' keyword.  */
> +	      goto_p = true;
> +	      /* Consume the token.  */
> +	      cp_lexer_consume_token (parser->lexer);
> +	    }
> +	  else
> +	    done = true;
> +	  break;
> +	default:
> +	  done = true;
> +	}
> +
>    /* Look for the opening `('.  */
>    if (!cp_parser_require (parser, CPP_OPEN_PAREN, RT_OPEN_PAREN))
>      return;
> -- 
> 1.8.3.1

I'd expect testcases to be added for the new syntax variants (duplicate 
qualifiers / goto and new orderings thereof).

There's a description of the syntax in extend.texi:

@example
asm @r{[}volatile@r{]} ( @var{AssemblerTemplate} 
                 : @var{OutputOperands} 
                 @r{[} : @var{InputOperands}
                 @r{[} : @var{Clobbers} @r{]} @r{]})

asm @r{[}volatile@r{]} goto ( @var{AssemblerTemplate} 
                      : 
                      : @var{InputOperands}
                      : @var{Clobbers}
                      : @var{GotoLabels})
@end example

I'd expect this to be updated by this patch, and again by the "asm inline" 
one.

What's the basis for allowing duplicates for C but not for C++?

On Thu, Nov 29, 2018 at 09:13:13PM +0000, Joseph Myers wrote:
> I'd expect testcases to be added for the new syntax variants (duplicate 
> qualifiers / goto and new orderings thereof).

Okay.

> There's a description of the syntax in extend.texi:
> 
> @example
> asm @r{[}volatile@r{]} ( @var{AssemblerTemplate} 
>                  : @var{OutputOperands} 
>                  @r{[} : @var{InputOperands}
>                  @r{[} : @var{Clobbers} @r{]} @r{]})
> 
> asm @r{[}volatile@r{]} goto ( @var{AssemblerTemplate} 
>                       : 
>                       : @var{InputOperands}
>                       : @var{Clobbers}
>                       : @var{GotoLabels})
> @end example
> 
> I'd expect this to be updated by this patch, and again by the "asm inline" 
> one.

That stuff needs to be rewritten :-(

But I'll see what I can do.

> What's the basis for allowing duplicates for C but not for C++?

It is the status quo.  It would make sense to allow duplicates for C++ as
well, sure.  If that is preferred I can make a patch for it?


Segher

On Thu, 29 Nov 2018, Segher Boessenkool wrote:

> > What's the basis for allowing duplicates for C but not for C++?
> 
> It is the status quo.  It would make sense to allow duplicates for C++ as
> well, sure.  If that is preferred I can make a patch for it?

Duplicate qualifiers are allowed *in declarations* for C (as per C99).  
They aren't allowed in asm.  I'd think the most obvious thing would be not 
to allow duplicate qualifiers in asm at all (but still allow any ordering 
of volatile, goto and inline).  Essentially, the use in asm is just 
reusing a keyword in a different context, so I don't think duplicates 
being allowed in declarations is relevant to allowing them in asm (any 
more than it indicates that __attribute__ ((const const const)) should be 
allowed just because const is a valid attribute).

On Thu, Nov 29, 2018 at 11:14:45PM +0000, Joseph Myers wrote:
> On Thu, 29 Nov 2018, Segher Boessenkool wrote:
> 
> > > What's the basis for allowing duplicates for C but not for C++?
> > 
> > It is the status quo.  It would make sense to allow duplicates for C++ as
> > well, sure.  If that is preferred I can make a patch for it?
> 
> Duplicate qualifiers are allowed *in declarations* for C (as per C99).  

And I used type-qualifier-list[opt] there, to start with.

> They aren't allowed in asm.  I'd think the most obvious thing would be not 
> to allow duplicate qualifiers in asm at all (but still allow any ordering 
> of volatile, goto and inline).  Essentially, the use in asm is just 
> reusing a keyword in a different context, so I don't think duplicates 
> being allowed in declarations is relevant to allowing them in asm (any 
> more than it indicates that __attribute__ ((const const const)) should be 
> allowed just because const is a valid attribute).

So "asm const restrict" is allowed, but "asm const const restrict" isn't.
Hrm.  That means we'll have to keep track of the ignored qualifiers.
(There aren't any ignored asm qualifiers in C++ so no such issue there).

What do you want done with const and restrict (and _Atomic, which is
allowed by the current grammar)?


Segher

On Thu, 29 Nov 2018, Segher Boessenkool wrote:

> So "asm const restrict" is allowed, but "asm const const restrict" isn't.

No, asm const restrict isn't allowed.  volatile is allowed; const and 
restrict are allowed with warnings because that replicates what the old 
bison parser allowed; but at most one qualifier is allowed at present.

> What do you want done with const and restrict (and _Atomic, which is
> allowed by the current grammar)?

Don't allow _Atomic, since it's not allowed at present.  Either allow at 
most one qualifier (being one of volatile / const / restrict) or remove 
support for const and restrict there and just allow (at most one) 
volatile.

On Fri, Nov 30, 2018 at 12:11:30AM +0000, Joseph Myers wrote:
> On Thu, 29 Nov 2018, Segher Boessenkool wrote:
> 
> > So "asm const restrict" is allowed, but "asm const const restrict" isn't.
> 
> No, asm const restrict isn't allowed.  volatile is allowed; const and 
> restrict are allowed with warnings because that replicates what the old 
> bison parser allowed; but at most one qualifier is allowed at present.

I mean the wanted behaviour, not the current behaviour.

> > What do you want done with const and restrict (and _Atomic, which is
> > allowed by the current grammar)?
> 
> Don't allow _Atomic, since it's not allowed at present.  Either allow at 
> most one qualifier (being one of volatile / const / restrict) or remove 
> support for const and restrict there and just allow (at most one) 
> volatile.

I'll go for the latter.  That also harmonises C and C++ here.

Thanks!


Segher