[PATCH 1/4] c/c++, asm: Write the asm-qualifier loop without "done" boolean

[PATCH 1/4] c/c++, asm: Write the asm-qualifier loop without "done" boolean

[Segher Boessenkool]

As suggested by Jason.


Segher


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

c/
	* c-parser.c (c_parser_asm_statement): Rewrite the loop to work without
	"done" boolean variable.

cp/
	* parser.c (cp_parser_asm_definition): Rewrite the loop to work without
	"done" boolean variable.

---
 gcc/c/c-parser.c | 66 +++++++++++++++++++++++++---------------------------
 gcc/cp/parser.c  | 71 +++++++++++++++++++++++++-------------------------------
 2 files changed, 63 insertions(+), 74 deletions(-)

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index b875c4f..121a91c 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -6372,40 +6372,37 @@ c_parser_asm_statement (c_parser *parser)
   is_volatile = false;
   is_inline = false;
   is_goto = false;
-  for (bool done = false; !done; )
-    switch (c_parser_peek_token (parser)->keyword)
-      {
-      case RID_VOLATILE:
-	if (!is_volatile)
-	  {
-	    is_volatile = true;
-	    quals = c_parser_peek_token (parser)->value;
-	    c_parser_consume_token (parser);
-	  }
-	else
-	  done = true;
-	break;
-      case RID_INLINE:
-	if (!is_inline)
-	  {
-	    is_inline = true;
-	    c_parser_consume_token (parser);
-	  }
-	else
-	  done = true;
-	break;
-      case RID_GOTO:
-	if (!is_goto)
-	  {
-	    is_goto = true;
-	    c_parser_consume_token (parser);
-	  }
-	else
-	  done = true;
-	break;
-      default:
-	done = true;
-      }
+  for (;;)
+    {
+      switch (c_parser_peek_token (parser)->keyword)
+	{
+	case RID_VOLATILE:
+	  if (is_volatile)
+	    break;
+	  is_volatile = true;
+	  quals = c_parser_peek_token (parser)->value;
+	  c_parser_consume_token (parser);
+	  continue;
+
+	case RID_INLINE:
+	  if (is_inline)
+	    break;
+	  is_inline = true;
+	  c_parser_consume_token (parser);
+	  continue;
+
+	case RID_GOTO:
+	  if (is_goto)
+	    break;
+	  is_goto = true;
+	  c_parser_consume_token (parser);
+	  continue;
+
+	default:
+	  break;
+	}
+      break;
+    }
 
   /* ??? 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 adfe09e..1cc34ba 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -19648,47 +19648,38 @@ cp_parser_asm_definition (cp_parser* parser)
       cp_function_chain->invalid_constexpr = true;
     }
 
-  /* See if the next token is `volatile'.  */
+  /* Handle the asm-qualifier-list.  */
   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_INLINE:
-	  if (!inline_p && parser->in_function_body)
-	    {
-	      /* Remember that we saw the `inline' keyword.  */
-	      inline_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;
-	}
+    for (;;)
+      {
+	switch (cp_lexer_peek_token (parser->lexer)->keyword)
+	  {
+	  case RID_VOLATILE:
+	    if (volatile_p)
+	      break;
+	    volatile_p = true;
+	    cp_lexer_consume_token (parser->lexer);
+	    continue;
+
+	  case RID_INLINE:
+	    if (inline_p || !parser->in_function_body)
+	      break;
+	    inline_p = true;
+	    cp_lexer_consume_token (parser->lexer);
+	    continue;
+
+	  case RID_GOTO:
+	    if (goto_p || !parser->in_function_body)
+	      break;
+	    goto_p = true;
+	    cp_lexer_consume_token (parser->lexer);
+	    continue;
+
+	  default:
+	    break;
+	  }
+	break;
+      }
 
   /* Look for the opening `('.  */
   if (!cp_parser_require (parser, CPP_OPEN_PAREN, RT_OPEN_PAREN))
-- 
1.8.3.1

[PATCH 0/4] c/c++, asm: Various updates

[Segher Boessenkool]

This ties up some loose ends, and adds some more testcases.

Tested on powerpc64-linux {-m32,-m64}.  Is this okay for trunk?


Segher


