[committed,PR99680] Check empty constraint before using CONSTRAINT_LEN.

The following patch fixes

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99680

The patch was successfully bootstrapped on x86-64.

Hi!

On Sat, Mar 20, 2021 at 10:53:32AM -0400, Vladimir Makarov via Gcc-patches wrote:
> It seems CONSTRAINT_LEN treats constraint '\0' as one having length 1.

Maybe we should fix that?  See attached patch.


Segher


diff --git a/gcc/genpreds.c b/gcc/genpreds.c
index 8499a2a2383b..b83a030d6a5d 100644
--- a/gcc/genpreds.c
+++ b/gcc/genpreds.c
@@ -1141,6 +1141,9 @@ write_insn_constraint_len (void)
 	"  switch (fc)\n"
 	"    {");
 
+  /* Empty constraints have length 0.  */
+  printf ("    case 0: return 0;\n");
+
   for (i = 0; i < ARRAY_SIZE (constraints_by_letter_table); i++)
     {
       class constraint_data *c = constraints_by_letter_table[i];
@@ -1470,14 +1473,9 @@ write_tm_preds_h (void)
 			    address_start, address_end);
       write_allows_reg_mem_function ();
 
-      if (constraint_max_namelen > 1)
-        {
-	  write_insn_constraint_len ();
-	  puts ("#define CONSTRAINT_LEN(c_,s_) "
-		"insn_constraint_len (c_,s_)\n");
-	}
-      else
-	puts ("#define CONSTRAINT_LEN(c_,s_) 1\n");
+      write_insn_constraint_len ();
+      puts ("#define CONSTRAINT_LEN(c_,s_) insn_constraint_len (c_,s_)\n");
+
       if (have_register_constraints)
 	puts ("extern enum reg_class reg_class_for_constraint_1 "
 	      "(enum constraint_num);\n"

Segher Boessenkool <segher@kernel.crashing.org> writes:
> Hi!
>
> On Sat, Mar 20, 2021 at 10:53:32AM -0400, Vladimir Makarov via Gcc-patches wrote:
>> It seems CONSTRAINT_LEN treats constraint '\0' as one having length 1.
>
> Maybe we should fix that?  See attached patch.

I don't strongly object, but CONSTRAINT_LEN (0) doesn't feel to me like a
question we should be asking.  IMO it would be confusing to have, say:

  p += CONSTRAINT_LEN (*p);

“stick” at a null terminator even though the code is notionally
an increment.

'\0' is just a normal string null terminator and so I don't think we
should be processing it as if it were a constraint character.
How about having a gcc_unreachable on zero instead?

Thanks,
Richard

> Segher
>
>
> diff --git a/gcc/genpreds.c b/gcc/genpreds.c
> index 8499a2a2383b..b83a030d6a5d 100644
> --- a/gcc/genpreds.c
> +++ b/gcc/genpreds.c
> @@ -1141,6 +1141,9 @@ write_insn_constraint_len (void)
>  	"  switch (fc)\n"
>  	"    {");
>  
> +  /* Empty constraints have length 0.  */
> +  printf ("    case 0: return 0;\n");
> +
>    for (i = 0; i < ARRAY_SIZE (constraints_by_letter_table); i++)
>      {
>        class constraint_data *c = constraints_by_letter_table[i];
> @@ -1470,14 +1473,9 @@ write_tm_preds_h (void)
>  			    address_start, address_end);
>        write_allows_reg_mem_function ();
>  
> -      if (constraint_max_namelen > 1)
> -        {
> -	  write_insn_constraint_len ();
> -	  puts ("#define CONSTRAINT_LEN(c_,s_) "
> -		"insn_constraint_len (c_,s_)\n");
> -	}
> -      else
> -	puts ("#define CONSTRAINT_LEN(c_,s_) 1\n");
> +      write_insn_constraint_len ();
> +      puts ("#define CONSTRAINT_LEN(c_,s_) insn_constraint_len (c_,s_)\n");
> +
>        if (have_register_constraints)
>  	puts ("extern enum reg_class reg_class_for_constraint_1 "
>  	      "(enum constraint_num);\n"

On 2021-03-22 6:22 a.m., Richard Sandiford wrote:
> '\0' is just a normal string null terminator and so I don't think we
> should be processing it as if it were a constraint character.
> How about having a gcc_unreachable on zero instead?

I would be nice to use gcc_unreachable but it requires to rewrite some 
loops working on constraints.  All the loops check 0 explicitly to stop 
cycling, some of them still use CONSTRAIN_LEN for 0, e.g. code taken 
from reload.c:

switch ((c = *p, len = CONSTRAINT_LEN (c, p)), c)
{
   case '\0':
   len = 0;
   break;

...

I think we could wait stage 1 to do this.

On Mon, Mar 22, 2021 at 10:22:55AM +0000, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Sat, Mar 20, 2021 at 10:53:32AM -0400, Vladimir Makarov via Gcc-patches wrote:
> >> It seems CONSTRAINT_LEN treats constraint '\0' as one having length 1.
> >
> > Maybe we should fix that?  See attached patch.
> 
> I don't strongly object, but CONSTRAINT_LEN (0) doesn't feel to me like a
> question we should be asking.  IMO it would be confusing to have, say:
> 
>   p += CONSTRAINT_LEN (*p);

The CONSTRAINT_LEN macro has two arguments: the first character of the
constraint string, and a pointer to it.  That first character is just a
premature optimisation if you ask me (and it shouldn't be a macro at
all, it should use a function), but the important point is that the
pointer to the string is the important one.  So your example reads as
  p += CONSTRAINT_LEN (*p, p);

> “stick” at a null terminator even though the code is notionally
> an increment.

Then don't write code like that?

Errors should be handled *some* way, instead of nastiness like
      len = CONSTRAINT_LEN (c, constraint);
      do
        constraint++;
      while (--len && *constraint && *constraint != ',');
(from recog.c).  But since CONSTRAINT_LEN (0, "") should return
*something*, returning length 0 makes a lot of sense.  Many things do
not need to treat it different from how real constraints are handled,
but everything that wants can do so.

> '\0' is just a normal string null terminator and so I don't think we
> should be processing it as if it were a constraint character.

But we don't.  We process it as if it is the first character of the
remaining constraint string.  Which it is :-)

> How about having a gcc_unreachable on zero instead?

a) Then it should *definitely* not be a macro.
b) We can do better error handling than that.


Segher

On Mon, Mar 22, 2021 at 10:28:48AM -0400, Vladimir Makarov wrote:
> On 2021-03-22 6:22 a.m., Richard Sandiford wrote:
> >'\0' is just a normal string null terminator and so I don't think we
> >should be processing it as if it were a constraint character.
> >How about having a gcc_unreachable on zero instead?
> 
> I would be nice to use gcc_unreachable but it requires to rewrite some 
> loops working on constraints.  All the loops check 0 explicitly to stop 
> cycling, some of them still use CONSTRAIN_LEN for 0, e.g. code taken 
> from reload.c:
> 
> switch ((c = *p, len = CONSTRAINT_LEN (c, p)), c)
> {
>   case '\0':
>   len = 0;
>   break;
> 
> ...
> 
> I think we could wait stage 1 to do this.

Yes.  And such gcc_unreachable should be in as high-level code as
possible, not deep in some macro.


Segher