[PATCH 13/20] MIPS/GAS: Improve instruction's mnemonic processing

[PATCH 13/20] MIPS/GAS: Improve instruction's mnemonic processing

[Maciej W. Rozycki]

Hi Richard,

 Following your earlier comment addressing microMIPS code:

On Sun, 23 May 2010, Richard Sandiford wrote:

> Message-ID: <87y6fa9u3t.fsf@firetop.home>

> You could use alloca to create an opcode without the "16" or "32",
> which would make the error-reporting code simpler.  It's best not
> to change the user's source line if we can help it.

I have made a complementing adjustment to original code currently present 
in mips_ip(), making the whole piece much simpler and less fragile.

2010-12-02  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* config/tc-mips.c (mips_ip): Make a copy of the instruction's
	mnemonic and use it for further processing.

 OK to apply?

  Maciej

binutils-gas-mips-ip-insn-copy.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2010-12-02 01:48:16.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2010-12-02 01:49:28.000000000 +0000
@@ -8642,67 +8642,54 @@ mips_ip (char *str, struct mips_cl_insn 
   unsigned int lastpos = 0;
   unsigned int limlo, limhi;
   char *s_reset;
-  char save_c = 0;
   offsetT min_range, max_range;
+  char *name;
   int argnum;
   unsigned int rtype;
+  char *dot;
+  long end;
 
   insn_error = NULL;
 
+  insn = NULL;
+
   /* If the instruction contains a '.', we first try to match an instruction
      including the '.'.  Then we try again without the '.'.  */
-  insn = NULL;
-  for (s = str; *s != '\0' && !ISSPACE (*s); ++s)
+  for (end = 0; str[end] != '\0' && !ISSPACE (str[end]); end++)
     continue;
 
-  /* If we stopped on whitespace, then replace the whitespace with null for
-     the call to hash_find.  Save the character we replaced just in case we
-     have to re-parse the instruction.  */
-  if (ISSPACE (*s))
+  /* Make a copy of the instruction so that we can fiddle with it.  */
+  name = alloca (end + 1);
+  memcpy (name, str, end);
+  name[end] = '\0';
+
+  for (;;)
     {
-      save_c = *s;
-      *s++ = '\0';
-    }
+      insn = (struct mips_opcode *) hash_find (op_hash, name);
 
-  insn = (struct mips_opcode *) hash_find (op_hash, str);
+      if (insn != NULL)
+	break;
 
-  /* If we didn't find the instruction in the opcode table, try again, but
-     this time with just the instruction up to, but not including the
-     first '.'.  */
+      /* If we didn't find the instruction in the opcode table, try again,
+         but this time with just the instruction up to, but not including
+         the first '.'.  */
+      dot = strchr (name, '.');
+      if (dot == NULL)
+	break;
+      *dot = '\0';
+    }
   if (insn == NULL)
     {
-      /* Restore the character we overwrite above (if any).  */
-      if (save_c)
-	*(--s) = save_c;
-
-      /* Scan up to the first '.' or whitespace.  */
-      for (s = str;
-	   *s != '\0' && *s != '.' && !ISSPACE (*s);
-	   ++s)
-	continue;
-
-      /* If we did not find a '.', then we can quit now.  */
-      if (*s != '.')
-	{
-	  insn_error = _("Unrecognized opcode");
-	  return;
-	}
-
-      /* Lookup the instruction in the hash table.  */
-      *s++ = '\0';
-      if ((insn = (struct mips_opcode *) hash_find (op_hash, str)) == NULL)
-	{
-	  insn_error = _("Unrecognized opcode");
-	  return;
-	}
+      insn_error = _("Unrecognized opcode");
+      return;
     }
 
-  argsStart = s;
+  argsStart = s = str + end;
   for (;;)
     {
       bfd_boolean ok;
 
-      gas_assert (strcmp (insn->name, str) == 0);
+      gas_assert (strcmp (insn->name, name) == 0);
 
       ok = is_opcode_valid (insn);
       if (! ok)
@@ -8724,8 +8711,6 @@ mips_ip (char *str, struct mips_cl_insn 
 			   mips_cpu_info_from_isa (mips_opts.isa)->name);
 		  insn_error = buf;
 		}
-	      if (save_c)
-		*(--s) = save_c;
 	      return;
 	    }
 	}