Segher Boessenkool (4):
  c/c++, asm: Write the asm-qualifier loop without "done" boolean
  c/c++, asm: Use nicer error for duplicate asm qualifiers
  c/c++, asm: Use nicer warning for const and restrict
  c++, asm: Do not handle any asm-qualifiers in top-level asm

 gcc/c/c-parser.c                  | 106 ++++++++++++++++++++++---------------
 gcc/c/c-tree.h                    |   2 +-
 gcc/c/c-typeck.c                  |   4 +-
 gcc/cp/parser.c                   | 107 ++++++++++++++++++++++----------------
 gcc/testsuite/g++.dg/asm-qual-1.C |  13 +++++
 gcc/testsuite/g++.dg/asm-qual-2.C |  46 ++++++++++++++++
 gcc/testsuite/g++.dg/asm-qual-3.C |  12 +++++
 gcc/testsuite/gcc.dg/asm-qual-1.c |   6 +--
 gcc/testsuite/gcc.dg/asm-qual-3.c |   9 ++++
 9 files changed, 209 insertions(+), 96 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/asm-qual-1.C
 create mode 100644 gcc/testsuite/g++.dg/asm-qual-2.C
 create mode 100644 gcc/testsuite/g++.dg/asm-qual-3.C
 create mode 100644 gcc/testsuite/gcc.dg/asm-qual-3.c

-- 
1.8.3.1

[PATCH 4/4] c++, asm: Do not handle any asm-qualifiers in top-level asm

[Segher Boessenkool]

Previously, "volatile" was allowed.  Changing this simplifies the code,
makes things more regular, and makes the C and C++ frontends handle
this the same way.


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

cp/
	* parser.c (cp_parser_asm_definition): Do not allow any asm qualifiers
	on top-level asm.

testsuite/
	* g++.dg/asm-qual-3.C: New testcase.
	* gcc.dg/asm-qual-3.c: New testcase.

---
 gcc/cp/parser.c                   |  7 ++-----
 gcc/testsuite/g++.dg/asm-qual-3.C | 12 ++++++++++++
 gcc/testsuite/gcc.dg/asm-qual-3.c |  9 +++++++++
 3 files changed, 23 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/asm-qual-3.C
 create mode 100644 gcc/testsuite/gcc.dg/asm-qual-3.c

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 4072afe..62c2a4f 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -19649,7 +19649,8 @@ cp_parser_asm_definition (cp_parser* parser)
   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))
