[C++ PATCH] Toplevel asm volatile (PR c++/89585)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[C++ PATCH] Toplevel asm volatile (PR c++/89585)

Jakub Jelinek
Hi!

The following patch tries to improve diagnostics of toplevel asm qualifiers
in C++ by actually parsing them and complaining if they appear at toplevel,
instead of just emitting a parse error that ( is expected, e.g. some
versions of Qt do use toplevel asm volatile and apparently the Qt code is
copied into lots of various projects.

In addition to that, it mentions in the documentation that qualifiers are
not allowed at toplevel asm statements; apparently our documentation at
least from r220506 for GCC 5 says that at toplevel Basic Asm needs to be
used and for Basic Asm lists volatile qualifier as optional and its behavior
(that it is ignored for Basic Asm).  Makes me wonder if we don't want to
keep accepting/ignoring volatile at toplevel for both C and C++ instead of
rejecting it (and rejecting just the other qualifiers).  Thoughts on this?

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Attached is an untested backport of this patch to 8.4, which does allow
asm volatile at toplevel, so that we don't break in 8.4 what has been
accepted in 8.2.  Ok if it passes bootstrap/regtest there?

2019-03-07  Jakub Jelinek  <[hidden email]>

        PR c++/89585
        * doc/extend.texi (Basic Asm): Document qualifiers are not allowed
        at toplevel.

        * parser.c (cp_parser_asm_definition): Parse asm qualifiers even
        at toplevel, but diagnose them.

        * g++.dg/asm-qual-3.C: Adjust expected diagnostics.

--- gcc/doc/extend.texi.jj 2019-02-26 21:35:26.782115737 +0100
+++ gcc/doc/extend.texi 2019-03-06 10:23:14.894032621 +0100
@@ -9064,6 +9064,8 @@ outside of C functions, you must use bas
 You can use this technique to emit assembler directives,
 define assembly language macros that can be invoked elsewhere in the file,
 or write entire functions in assembly language.
+Basic @code{asm} statements outside of functions may not use any
+qualifiers.
 
 @item
 Functions declared
--- gcc/cp/parser.c.jj 2019-03-05 09:39:18.850146559 +0100
+++ gcc/cp/parser.c 2019-03-06 19:41:27.861895296 +0100
@@ -19766,8 +19766,9 @@ cp_parser_asm_definition (cp_parser* par
   location_t volatile_loc = UNKNOWN_LOCATION;
   location_t inline_loc = UNKNOWN_LOCATION;
   location_t goto_loc = UNKNOWN_LOCATION;
+  location_t first_loc = UNKNOWN_LOCATION;
 
-  if (cp_parser_allow_gnu_extensions_p (parser) && parser->in_function_body)
+  if (cp_parser_allow_gnu_extensions_p (parser))
     for (;;)
       {
  cp_token *token = cp_lexer_peek_token (parser->lexer);
@@ -19782,6 +19783,8 @@ cp_parser_asm_definition (cp_parser* par
       }
     else
       volatile_loc = loc;
+    if (!first_loc)
+      first_loc = loc;
     cp_lexer_consume_token (parser->lexer);
     continue;
 
@@ -19793,6 +19796,8 @@ cp_parser_asm_definition (cp_parser* par
       }
     else
       inline_loc = loc;
+    if (!first_loc)
+      first_loc = loc;
     cp_lexer_consume_token (parser->lexer);
     continue;
 
@@ -19804,6 +19809,8 @@ cp_parser_asm_definition (cp_parser* par
       }
     else
       goto_loc = loc;
+    if (!first_loc)
+      first_loc = loc;
     cp_lexer_consume_token (parser->lexer);
     continue;
 
@@ -19823,6 +19830,12 @@ cp_parser_asm_definition (cp_parser* par
   bool inline_p = (inline_loc != UNKNOWN_LOCATION);
   bool goto_p = (goto_loc != UNKNOWN_LOCATION);
 
+  if (!parser->in_function_body && (volatile_p || inline_p || goto_p))
+    {
+      error_at (first_loc, "asm qualifier outside of function body");
+      volatile_p = inline_p = goto_p = false;
+    }
+
   /* Look for the opening `('.  */
   if (!cp_parser_require (parser, CPP_OPEN_PAREN, RT_OPEN_PAREN))
     return;
--- gcc/testsuite/g++.dg/asm-qual-3.C.jj 2018-12-20 08:50:27.234484465 +0100
+++ gcc/testsuite/g++.dg/asm-qual-3.C 2019-03-06 19:40:20.932993605 +0100
@@ -2,11 +2,11 @@
 // { dg-do compile }
 // { dg-options "-std=gnu++98" }
 
-asm const ("");    // { dg-error {expected '\(' before 'const'} }
-asm volatile (""); // { dg-error {expected '\(' before 'volatile'} }
+asm const ("");    // { dg-error {'const' is not an asm qualifier} }
+asm volatile (""); // { dg-error {asm qualifier outside of function body} }
 asm restrict (""); // { dg-error {expected '\(' before 'restrict'} }
-asm inline ("");   // { dg-error {expected '\(' before 'inline'} }
-asm goto ("");     // { dg-error {expected '\(' before 'goto'} }
+asm inline ("");   // { dg-error {asm qualifier outside of function body} }
+asm goto ("");     // { dg-error {asm qualifier outside of function body} }
 
 // There are many other things wrong with this code, so:
 // { dg-excess-errors "" }

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: [C++ PATCH] Toplevel asm volatile (PR c++/89585)

Jason Merrill
On Wed, Mar 6, 2019 at 6:39 PM Jakub Jelinek <[hidden email]> wrote:

>
> The following patch tries to improve diagnostics of toplevel asm qualifiers
> in C++ by actually parsing them and complaining if they appear at toplevel,
> instead of just emitting a parse error that ( is expected, e.g. some
> versions of Qt do use toplevel asm volatile and apparently the Qt code is
> copied into lots of various projects.
>
> In addition to that, it mentions in the documentation that qualifiers are
> not allowed at toplevel asm statements; apparently our documentation at
> least from r220506 for GCC 5 says that at toplevel Basic Asm needs to be
> used and for Basic Asm lists volatile qualifier as optional and its behavior
> (that it is ignored for Basic Asm).  Makes me wonder if we don't want to
> keep accepting/ignoring volatile at toplevel for both C and C++ instead of
> rejecting it (and rejecting just the other qualifiers).  Thoughts on this?

That seems reasonable.  Or using warning or permerror instead of error.

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> Attached is an untested backport of this patch to 8.4, which does allow
> asm volatile at toplevel, so that we don't break in 8.4 what has been
> accepted in 8.2.  Ok if it passes bootstrap/regtest there?

Both OK.

Jason
Reply | Threaded
Open this post in threaded view
|

[C++ PATCH] Toplevel asm volatile followup (PR c++/89585)

Jakub Jelinek
On Wed, Mar 06, 2019 at 07:44:25PM -0500, Jason Merrill wrote:
> > In addition to that, it mentions in the documentation that qualifiers are
> > not allowed at toplevel asm statements; apparently our documentation at
> > least from r220506 for GCC 5 says that at toplevel Basic Asm needs to be
> > used and for Basic Asm lists volatile qualifier as optional and its behavior
> > (that it is ignored for Basic Asm).  Makes me wonder if we don't want to
> > keep accepting/ignoring volatile at toplevel for both C and C++ instead of
> > rejecting it (and rejecting just the other qualifiers).  Thoughts on this?
>
> That seems reasonable.  Or using warning or permerror instead of error.

This incremental patch uses warning.  Bootstrapped/regtested on
x86_64-linux and i686-linux, ok for trunk?

2019-03-07  Jakub Jelinek  <[hidden email]>

        PR c++/89585
        * parser.c (cp_parser_asm_definition): Just warn instead of error
        on volatile qualifier outside of function body.

        * g++.dg/asm-qual-3.C: Adjust expected diagnostics for toplevel
        asm volatile.

--- gcc/cp/parser.c.jj 2019-03-07 09:17:41.406631683 +0100
+++ gcc/cp/parser.c 2019-03-07 10:06:49.297602880 +0100
@@ -19782,9 +19782,12 @@ cp_parser_asm_definition (cp_parser* par
  inform (volatile_loc, "first seen here");
       }
     else
-      volatile_loc = loc;
-    if (!first_loc)
-      first_loc = loc;
+      {
+ if (!parser->in_function_body)
+  warning_at (loc, 0, "asm qualifier %qT ignored outside of "
+      "function body", token->u.value);
+ volatile_loc = loc;
+      }
     cp_lexer_consume_token (parser->lexer);
     continue;
 
@@ -19830,10 +19833,10 @@ cp_parser_asm_definition (cp_parser* par
   bool inline_p = (inline_loc != UNKNOWN_LOCATION);
   bool goto_p = (goto_loc != UNKNOWN_LOCATION);
 
-  if (!parser->in_function_body && (volatile_p || inline_p || goto_p))
+  if (!parser->in_function_body && (inline_p || goto_p))
     {
       error_at (first_loc, "asm qualifier outside of function body");
-      volatile_p = inline_p = goto_p = false;
+      inline_p = goto_p = false;
     }
 
   /* Look for the opening `('.  */
--- gcc/testsuite/g++.dg/asm-qual-3.C.jj 2019-03-07 09:17:41.417631503 +0100
+++ gcc/testsuite/g++.dg/asm-qual-3.C 2019-03-07 10:11:18.160228806 +0100
@@ -3,7 +3,7 @@
 // { dg-options "-std=gnu++98" }
 
 asm const ("");    // { dg-error {'const' is not an asm qualifier} }
-asm volatile (""); // { dg-error {asm qualifier outside of function body} }
+asm volatile (""); // { dg-warning {asm qualifier 'volatile' ignored outside of function body} }
 asm restrict (""); // { dg-error {expected '\(' before 'restrict'} }
 asm inline ("");   // { dg-error {asm qualifier outside of function body} }
 asm goto ("");     // { dg-error {asm qualifier outside of function body} }


        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: [C++ PATCH] Toplevel asm volatile (PR c++/89585)

Matthias Klose-6
In reply to this post by Jakub Jelinek
On 07.03.19 00:39, Jakub Jelinek wrote:

> Hi!
>
> The following patch tries to improve diagnostics of toplevel asm qualifiers
> in C++ by actually parsing them and complaining if they appear at toplevel,
> instead of just emitting a parse error that ( is expected, e.g. some
> versions of Qt do use toplevel asm volatile and apparently the Qt code is
> copied into lots of various projects.
>
> In addition to that, it mentions in the documentation that qualifiers are
> not allowed at toplevel asm statements; apparently our documentation at
> least from r220506 for GCC 5 says that at toplevel Basic Asm needs to be
> used and for Basic Asm lists volatile qualifier as optional and its behavior
> (that it is ignored for Basic Asm).  Makes me wonder if we don't want to
> keep accepting/ignoring volatile at toplevel for both C and C++ instead of
> rejecting it (and rejecting just the other qualifiers).  Thoughts on this?
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> Attached is an untested backport of this patch to 8.4, which does allow
> asm volatile at toplevel, so that we don't break in 8.4 what has been
> accepted in 8.2.  Ok if it passes bootstrap/regtest there?

isn't that required for the gcc-7 branch as well? r267536 backported these
patches to the 7 branch as well.

Matthias
Reply | Threaded
Open this post in threaded view
|

Re: [C++ PATCH] Toplevel asm volatile (PR c++/89585)

Jakub Jelinek
On Thu, Mar 07, 2019 at 10:11:56PM +0100, Matthias Klose wrote:

> On 07.03.19 00:39, Jakub Jelinek wrote:
> > The following patch tries to improve diagnostics of toplevel asm qualifiers
> > in C++ by actually parsing them and complaining if they appear at toplevel,
> > instead of just emitting a parse error that ( is expected, e.g. some
> > versions of Qt do use toplevel asm volatile and apparently the Qt code is
> > copied into lots of various projects.
> >
> > In addition to that, it mentions in the documentation that qualifiers are
> > not allowed at toplevel asm statements; apparently our documentation at
> > least from r220506 for GCC 5 says that at toplevel Basic Asm needs to be
> > used and for Basic Asm lists volatile qualifier as optional and its behavior
> > (that it is ignored for Basic Asm).  Makes me wonder if we don't want to
> > keep accepting/ignoring volatile at toplevel for both C and C++ instead of
> > rejecting it (and rejecting just the other qualifiers).  Thoughts on this?
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > Attached is an untested backport of this patch to 8.4, which does allow
> > asm volatile at toplevel, so that we don't break in 8.4 what has been
> > accepted in 8.2.  Ok if it passes bootstrap/regtest there?
>
> isn't that required for the gcc-7 branch as well?

Yes.

> r267536 backported these patches to the 7 branch as well.

If you've tested it, feel free to commit it to 7.x.

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: [C++ PATCH] Toplevel asm volatile followup (PR c++/89585)

Jason Merrill
In reply to this post by Jakub Jelinek
On 3/7/19 2:13 PM, Jakub Jelinek wrote:

> On Wed, Mar 06, 2019 at 07:44:25PM -0500, Jason Merrill wrote:
>>> In addition to that, it mentions in the documentation that qualifiers are
>>> not allowed at toplevel asm statements; apparently our documentation at
>>> least from r220506 for GCC 5 says that at toplevel Basic Asm needs to be
>>> used and for Basic Asm lists volatile qualifier as optional and its behavior
>>> (that it is ignored for Basic Asm).  Makes me wonder if we don't want to
>>> keep accepting/ignoring volatile at toplevel for both C and C++ instead of
>>> rejecting it (and rejecting just the other qualifiers).  Thoughts on this?
>>
>> That seems reasonable.  Or using warning or permerror instead of error.
>
> This incremental patch uses warning.  Bootstrapped/regtested on
> x86_64-linux and i686-linux, ok for trunk?

OK.

Jason