@@ -10114,8 +10099,6 @@ mips_ip (char *str, struct mips_cl_insn 
 	  insn_error = _("Illegal operands");
 	  continue;
 	}
-      if (save_c)
-	*(--argsStart) = save_c;
       insn_error = _("Illegal operands");
       return;
     }

Re: [PATCH 13/20] MIPS/GAS: Improve instruction's mnemonic processing

[Richard Sandiford]

Patches 11 and 12 OK.

>> You could use alloca to create an opcode without the "16" or "32",
>> which would make the error-reporting code simpler.  It's best not
>> to change the user's source line if we can help it.
>
> I have made a complementing adjustment to original code currently present 
> in mips_ip(), making the whole piece much simpler and less fragile.
>
> 2010-12-02  Maciej W. Rozycki  <macro@codesourcery.com>
>
> 	gas/
> 	* config/tc-mips.c (mips_ip): Make a copy of the instruction's
> 	mnemonic and use it for further processing.

This patch doesn't preserve the current behaviour.

The current code treats the text after the "." as the first thing that
should be matched against the format string.  I assume it dates back to
a time when certain types of operand suffix were handled using format
characters.  (Maybe the floating-point condition codes and formats?)
This meant that things like:

	addu.$4,$5,$6

were also acceptable, although of course:

	c.eq.d.$f0,$f2

wasn't.

The patch instead ignores everything after the ".", which means that
we'd accept stuff like:

	addu.foobar $4,$5,$7

(although again not "c.eq.d.foobar").

I think we can simply remove the dot check.  I don't see any testsuite
regressions after doing that.

Richard

Re: [PATCH 13/20] MIPS/GAS: Improve instruction's mnemonic processing

[Maciej W. Rozycki]

On Tue, 7 Dec 2010, Richard Sandiford wrote:

> > 	* config/tc-mips.c (mips_ip): Make a copy of the instruction's
> > 	mnemonic and use it for further processing.
> 
> This patch doesn't preserve the current behaviour.

 Ouch, I missed this subtlety, sorry. :(

> The current code treats the text after the "." as the first thing that
> should be matched against the format string.  I assume it dates back to
> a time when certain types of operand suffix were handled using format
> characters.  (Maybe the floating-point condition codes and formats?)
> This meant that things like:
> 
> 	addu.$4,$5,$6
> 
> were also acceptable, although of course:
> 
> 	c.eq.d.$f0,$f2
> 
> wasn't.

 I did some archaeology and tracked down the introduction of this feature 
to have happened between binutils 2.8 and 2.9.  I checked the opcode table 
differences between the two versions and found no modifications that would 
justify the change.  I looked at the relevant ChangeLog entries and there 
is nothing about this change.

 My only thought of any use for this stuff might be ITBL or some 
unfinished work that has crept in by accident.  The latter hypothesis is 
further backed up by the lack of a corresponding ChangeLog entry.  Either 
way less than useful it would seem.

> The patch instead ignores everything after the ".", which means that
> we'd accept stuff like:
> 
> 	addu.foobar $4,$5,$7
> 
> (although again not "c.eq.d.foobar").
> 
> I think we can simply remove the dot check.  I don't see any testsuite
> regressions after doing that.

 Yes, I agree.  Here's a new version.  No regressions for mips-sde-elf or 
mips-linux-gnu.

2010-12-10  Maciej W. Rozycki  <macro@codesourcery.com>

	* config/tc-mips.c (mips_ip): Make a copy of the instruction's
	mnemonic and use it for further processing.

 OK?

  Maciej

binutils-20101210-gas-mips-ip-insn-copy.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2010-12-10 18:17:43.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2010-12-10 18:17:43.000000000 +0000
@@ -8655,67 +8655,38 @@ mips_ip (char *str, struct mips_cl_insn 
   unsigned int lastpos = 0;
   unsigned int limlo, limhi;
   char *s_reset;
-  char save_c = 0;
   offsetT min_range, max_range;
+  char *name;
   int argnum;
   unsigned int rtype;
+  long end;
 
   insn_error = NULL;
 
-  /* If the instruction contains a '.', we first try to match an instruction
-     including the '.'.  Then we try again without the '.'.  */
   insn = NULL;
-  for (s = str; *s != '\0' && !ISSPACE (*s); ++s)
-    continue;
 
-  /* If we stopped on whitespace, then replace the whitespace with null for
-     the call to hash_find.  Save the character we replaced just in case we
-     have to re-parse the instruction.  */
-  if (ISSPACE (*s))
-    {
-      save_c = *s;
-      *s++ = '\0';
-    }
+  /* Try to match an instruction up to a space or to the end.  */
+  for (end = 0; str[end] != '\0' && !ISSPACE (str[end]); end++)
+    continue;
 
-  insn = (struct mips_opcode *) hash_find (op_hash, str);
+  /* Make a copy of the instruction so that we can fiddle with it.  */
+  name = alloca (end + 1);
+  memcpy (name, str, end);
+  name[end] = '\0';
 
-  /* If we didn't find the instruction in the opcode table, try again, but
-     this time with just the instruction up to, but not including the
-     first '.'.  */
+  insn = (struct mips_opcode *) hash_find (op_hash, name);
   if (insn == NULL)
     {
-      /* Restore the character we overwrite above (if any).  */
-      if (save_c)
-	*(--s) = save_c;
-
-      /* Scan up to the first '.' or whitespace.  */
-      for (s = str;
-	   *s != '\0' && *s != '.' && !ISSPACE (*s);
-	   ++s)
-	continue;
-
-      /* If we did not find a '.', then we can quit now.  */
-      if (*s != '.')
-	{
-	  insn_error = _("Unrecognized opcode");
-	  return;
-	}
-
-      /* Lookup the instruction in the hash table.  */
-      *s++ = '\0';
-      if ((insn = (struct mips_opcode *) hash_find (op_hash, str)) == NULL)
-	{
-	  insn_error = _("Unrecognized opcode");
-	  return;
-	}
+      insn_error = _("Unrecognized opcode");
+      return;
     }
 
-  argsStart = s;
+  argsStart = s = str + end;
   for (;;)
     {
       bfd_boolean ok;
 
-      gas_assert (strcmp (insn->name, str) == 0);
+      gas_assert (strcmp (insn->name, name) == 0);
 
       ok = is_opcode_valid (insn);
       if (! ok)
@@ -8737,8 +8708,6 @@ mips_ip (char *str, struct mips_cl_insn 
 			   mips_cpu_info_from_isa (mips_opts.isa)->name);
 		  insn_error = buf;
 		}
-	      if (save_c)
-		*(--s) = save_c;
 	      return;
 	    }
 	}
