[c++ patch] Fix PRs 30849/30850/30851

[c++ patch] Fix PRs 30849/30850/30851

[Lee Millward]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

Hi,

These three bugs are to do with inline ASM statements containing
invalid inputs/outputs that trigger ICEs in various part of the
compiler not prepared to handle error_mark_nodes. Since all of these
bugs are very similiar in nature and the proposed fixes is both small
and touches the same piece of code, I have grouped them all together
rather than break them up into separate patches.

The attached patch catches these invalid statements in the parser by
adding two checks to cp_parser_asm_definition for the parsing of
inputs and outputs. In each case it looks to see if the parsed
statement was invalid, in which case an error will already have been
issued to that effect and discards those statements found to be
invalid. This approach seemed cleaner to me than adding in checks for
error_mark_nodes to the affected places but I can test a patch to do
that if it's deemed a better aproach.

Bootstrapped and regression tested on i686-pc-linux-gnu with no new
regressions. Ok for mainline and release branches?

Cheers,
Lee.

:ADDPATCH c++:

cp/
	PR c++/30849
	PR c++/30850
	PR c++/30851
	* parser.c (cp_parser_asm_definition): Detect and discard asm
	statements with invalid inputs or outputs.

testsuite/
	PR c++/30849
	* g++.dg/parse/asm1.C: New test.

	PR c++/30850
	* g++.dg/parse/asm2.C: Likewise.

	PR c++/30851
	* g++.dg/parse/asm3.C: Likewise.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3467 bytes --]

Index: gcc/testsuite/g++.dg/parse/asm1.C
===================================================================
--- gcc/testsuite/g++.dg/parse/asm1.C	(revision 0)
+++ gcc/testsuite/g++.dg/parse/asm1.C	(revision 0)
@@ -0,0 +1,6 @@
+//PR c++/30849
+
+void foo()
+{
+  asm("" : 0);  // { dg-error "numeric constant|token" }
+}
Index: gcc/testsuite/g++.dg/parse/asm2.C
===================================================================
--- gcc/testsuite/g++.dg/parse/asm2.C	(revision 0)
+++ gcc/testsuite/g++.dg/parse/asm2.C	(revision 0)
@@ -0,0 +1,6 @@
+//PR c++/30850
+
+void foo()
+{
+  asm("" :: 0);  // { dg-error "numeric constant|token" }
+}
Index: gcc/testsuite/g++.dg/parse/asm3.C
===================================================================
--- gcc/testsuite/g++.dg/parse/asm3.C	(revision 0)
+++ gcc/testsuite/g++.dg/parse/asm3.C	(revision 0)
@@ -0,0 +1,6 @@
+//PR c++/30851
+
+void foo()
+{
+  asm ("%[x]" : [0](x));  // { dg-error "numeric constant|token" }
+}
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 126720)
+++ gcc/cp/parser.c	(working copy)
@@ -11563,6 +11563,8 @@ cp_parser_asm_definition (cp_parser* par
   tree asm_stmt;
   bool volatile_p = false;
   bool extended_p = false;
+  bool invalid_inputs_p = false;
+  bool invalid_outputs_p = false;
 
   /* Look for the `asm' keyword.  */
   cp_parser_require_keyword (parser, RID_ASM, "`asm'");
@@ -11616,6 +11618,10 @@ cp_parser_asm_definition (cp_parser* par
 	      && cp_lexer_next_token_is_not (parser->lexer,
 					     CPP_CLOSE_PAREN))
 	    outputs = cp_parser_asm_operand_list (parser);
+
+	    if (outputs
+		&& TREE_VALUE (TREE_PURPOSE (outputs)) == error_mark_node)
+	      invalid_outputs_p = true;
 	}
       /* If the next token is `::', there are no outputs, and the
 	 next token is the beginning of the inputs.  */
@@ -11635,6 +11641,10 @@ cp_parser_asm_definition (cp_parser* par
 	      && cp_lexer_next_token_is_not (parser->lexer,
 					     CPP_CLOSE_PAREN))
 	    inputs = cp_parser_asm_operand_list (parser);
+
+	    if (inputs
+		&& TREE_VALUE (TREE_PURPOSE (inputs)) == error_mark_node)
+	      invalid_inputs_p = true;
 	}
       else if (cp_lexer_next_token_is (parser->lexer, CPP_SCOPE))
 	/* The clobbers are coming next.  */
@@ -11658,23 +11668,26 @@ cp_parser_asm_definition (cp_parser* par
 					   /*consume_paren=*/true);
   cp_parser_require (parser, CPP_SEMICOLON, "`;'");
 