+
+  if (cp_parser_allow_gnu_extensions_p (parser) && parser->in_function_body)
     for (;;)
       {
 	cp_token *token = cp_lexer_peek_token (parser->lexer);
@@ -19668,8 +19669,6 @@ cp_parser_asm_definition (cp_parser* parser)
 	    continue;
 
 	  case RID_INLINE:
-	    if (!parser->in_function_body)
-	      break;
 	    if (inline_loc)
 	      {
 		error_at (loc, "duplicate asm qualifier %qT", token->u.value);
@@ -19681,8 +19680,6 @@ cp_parser_asm_definition (cp_parser* parser)
 	    continue;
 
 	  case RID_GOTO:
-	    if (!parser->in_function_body)
-	      break;
 	    if (goto_loc)
 	      {
 		error_at (loc, "duplicate asm qualifier %qT", token->u.value);
diff --git a/gcc/testsuite/g++.dg/asm-qual-3.C b/gcc/testsuite/g++.dg/asm-qual-3.C
new file mode 100644
index 0000000..95c9b57
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asm-qual-3.C
@@ -0,0 +1,12 @@
+// Test that asm-qualifiers are not allowed on toplevel asm.
+// { dg-do compile }
+// { dg-options "-std=gnu++98" }
+
+asm const ("");    // { dg-error {expected '\(' before 'const'} }
+asm volatile (""); // { dg-error {expected '\(' before 'volatile'} }
+asm restrict (""); // { dg-error {expected '\(' before 'restrict'} }
+asm inline ("");   // { dg-error {expected '\(' before 'inline'} }
+asm goto ("");     // { dg-error {expected '\(' before 'goto'} }
+
+// There are many other things wrong with this code, so:
+// { dg-excess-errors "" }
diff --git a/gcc/testsuite/gcc.dg/asm-qual-3.c b/gcc/testsuite/gcc.dg/asm-qual-3.c
new file mode 100644
index 0000000..f85d8bf
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/asm-qual-3.c
@@ -0,0 +1,9 @@
+/* Test that asm-qualifiers are not allowed on toplevel asm.  */
+/* { dg-do compile } */
+/* { dg-options "-std=gnu99" } */
+
+asm const ("");    /* { dg-error {expected '\(' before 'const'} } */
+asm volatile (""); /* { dg-error {expected '\(' before 'volatile'} } */
+asm restrict (""); /* { dg-error {expected '\(' before 'restrict'} } */
+asm inline ("");   /* { dg-error {expected '\(' before 'inline'} } */
+asm goto ("");     /* { dg-error {expected '\(' before 'goto'} } */
-- 
1.8.3.1

[PATCH 3/4] c/c++, asm: Use nicer error for const and restrict

[Segher Boessenkool]

Not all qualifiers are asm qualifiers.  We can talk about that in a
nicer way than just giving a generic parser error.

This also adds two testcases for C++, that previously were for C only.


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

c/
	* c-parser.c (c_parser_asm_statement) <RID_CONST, RID_RESTRICT>: Give
	a more specific error message (instead of just falling through).

cp/
	* parser.c (cp_parser_asm_definition) <RID_CONST, RID_RESTRICT>: Give
	a more specific error message (instead of just falling through).

testsuite/
	* g++.dg/asm-qual-1.C: New testcase.
	* g++.dg/asm-qual-2.C: New testcase.
	* gcc.dg/asm-qual-1.c: Update.

---
 gcc/c/c-parser.c                  |  6 +++++
 gcc/cp/parser.c                   |  6 +++++
 gcc/testsuite/g++.dg/asm-qual-1.C | 13 +++++++++++
 gcc/testsuite/g++.dg/asm-qual-2.C | 46 +++++++++++++++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/asm-qual-1.c |  6 ++---
 5 files changed, 73 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/asm-qual-1.C
 create mode 100644 gcc/testsuite/g++.dg/asm-qual-2.C

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 652e53c..0def497 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -6411,6 +6411,12 @@ c_parser_asm_statement (c_parser *parser)
 	  c_parser_consume_token (parser);
 	  continue;
 
+	case RID_CONST:
+	case RID_RESTRICT:
+	  error_at (loc, "%qE is not an asm qualifier", token->value);
+	  c_parser_consume_token (parser);
+	  continue;
+
 	default:
 	  break;
 	}
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 06a6bb0..4072afe 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -19693,6 +19693,12 @@ cp_parser_asm_definition (cp_parser* parser)
 	    cp_lexer_consume_token (parser->lexer);
 	    continue;
 
+	  case RID_CONST:
+	  case RID_RESTRICT:
+	    error_at (loc, "%qT is not an asm qualifier", token->u.value);
+	    cp_lexer_consume_token (parser->lexer);
+	    continue;
+
 	  default:
 	    break;
 	  }
diff --git a/gcc/testsuite/g++.dg/asm-qual-1.C b/gcc/testsuite/g++.dg/asm-qual-1.C
new file mode 100644
index 0000000..3fba592
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asm-qual-1.C
@@ -0,0 +1,13 @@
+// Test that qualifiers other than volatile are disallowed on asm.
+// { dg-do compile }
+// { dg-options "-std=gnu++98" }
+
+void
+f ()
+{
+  asm volatile ("");
+
+  asm const (""); // { dg-error {'const' is not an asm qualifier} }
+
+  asm __restrict (""); // { dg-error {'__restrict' is not an asm qualifier} }
+}
diff --git a/gcc/testsuite/g++.dg/asm-qual-2.C b/gcc/testsuite/g++.dg/asm-qual-2.C
new file mode 100644
index 0000000..52968bd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asm-qual-2.C
@@ -0,0 +1,46 @@
+// Test that qualifiers on asm are allowed in any order.
+// { dg-do compile }
+// { dg-options "-std=c++98" }
+
+void
+f ()
+{
+  asm volatile goto ("" :::: lab);
+  asm volatile inline ("" :::);
+  asm inline volatile ("" :::);
+  asm inline goto ("" :::: lab);
+  asm goto volatile ("" :::: lab);
+  asm goto inline ("" :::: lab);
+
+  asm volatile inline goto ("" :::: lab);
+  asm volatile goto inline ("" :::: lab);
+  asm inline volatile goto ("" :::: lab);
+  asm inline goto volatile ("" :::: lab);
+  asm goto volatile inline ("" :::: lab);
+  asm goto inline volatile ("" :::: lab);
+
+  /* Duplicates are not allowed.  */
+  asm goto volatile volatile ("" :::: lab);  /* { dg-error "" } */
+  asm volatile goto volatile ("" :::: lab);  /* { dg-error "" } */
+  asm volatile volatile goto ("" :::: lab);  /* { dg-error "" } */
+  asm goto goto volatile ("" :::: lab);  /* { dg-error "" } */
+  asm goto volatile goto ("" :::: lab);  /* { dg-error "" } */
+  asm volatile goto goto ("" :::: lab);  /* { dg-error "" } */
+
+  asm inline volatile volatile ("" :::);  /* { dg-error "" } */
+  asm volatile inline volatile ("" :::);  /* { dg-error "" } */
+  asm volatile volatile inline ("" :::);  /* { dg-error "" } */
+  asm inline inline volatile ("" :::);  /* { dg-error "" } */
+  asm inline volatile inline ("" :::);  /* { dg-error "" } */
+  asm volatile inline inline ("" :::);  /* { dg-error "" } */
+
+  asm goto inline inline ("" :::: lab);  /* { dg-error "" } */
+  asm inline goto inline ("" :::: lab);  /* { dg-error "" } */
+  asm inline inline goto ("" :::: lab);  /* { dg-error "" } */
+  asm goto goto inline ("" :::: lab);  /* { dg-error "" } */
+  asm goto inline goto ("" :::: lab);  /* { dg-error "" } */
+  asm inline goto goto ("" :::: lab);  /* { dg-error "" } */
+
+lab:
+  ;
+}
diff --git a/gcc/testsuite/gcc.dg/asm-qual-1.c b/gcc/testsuite/gcc.dg/asm-qual-1.c
index cb37283..eff6b45 100644
--- a/gcc/testsuite/gcc.dg/asm-qual-1.c
+++ b/gcc/testsuite/gcc.dg/asm-qual-1.c
@@ -8,9 +8,7 @@ f (void)
 {
   asm volatile ("");
 
-  asm const (""); /* { dg-error {expected '\(' before 'const'} } */
-  /* { dg-error {expected identifier} {} {target *-*-*} .-1 } */
+  asm const (""); /* { dg-error {'const' is not an asm qualifier} } */
 
-  asm restrict (""); /* { dg-error {expected '\(' before 'restrict'} } */
-  /* { dg-error {expected identifier} {} {target *-*-*} .-1 } */
+  asm restrict (""); /* { dg-error {'restrict' is not an asm qualifier} } */
 }
-- 
1.8.3.1

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

[Segher Boessenkool]

Also as suggested by Jason.


Segher


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

c/
	* c-parser.c (c_parser_asm_statement): Keep track of the location each
	asm qualifier is first seen; use that to give nicer "duplicate asm
	qualifier" messages.  Delete 'quals" variable, instead pass the
	"is_volatile_ flag to build_asm_stmt directly.
	* c-tree.h (build_asm_stmt): Make the first arg bool instead of tree.
	* c-typeck.c (build_asm_stmt): Ditto; adjust.

cp/
	* parser.c (cp_parser_asm_definition): Rewrite the loop to work without
	"done" boolean variable.
	* parser.c (cp_parser_asm_definition): Keep track of the location each
	asm qualifier is first seen; use that to give nicer "duplicate asm
	qualifier" messages.

---
 gcc/c/c-parser.c | 58 ++++++++++++++++++++++++++++++++++++--------------------
 gcc/c/c-tree.h   |  2 +-
 gcc/c/c-typeck.c |  4 ++--
 gcc/cp/parser.c  | 45 +++++++++++++++++++++++++++++++------------
 4 files changed, 73 insertions(+), 36 deletions(-)

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");
+	    }
+	  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");
+	    }
+	  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");
+	    }
+	  else
+	    goto_loc = loc;
 	  c_parser_consume_token (parser);
 	  continue;
 