@@ -10121,8 +10090,6 @@ mips_ip (char *str, struct mips_cl_insn 
 	  insn_error = _("Illegal operands");
 	  continue;
 	}
-      if (save_c)
-	*(--argsStart) = save_c;
       insn_error = _("Illegal operands");
       return;
     }

Re: [PATCH 13/20] MIPS/GAS: Improve instruction's mnemonic processing

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> 2010-12-10  Maciej W. Rozycki  <macro@codesourcery.com>
>
> 	* config/tc-mips.c (mips_ip): Make a copy of the instruction's
> 	mnemonic and use it for further processing.

OK as part of the microMIPS patch.

(In case you're wondering why not just "OK": there's nothing wrong with
temporarily replacing whitespace with nulls, so without the microMIPS
changes, that's what we'd do here.  The thing I objected to with the
microMIPS patch was editing the mnemonic in-place, by removing characters.
That's what you're fixing with the combination of this patch and the
updated microMIPS one, so the two patches are OK together.)

Richard

Re: [PATCH 13/20] MIPS/GAS: Improve instruction's mnemonic processing

[Maciej W. Rozycki]

On Sat, 11 Dec 2010, Richard Sandiford wrote:

> > 	* config/tc-mips.c (mips_ip): Make a copy of the instruction's
> > 	mnemonic and use it for further processing.
> 
> OK as part of the microMIPS patch.

 That's not going to be a problem -- `quilt' makes such patch shuffling, 
folding, etc. quite easy. :)

> (In case you're wondering why not just "OK": there's nothing wrong with
> temporarily replacing whitespace with nulls, so without the microMIPS
> changes, that's what we'd do here.  The thing I objected to with the
> microMIPS patch was editing the mnemonic in-place, by removing characters.
> That's what you're fixing with the combination of this patch and the
> updated microMIPS one, so the two patches are OK together.)

 I find it a bit hackish, especially as code has to be written carefully 
to replace the original character at return points as needed (currently 
three of the seven do so; the exact requirement pattern seems unobvious to 
me -- is it any clearer to you?), but not particularly incorrect.

  Maciej