Re: [PATCH] Fix another CONSTRAINT_LEN related bug (PR middle-end/84831)

[Richard Biener <rguenther at suse dot de>]

On Mon, 12 Mar 2018, Jakub Jelinek wrote:

> Hi!
> 
> I thought the vregs pass is the first one to analyze constraints,
> but I was wrong, already before that parse_{output,input}_constraint
> are called, already during GIMPLE passes.  They don't really fail miserably
> if , appears in the middle of multi-letter constraint (unlike e.g. the RA),
> which is what my previous patch was fixing, but care if '\0' appears
> in the middle of multi-letter constraint, because then we continue walking
> random characters after the constraint string.  parse_input_constraint
> is actually fine, because it has c_len = strlen (constraint); and uses
> j < c_len, but parse_output_constraint doesn't do that.  While doing it
> is possible, I think it is needlessly slow (needs to walk the constraint
> twice), so this patch instead makes sure there are no '\0's in the middle
> of constraints, if there are, doesn't jump over it.
> 
> The patch is larger because of reindentation, here is diff -upbd for
> easier understanding what has changed.
> 
> --- gcc/stmt.c.jj	2018-01-03 10:19:55.150533956 +0100
> +++ gcc/stmt.c	2018-03-12 13:25:03.790733765 +0100
> @@ -247,7 +247,8 @@ parse_output_constraint (const char **co
>      }
>  
>    /* Loop through the constraint string.  */
> -  for (p = constraint + 1; *p; p += CONSTRAINT_LEN (*p, p))
> +  for (p = constraint + 1; *p; )
> +    {
>      switch (*p)
>        {
>        case '+':
> @@ -304,6 +305,11 @@ parse_output_constraint (const char **co
>  	break;
>        }
>  
> +      for (size_t len = CONSTRAINT_LEN (*p, p); len; len--, p++)
> +	if (*p == '\0')
> +	  break;
> +    }
> +
>    return true;
>  }
>  
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Richard.

> 2018-03-12  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/84831
> 	* stmt.c (parse_output_constraint): If the CONSTRAINT_LEN (*p, p)
> 	characters starting at p contain '\0' character, don't look beyond
> 	that.
> 
> --- gcc/stmt.c.jj	2018-01-03 10:19:55.150533956 +0100
> +++ gcc/stmt.c	2018-03-12 13:25:03.790733765 +0100
> @@ -247,62 +247,68 @@ parse_output_constraint (const char **co
>      }
>  
>    /* Loop through the constraint string.  */
> -  for (p = constraint + 1; *p; p += CONSTRAINT_LEN (*p, p))
> -    switch (*p)
> -      {
> -      case '+':
> -      case '=':
> -	error ("operand constraint contains incorrectly positioned "
> -	       "%<+%> or %<=%>");
> -	return false;
> -
> -      case '%':
> -	if (operand_num + 1 == ninputs + noutputs)
> -	  {
> -	    error ("%<%%%> constraint used with last operand");
> -	    return false;
> -	  }
> -	break;
> -
> -      case '?':  case '!':  case '*':  case '&':  case '#':
> -      case '$':  case '^':
> -      case 'E':  case 'F':  case 'G':  case 'H':
> -      case 's':  case 'i':  case 'n':
> -      case 'I':  case 'J':  case 'K':  case 'L':  case 'M':
> -      case 'N':  case 'O':  case 'P':  case ',':
> -	break;
> -
> -      case '0':  case '1':  case '2':  case '3':  case '4':
> -      case '5':  case '6':  case '7':  case '8':  case '9':
> -      case '[':
> -	error ("matching constraint not valid in output operand");
> -	return false;
> -
> -      case '<':  case '>':
> -	/* ??? Before flow, auto inc/dec insns are not supposed to exist,
> -	   excepting those that expand_call created.  So match memory
> -	   and hope.  */
> -	*allows_mem = true;
> -	break;
> -
> -      case 'g':  case 'X':
> -	*allows_reg = true;
> -	*allows_mem = true;
> -	break;
> -
> -      default:
> -	if (!ISALPHA (*p))
> -	  break;
> -	enum constraint_num cn = lookup_constraint (p);
> -	if (reg_class_for_constraint (cn) != NO_REGS
> -	    || insn_extra_address_constraint (cn))
> +  for (p = constraint + 1; *p; )
> +    {
> +      switch (*p)
> +	{
> +	case '+':
> +	case '=':
> +	  error ("operand constraint contains incorrectly positioned "
> +		 "%<+%> or %<=%>");
> +	  return false;
> +
> +	case '%':
> +	  if (operand_num + 1 == ninputs + noutputs)
> +	    {
> +	      error ("%<%%%> constraint used with last operand");
> +	      return false;
> +	    }
> +	  break;
> +
> +	case '?':  case '!':  case '*':  case '&':  case '#':
> +	case '$':  case '^':
> +	case 'E':  case 'F':  case 'G':  case 'H':
> +	case 's':  case 'i':  case 'n':
> +	case 'I':  case 'J':  case 'K':  case 'L':  case 'M':
> +	case 'N':  case 'O':  case 'P':  case ',':
> +	  break;
> +
> +	case '0':  case '1':  case '2':  case '3':  case '4':
> +	case '5':  case '6':  case '7':  case '8':  case '9':
> +	case '[':
> +	  error ("matching constraint not valid in output operand");
> +	  return false;
> +
> +	case '<':  case '>':
> +	  /* ??? Before flow, auto inc/dec insns are not supposed to exist,
> +	     excepting those that expand_call created.  So match memory
> +	     and hope.  */
> +	  *allows_mem = true;
> +	  break;
> +
> +	case 'g':  case 'X':
>  	  *allows_reg = true;
> -	else if (insn_extra_memory_constraint (cn))
>  	  *allows_mem = true;
> -	else
> -	  insn_extra_constraint_allows_reg_mem (cn, allows_reg, allows_mem);
> -	break;
> -      }
> +	  break;
> +
> +	default:
> +	  if (!ISALPHA (*p))
> +	    break;
> +	  enum constraint_num cn = lookup_constraint (p);
> +	  if (reg_class_for_constraint (cn) != NO_REGS
> +	      || insn_extra_address_constraint (cn))
> +	    *allows_reg = true;
> +	  else if (insn_extra_memory_constraint (cn))
> +	    *allows_mem = true;
> +	  else
> +	    insn_extra_constraint_allows_reg_mem (cn, allows_reg, allows_mem);
> +	  break;
> +	}
> +
> +      for (size_t len = CONSTRAINT_LEN (*p, p); len; len--, p++)
> +	if (*p == '\0')
> +	  break;
> +    }
>  
>    return true;
>  }
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)