@@ -6405,6 +6417,10 @@ c_parser_asm_statement (c_parser *parser)
       break;
     }
 
+  bool is_volatile = (volatile_loc != UNKNOWN_LOCATION);
+  bool is_inline = (inline_loc != UNKNOWN_LOCATION);
+  bool is_goto = (goto_loc != UNKNOWN_LOCATION);
+
   /* ??? Follow the C++ parser rather than using the
      lex_untranslated_string kludge.  */
   parser->lex_untranslated_string = true;
@@ -6479,9 +6495,9 @@ c_parser_asm_statement (c_parser *parser)
   if (!c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>"))
     c_parser_skip_to_end_of_block_or_statement (parser);
 
-  ret = build_asm_stmt (quals, build_asm_expr (asm_loc, str, outputs, inputs,
-					       clobbers, labels, simple,
-					       is_inline));
+  ret = build_asm_stmt (is_volatile,
+			build_asm_expr (asm_loc, str, outputs, inputs,
+					clobbers, labels, simple, is_inline));
 
  error:
   parser->lex_untranslated_string = false;
diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index f08a8fc..dc9e3cd 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -679,7 +679,7 @@ extern tree c_start_case (location_t, location_t, tree, bool);
 extern void c_finish_case (tree, tree);
 extern tree build_asm_expr (location_t, tree, tree, tree, tree, tree, bool,
 			    bool);
-extern tree build_asm_stmt (tree, tree);
+extern tree build_asm_stmt (bool, tree);
 extern int c_types_compatible_p (tree, tree);
 extern tree c_begin_compound_stmt (bool);
 extern tree c_end_compound_stmt (location_t, tree, bool);
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 1a89727..abf9b21 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -10316,9 +10316,9 @@ process_init_element (location_t loc, struct c_expr value, bool implicit,
    (guaranteed to be 'volatile' or null) and ARGS (represented using
    an ASM_EXPR node).  */
 tree
-build_asm_stmt (tree cv_qualifier, tree args)
+build_asm_stmt (bool is_volatile, tree args)
 {
-  if (!ASM_VOLATILE_P (args) && cv_qualifier)
+  if (is_volatile)
     ASM_VOLATILE_P (args) = 1;
   return add_stmt (args);
 }
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
@@ -19630,12 +19630,9 @@ cp_parser_asm_definition (cp_parser* parser)
   tree clobbers = NULL_TREE;
   tree labels = NULL_TREE;
   tree asm_stmt;
-  bool volatile_p = false;
   bool extended_p = false;
   bool invalid_inputs_p = false;
   bool invalid_outputs_p = false;
-  bool inline_p = false;
-  bool goto_p = false;
   required_token missing = RT_NONE;
 
   /* Look for the `asm' keyword.  */
@@ -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");
+	      }
+	    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");
+	      }
+	    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");
+	      }
+	    else
+	      goto_loc = loc;
 	    cp_lexer_consume_token (parser->lexer);
 	    continue;
 