-  /* Create the ASM_EXPR.  */
-  if (parser->in_function_body)
+  if (!invalid_inputs_p && !invalid_outputs_p)
     {
-      asm_stmt = finish_asm_stmt (volatile_p, string, outputs,
-				  inputs, clobbers);
-      /* If the extended syntax was not used, mark the ASM_EXPR.  */
-      if (!extended_p)
-	{
-	  tree temp = asm_stmt;
-	  if (TREE_CODE (temp) == CLEANUP_POINT_EXPR)
-	    temp = TREE_OPERAND (temp, 0);
+      /* Create the ASM_EXPR.  */
+      if (parser->in_function_body)
+	{
+	  asm_stmt = finish_asm_stmt (volatile_p, string, outputs,
+				      inputs, clobbers);
+	  /* If the extended syntax was not used, mark the ASM_EXPR.  */
+	  if (!extended_p)
+	    {
+	      tree temp = asm_stmt;
+	      if (TREE_CODE (temp) == CLEANUP_POINT_EXPR)
+		temp = TREE_OPERAND (temp, 0);
 
-	  ASM_INPUT_P (temp) = 1;
+	      ASM_INPUT_P (temp) = 1;
+	    }
 	}
+      else
+	cgraph_add_asm_node (string);
     }
-  else
-    cgraph_add_asm_node (string);
 }
 
 /* Declarators [gram.dcl.decl] */

Re: [c++ patch] Fix PRs 30849/30850/30851

[Nathan Sidwell]

Lee Millward wrote:
> Hi,
> 
> These three bugs are to do with inline ASM statements containing
> invalid inputs/outputs that trigger ICEs in various part of the
> compiler not prepared to handle error_mark_nodes. Since all of these
> bugs are very similiar in nature and the proposed fixes is both small
> and touches the same piece of code, I have grouped them all together
> rather than break them up into separate patches.
> 
> The attached patch catches these invalid statements in the parser by
> adding two checks to cp_parser_asm_definition for the parsing of
> inputs and outputs. In each case it looks to see if the parsed
> statement was invalid, in which case an error will already have been
> issued to that effect and discards those statements found to be
> invalid. This approach seemed cleaner to me than adding in checks for
> error_mark_nodes to the affected places but I can test a patch to do
> that if it's deemed a better aproach.

I think your approach is fine.

>  	    inputs = cp_parser_asm_operand_list (parser);
> +
> +	    if (inputs
> +		&& TREE_VALUE (TREE_PURPOSE (inputs)) == error_mark_node)
> +	      invalid_inputs_p = true;

Doesn't this just check if the first operand was bogus though?

Why not just have cp_parser_asm_operand_list return error_mark_node in the face 
of any bogus operand? (with suitable operand eating in that case)?

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: [c++ patch] Fix PRs 30849/30850/30851

[Lee Millward]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

Hi Nathan,

On 7/22/07, Nathan Sidwell <nathan@codesourcery.com> wrote:
> Why not just have cp_parser_asm_operand_list return error_mark_node in the face
> of any bogus operand? (with suitable operand eating in that case)?

The attached patch implements this suggestion by modifying
cp_parser_asm_operand_list to track whether any of the operands are
invalid and to return error_mark_node in that case, adjusting the
checks in cp_parser_asm_definition accordingly.

Bootstrapped and regression tested with no new failures on
i686-pc-linux-gnu. Ok to apply?

Cheers,
Lee.

cp/
	PR c++/30849
	PR c++/30850
	PR c++/30851
	* parser.c (cp_parser_asm_definition): Detect and discard asm
	statements with invalid inputs or outputs.
        (cp_parser_asm_operand_list): Return error mark node if any
        of the operands are invalid. Adjust documentation to reflect new
        potential return value.

testsuite/
	PR c++/30849
	* g++.dg/parse/asm1.C: New test.

	PR c++/30850
	* g++.dg/parse/asm2.C: Likewise.

	PR c++/30851
	* g++.dg/parse/asm3.C: Likewise.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3730 bytes --]

Index: parser.c
===================================================================
--- parser.c	(revision 126995)
+++ parser.c	(working copy)
@@ -11756,6 +11756,8 @@ cp_parser_asm_definition (cp_parser* par
   tree asm_stmt;
   bool volatile_p = false;
   bool extended_p = false;
+  bool invalid_inputs_p = false;
+  bool invalid_outputs_p = false;
 
   /* Look for the `asm' keyword.  */
   cp_parser_require_keyword (parser, RID_ASM, "`asm'");
@@ -11809,6 +11811,9 @@ cp_parser_asm_definition (cp_parser* par
 	      && cp_lexer_next_token_is_not (parser->lexer,
 					     CPP_CLOSE_PAREN))
 	    outputs = cp_parser_asm_operand_list (parser);
+
+	    if (outputs == error_mark_node)
+	      invalid_outputs_p = true;
 	}
       /* If the next token is `::', there are no outputs, and the
 	 next token is the beginning of the inputs.  */