@@ -19681,6 +19699,10 @@ cp_parser_asm_definition (cp_parser* parser)
 	break;
       }
 
+  bool volatile_p = (volatile_loc != UNKNOWN_LOCATION);
+  bool inline_p = (inline_loc != UNKNOWN_LOCATION);
+  bool goto_p = (goto_loc != UNKNOWN_LOCATION);
+
   /* Look for the opening `('.  */
   if (!cp_parser_require (parser, CPP_OPEN_PAREN, RT_OPEN_PAREN))
     return;
@@ -19772,8 +19794,7 @@ cp_parser_asm_definition (cp_parser* parser)
 					     CPP_CLOSE_PAREN))
 	    clobbers = cp_parser_asm_clobber_list (parser);
 	}
-      else if (goto_p
-	       && cp_lexer_next_token_is (parser->lexer, CPP_SCOPE))
+      else if (goto_p && cp_lexer_next_token_is (parser->lexer, CPP_SCOPE))
 	/* The labels are coming next.  */
 	labels_p = true;
 
-- 
1.8.3.1

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

[David Malcolm]

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


> +	  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

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

[David Malcolm]

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

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

[Martin Sebor]

> +	    {
> +	      error_at (loc, "duplicate asm qualifier %qE", token->value);

We have been making an effort to quote keywords, identifiers,
option names, and other such things in diagnostics.  In
the message above and all others like it in this patch kit
that mention "asm" the keyword should be quoted the same
way as the name of the qualifier.

Thanks
Martin

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

[Segher Boessenkool]

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.

> 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.


Segher

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

[Segher Boessenkool]

On Tue, Dec 11, 2018 at 10:48:15AM -0500, David Malcolm wrote:
> For bonus points, these could offer fix-it hints, so that an IDE can
> offer to delete the duplicate qualifier token.

Yes it could.  But have you ever seen this error, in a real program?
(It was an error before, just without a nice message).


Segher

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

[Segher Boessenkool]

On Tue, Dec 11, 2018 at 10:31:02AM -0700, Martin Sebor wrote:
> >+	    {
> >+	      error_at (loc, "duplicate asm qualifier %qE", token->value);
> 
> We have been making an effort to quote keywords, identifiers,
> option names, and other such things in diagnostics.  In
> the message above and all others like it in this patch kit
> that mention "asm" the keyword should be quoted the same
> way as the name of the qualifier.

This message is about "asm qualifiers".  It is not about the "asm"
statement.  You can write this without "asm" keyword, too, anyway (with
an "__asm__" or such), making the message even more awkward to quote in
that case.  The location of the error has nothing to do with the "asm",
either.

You should only quote keywords that are in the source text.  Not random
words that *could* be a keyword :-)


Segher

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

[Martin Sebor]

On 12/12/18 10:50 AM, Segher Boessenkool wrote:
> On Tue, Dec 11, 2018 at 10:31:02AM -0700, Martin Sebor wrote:
>>> +	    {
>>> +	      error_at (loc, "duplicate asm qualifier %qE", token->value);
>>
>> We have been making an effort to quote keywords, identifiers,
>> option names, and other such things in diagnostics.  In
>> the message above and all others like it in this patch kit
>> that mention "asm" the keyword should be quoted the same
>> way as the name of the qualifier.
> 
> This message is about "asm qualifiers".  It is not about the "asm"
> statement.  You can write this without "asm" keyword, too, anyway (with
> an "__asm__" or such), making the message even more awkward to quote in
> that case.  The location of the error has nothing to do with the "asm",
> either.
> 
> You should only quote keywords that are in the source text.  Not random
> words that *could* be a keyword :-)

asm is not a random word, or even an English word (please look
in a dictionary if you're in doubt).  It is a C/C++ keyword :-)

Martin

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

[Segher Boessenkool]

On Wed, Dec 12, 2018 at 11:02:29AM -0700, Martin Sebor wrote:
> On 12/12/18 10:50 AM, Segher Boessenkool wrote:
> >On Tue, Dec 11, 2018 at 10:31:02AM -0700, Martin Sebor wrote:
> >>>+	    {
> >>>+	      error_at (loc, "duplicate asm qualifier %qE", token->value);
> >>
> >>We have been making an effort to quote keywords, identifiers,
> >>option names, and other such things in diagnostics.  In
> >>the message above and all others like it in this patch kit
> >>that mention "asm" the keyword should be quoted the same
> >>way as the name of the qualifier.
> >
> >This message is about "asm qualifiers".  It is not about the "asm"
> >statement.  You can write this without "asm" keyword, too, anyway (with
> >an "__asm__" or such), making the message even more awkward to quote in
> >that case.  The location of the error has nothing to do with the "asm",
> >either.
> >
> >You should only quote keywords that are in the source text.  Not random
> >words that *could* be a keyword :-)
> 
> asm is not a random word, or even an English word (please look
> in a dictionary if you're in doubt).  It is a C/C++ keyword :-)

An "asm qualifier" is a GCC extension.


Segher

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

[David Malcolm]

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

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

[Segher Boessenkool]

On Wed, Dec 12, 2018 at 06:23:08PM -0500, David Malcolm wrote:
> On Wed, 2018-12-12 at 11:40 -0600, Segher Boessenkool wrote:
> > > 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" } */

Hrm.

> > > 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 :;
>                ^~~~

Yup.

> where each error/note pair are in their own diagnostic group.  I don't
> think it matters that the locations "overlap" here.

So how can I do that, "put them in their own group"?  As far as I can
see the interface allows only nested groups.

> 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.

If you declare the auto_group somewhere inside the switch (or anywhere
in the loop even), no two qualifiers will be in the same group; and if
you put the auto_group outside the loop, all asm qualifiers will be in
one and the same group.  Or, what do I misunderstand here?

> Let me know if you want me to update the patch for you.

Please do a separate patch on top?

> Hope this is constructive

It is :-)


Segher

Ping: [PATCH 0/4] c/c++, asm: Various updates

[Segher Boessenkool]

Ping?

On Mon, Dec 10, 2018 at 10:47:23PM +0000, Segher Boessenkool wrote:
> This ties up some loose ends, and adds some more testcases.
> 
> Tested on powerpc64-linux {-m32,-m64}.  Is this okay for trunk?
> 
> 
> Segher
> 
> 
> Segher Boessenkool (4):
>   c/c++, asm: Write the asm-qualifier loop without "done" boolean
>   c/c++, asm: Use nicer error for duplicate asm qualifiers
>   c/c++, asm: Use nicer warning for const and restrict
>   c++, asm: Do not handle any asm-qualifiers in top-level asm
> 
>  gcc/c/c-parser.c                  | 106 ++++++++++++++++++++++---------------
>  gcc/c/c-tree.h                    |   2 +-
>  gcc/c/c-typeck.c                  |   4 +-
>  gcc/cp/parser.c                   | 107 ++++++++++++++++++++++----------------
>  gcc/testsuite/g++.dg/asm-qual-1.C |  13 +++++
>  gcc/testsuite/g++.dg/asm-qual-2.C |  46 ++++++++++++++++
>  gcc/testsuite/g++.dg/asm-qual-3.C |  12 +++++
>  gcc/testsuite/gcc.dg/asm-qual-1.c |   6 +--
>  gcc/testsuite/gcc.dg/asm-qual-3.c |   9 ++++
>  9 files changed, 209 insertions(+), 96 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/asm-qual-1.C
>  create mode 100644 gcc/testsuite/g++.dg/asm-qual-2.C
>  create mode 100644 gcc/testsuite/g++.dg/asm-qual-3.C
>  create mode 100644 gcc/testsuite/gcc.dg/asm-qual-3.c
> 
> -- 
> 1.8.3.1

Re: [PATCH 1/4] c/c++, asm: Write the asm-qualifier loop without "done" boolean

[Jason Merrill]

On 12/10/18 5:47 PM, Segher Boessenkool wrote:
> As suggested by Jason.
> 
> 
> Segher
> 
> 
> 2018-12-10  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> c/
> 	* c-parser.c (c_parser_asm_statement): Rewrite the loop to work without
> 	"done" boolean variable.
> 
> cp/
> 	* parser.c (cp_parser_asm_definition): Rewrite the loop to work without
> 	"done" boolean variable.

OK, thanks.

Jason

Re: [PATCH 4/4] c++, asm: Do not handle any asm-qualifiers in top-level asm

[Jason Merrill]

On 12/10/18 5:47 PM, Segher Boessenkool wrote:
> Previously, "volatile" was allowed.  Changing this simplifies the code,
> makes things more regular, and makes the C and C++ frontends handle
> this the same way.
> 
> 
> 2018-12-10  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> cp/
> 	* parser.c (cp_parser_asm_definition): Do not allow any asm qualifiers
> 	on top-level asm.

OK.

Jason

Re: [PATCH 3/4] c/c++, asm: Use nicer error for const and restrict

[Jason Merrill]

On 12/10/18 5:47 PM, Segher Boessenkool wrote:
> Not all qualifiers are asm qualifiers.  We can talk about that in a
> nicer way than just giving a generic parser error.
> 
> This also adds two testcases for C++, that previously were for C only.
> 
> 
> 2018-12-10  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> c/
> 	* c-parser.c (c_parser_asm_statement) <RID_CONST, RID_RESTRICT>: Give
> 	a more specific error message (instead of just falling through).
> 
> cp/
> 	* parser.c (cp_parser_asm_definition) <RID_CONST, RID_RESTRICT>: Give
> 	a more specific error message (instead of just falling through).

OK.

Jason