@@ -11828,6 +11833,9 @@ cp_parser_asm_definition (cp_parser* par
 	      && cp_lexer_next_token_is_not (parser->lexer,
 					     CPP_CLOSE_PAREN))
 	    inputs = cp_parser_asm_operand_list (parser);
+
+	    if (inputs == error_mark_node)
+	      invalid_inputs_p = true;
 	}
       else if (cp_lexer_next_token_is (parser->lexer, CPP_SCOPE))
 	/* The clobbers are coming next.  */
@@ -11851,23 +11859,26 @@ cp_parser_asm_definition (cp_parser* par
 					   /*consume_paren=*/true);
   cp_parser_require (parser, CPP_SEMICOLON, "`;'");
 
-  /* Create the ASM_EXPR.  */
-  if (parser->in_function_body)
+  if (!invalid_inputs_p && !invalid_outputs_p)
     {
-      asm_stmt = finish_asm_stmt (volatile_p, string, outputs,
-				  inputs, clobbers);
-      /* If the extended syntax was not used, mark the ASM_EXPR.  */
-      if (!extended_p)
-	{
-	  tree temp = asm_stmt;
-	  if (TREE_CODE (temp) == CLEANUP_POINT_EXPR)
-	    temp = TREE_OPERAND (temp, 0);
+      /* Create the ASM_EXPR.  */
+      if (parser->in_function_body)
+	{
+	  asm_stmt = finish_asm_stmt (volatile_p, string, outputs,
+				      inputs, clobbers);
+	  /* If the extended syntax was not used, mark the ASM_EXPR.  */
+	  if (!extended_p)
+	    {
+	      tree temp = asm_stmt;
+	      if (TREE_CODE (temp) == CLEANUP_POINT_EXPR)
+		temp = TREE_OPERAND (temp, 0);
 
-	  ASM_INPUT_P (temp) = 1;
+	      ASM_INPUT_P (temp) = 1;
+	    }
 	}
+      else
+	cgraph_add_asm_node (string);
     }
-  else
-    cgraph_add_asm_node (string);
 }
 
 /* Declarators [gram.dcl.decl] */
@@ -15646,12 +15657,14 @@ cp_parser_asm_specification_opt (cp_pars
    each node is the expression.  The TREE_PURPOSE is itself a
    TREE_LIST whose TREE_PURPOSE is a STRING_CST for the bracketed
    string-literal (or NULL_TREE if not present) and whose TREE_VALUE
-   is a STRING_CST for the string literal before the parenthesis.  */
+   is a STRING_CST for the string literal before the parenthesis. Returns
+   ERROR_MARK_NODE if any of the operands are invalid.  */
 
 static tree
 cp_parser_asm_operand_list (cp_parser* parser)
 {
   tree asm_operands = NULL_TREE;
+  bool invalid_operands = false;
 
   while (true)
     {
@@ -15683,6 +15696,11 @@ cp_parser_asm_operand_list (cp_parser* p
       /* Look for the `)'.  */
       cp_parser_require (parser, CPP_CLOSE_PAREN, "`)'");
 
+      if (name == error_mark_node 
+	   || string_literal == error_mark_node 
+	   || expression == error_mark_node)
+        invalid_operands = true;
+
       /* Add this operand to the list.  */
       asm_operands = tree_cons (build_tree_list (name, string_literal),
 				expression,
@@ -15695,7 +15713,7 @@ cp_parser_asm_operand_list (cp_parser* p
       cp_lexer_consume_token (parser->lexer);
     }
 
-  return nreverse (asm_operands);
+  return invalid_operands ? error_mark_node : nreverse (asm_operands);
 }
 
 /* Parse an asm-clobber-list.

Re: [c++ patch] Fix PRs 30849/30850/30851

[Nathan Sidwell]

Lee Millward wrote:
> Hi Nathan,
> 
> On 7/22/07, Nathan Sidwell <nathan@codesourcery.com> wrote:
>> Why not just have cp_parser_asm_operand_list return error_mark_node in the face
>> of any bogus operand? (with suitable operand eating in that case)?
> 
> The attached patch implements this suggestion by modifying
> cp_parser_asm_operand_list to track whether any of the operands are
> invalid and to return error_mark_node in that case, adjusting the
> checks in cp_parser_asm_definition accordingly.
> 
> Bootstrapped and regression tested with no new failures on
> i686-pc-linux-gnu. Ok to apply?

ok thanks.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk