Re: [PATCH] nios2: Fix parsing of pseudo-instructions

[Sandra Loosemore <sandra at codesourcery dot com>]

On 10/02/2017 10:57 PM, Henry Wong wrote:
> On 10/02/2017 07:24 PM, Sandra Loosemore wrote:
> > On 09/27/2017 05:18 PM, Henry Wong wrote:
> >
> > > Changes:
> > >
> > > * config/tc-nios2.c (nios2_modify_arg, nios2_negate_arg): Fix segfault
> > > when parsing Nios II pseudo-instructions that have missing arguments
> > > * testsuite/gas/nios2/illegal_pseudoinst.s: New test for illegal Nios II
> > > pseudo-instructions
> > > * testsuite/gas/nios2/illegal_pseudoinst.l: stderr output
> > > * testsuite/gas/nios2/nios2.exp: Changed so the above new test is run.
> >
> > Hmmm.  I think it would be better to put a more general check for the
> > right number of arguments in nios2_translate_pseudo_insn; add a field
> > to nios2_ps_insn_infoS to hold the expected number of arguments for
> > the pseudo-op, and check it before dispatching to arg_modifier_func.
> >
> > Also, your patch doesn't follow GNU coding standards with regards to
> > whitespace and indentation.
> >
> > Do you want to submit a revised patch, or have me handle the fix?
> >
> > -Sandra
>
> I had considered a slightly more general check as you suggested, but I'm
> not really convinced that it's the best option.
>
> Adding a field to nios2_ps_insn_infoS to hold the number of expected
> arguments would add the cost of yet another field that needs maintaining.
>
> As for moving the number-of-arguments check into
> nios2_translate_pseudo_insn: nios2_translate_pseudo_insn() actually has
> no knowledge of whether this check is necessary or sufficient for a
> particular argument transformation. Currently, this check is necessary
> for only two of the transformations (modify_arg and negate_arg). Doing a
> general check would require a new field in the nios2_ps_insn_infoS table
> (can't use the index field because the interpretation of the index field
> depends on which transformation is being done).
>
> It feels cleaner to say "The function that does the transformation must
> ensure the transformation can be safely done" (new code in two places),
> rather than do the argument-count check in a central location (new code
> in one place), yet comparing to a constant that is different for each
> opcode, yet not removing the responsibility of each transformation
> function to do any *other* checks needed for safety (or if paranoid,
> repeat the same check using the 'index' field). e.g., I see a few
> existing asserts comparing against NIOS2_MAX_INSN_TOKENS.
>
> I'm not sure I'd trade [new code in two places] for [new code in one
> place plus 20 constants and responsibility for safety checks split into
> two places].
>
> Thoughts?
>
>
> Attached: Let's see if I got the indentation styles right this time.
>
> The one thing I'm not entirely comfortable with in my current patch is
> that a failure is indicated by setting parsed_args[ndx] to NULL so that
> nios2_free_arg() won't attempt to free it. However, it seems mostly safe
> to do, as the transformation function knows exactly with cleanup
> function it's paired with and knows how to avoid a cleanup attempt.
> Another option that touches more code is to get all of the
> transformation functions to return success/fail and pass that all the
> way up to md_assemble (possibly by getting nios2_translate_pseudo_insn()
> to return NULL) so it knows whether to attempt calling
> nios2_cleanup_pseudo_insn...

I've checked in the attached version of the patch.  When I took another 
look at this, I realized that the number of expected arguments for each 
pseudo-insn is already encoded in the main instruction tables, so 
there's no need to extend the pseudo-instruction tables.  The code that 
does the check is copied from the place that handles ordinary 
instructions.  The messiest part was handling the control flow in the 
error case (most of that patch hunk is just re-indenting in a conditional).

Thanks again for identifying the bug and providing the test case.   :-)

-Sandra

2017-10-16  Sandra Loosemore  <sandra@codesourcery.com>
	    Henry Wong  <henry@stuffedcow.net>

	gas/
	* config/tc-nios2.c (nios2_translate_pseudo_insn): Check for
	correct number of arguments.
	(md_assemble): Handle failure of nios2_translate_pseudo_insn.
	* testsuite/gas/nios2/illegal_pseudoinst.l: New file.
	* testsuite/gas/nios2/illegal_pseudoinst.s: New file.
	* testsuite/gas/nios2/nios2.exp: Add illegal_pseudoinst test.

diff --git a/gas/config/tc-nios2.c b/gas/config/tc-nios2.c
index 4ac3eaa..372d550 100644
--- a/gas/config/tc-nios2.c
+++ b/gas/config/tc-nios2.c
@@ -3244,16 +3244,29 @@ static nios2_ps_insn_infoS*
 nios2_translate_pseudo_insn (nios2_insn_infoS *insn)
 {
 
+  const struct nios2_opcode *op = insn->insn_nios2_opcode;
   nios2_ps_insn_infoS *ps_insn;
+  unsigned int tokidx, ntok;
 
   /* Find which real insn the pseudo-op translates to and
      switch the insn_info ptr to point to it.  */
-  ps_insn = nios2_ps_lookup (insn->insn_nios2_opcode->name);
+  ps_insn = nios2_ps_lookup (op->name);
 
   if (ps_insn != NULL)
     {
       insn->insn_nios2_opcode = nios2_opcode_lookup (ps_insn->insn);
       insn->insn_tokens[0] = insn->insn_nios2_opcode->name;
+      
+      /* Make sure there are enough arguments.  */
+      ntok = ((op->pinfo & NIOS2_INSN_OPTARG)
+	      ? op->num_args - 1 : op->num_args);
+      for (tokidx = 1; tokidx <= ntok; tokidx++)
+	if (insn->insn_tokens[tokidx] == NULL)
+	  {
+	    as_bad ("missing argument");
+	    return NULL;
+	  }
+
       /* Modify the args so they work with the real insn.  */
       ps_insn->arg_modifer_func ((char **) insn->insn_tokens,
 				 ps_insn->arg_modifier, ps_insn->num,
@@ -3684,6 +3697,7 @@ md_assemble (char *op_str)
   unsigned long saved_pinfo = 0;
   nios2_insn_infoS thisinsn;
   nios2_insn_infoS *insn = &thisinsn;
+  bfd_boolean ps_error = FALSE;
 
   /* Make sure we are aligned on an appropriate boundary.  */
   if (nios2_current_align < nios2_min_align)
@@ -3730,35 +3744,45 @@ md_assemble (char *op_str)
 	 with its real equivalent, and then continue.  */
       if ((insn->insn_nios2_opcode->pinfo & NIOS2_INSN_MACRO)
 	  == NIOS2_INSN_MACRO)
-	ps_insn = nios2_translate_pseudo_insn (insn);
-
-      /* Assemble the parsed arguments into the instruction word.  */
-      nios2_assemble_args (insn);
-
-      /* Handle relaxation and other transformations.  */
-      if (nios2_as_options.relax != relax_none
-	  && !nios2_as_options.noat
-	  && insn->insn_nios2_opcode->pinfo & NIOS2_INSN_UBRANCH)
-	output_ubranch (insn);
-      else if (nios2_as_options.relax != relax_none
-	       && !nios2_as_options.noat
-	       && insn->insn_nios2_opcode->pinfo & NIOS2_INSN_CBRANCH)
-	output_cbranch (insn);
-      else if (nios2_as_options.relax == relax_all
-	       && !nios2_as_options.noat
-	       && insn->insn_nios2_opcode->pinfo & NIOS2_INSN_CALL
-	       && insn->insn_reloc
-	       && ((insn->insn_reloc->reloc_type
-		    == BFD_RELOC_NIOS2_CALL26)
-		   || (insn->insn_reloc->reloc_type
-		       == BFD_RELOC_NIOS2_CALL26_NOAT)))
-	output_call (insn);
-      else if (saved_pinfo == NIOS2_INSN_MACRO_MOVIA)
-	output_movia (insn);
-      else
-	output_insn (insn);
-      if (ps_insn)
-	nios2_cleanup_pseudo_insn (insn, ps_insn);
+	{
+	  ps_insn = nios2_translate_pseudo_insn (insn);
+	  if (!ps_insn)
+	    ps_error = TRUE;
+	}
+
+      /* If we found invalid pseudo-instruction syntax, the error's already
+	 been diagnosed in nios2_translate_pseudo_insn, so skip
+	 remaining processing.  */
+      if (!ps_error)
+	{
+	  /* Assemble the parsed arguments into the instruction word.  */
+	  nios2_assemble_args (insn);
+
+	  /* Handle relaxation and other transformations.  */
+	  if (nios2_as_options.relax != relax_none
+	      && !nios2_as_options.noat
+	      && insn->insn_nios2_opcode->pinfo & NIOS2_INSN_UBRANCH)
+	    output_ubranch (insn);
+	  else if (nios2_as_options.relax != relax_none
+		   && !nios2_as_options.noat
+		   && insn->insn_nios2_opcode->pinfo & NIOS2_INSN_CBRANCH)
+	    output_cbranch (insn);
+	  else if (nios2_as_options.relax == relax_all
+		   && !nios2_as_options.noat
+		   && insn->insn_nios2_opcode->pinfo & NIOS2_INSN_CALL
+		   && insn->insn_reloc
+		   && ((insn->insn_reloc->reloc_type
+			== BFD_RELOC_NIOS2_CALL26)
+		       || (insn->insn_reloc->reloc_type
+			   == BFD_RELOC_NIOS2_CALL26_NOAT)))
+	    output_call (insn);
+	  else if (saved_pinfo == NIOS2_INSN_MACRO_MOVIA)
+	    output_movia (insn);
+	  else
+	    output_insn (insn);
+	  if (ps_insn)
+	    nios2_cleanup_pseudo_insn (insn, ps_insn);
+	}
     }
   else
     /* Unrecognised instruction - error.  */
diff --git a/gas/testsuite/gas/nios2/illegal_pseudoinst.l b/gas/testsuite/gas/nios2/illegal_pseudoinst.l
new file mode 100644
index 0000000..7d4ffdf
--- /dev/null
+++ b/gas/testsuite/gas/nios2/illegal_pseudoinst.l
@@ -0,0 +1,35 @@
+.*illegal_pseudoinst.s: Assembler messages:
+.*illegal_pseudoinst.s:5: Error: missing argument
+.*illegal_pseudoinst.s:6: Error: expecting , near r2
+.*illegal_pseudoinst.s:6: Error: missing argument
+.*illegal_pseudoinst.s:7: Error: missing argument
+.*illegal_pseudoinst.s:8: Error: expecting , near r2
+.*illegal_pseudoinst.s:8: Error: missing argument
+.*illegal_pseudoinst.s:9: Error: missing argument
+.*illegal_pseudoinst.s:10: Error: missing argument
+.*illegal_pseudoinst.s:11: Error: missing argument
+.*illegal_pseudoinst.s:14: Error: missing argument
+.*illegal_pseudoinst.s:15: Error: missing argument
+.*illegal_pseudoinst.s:16: Error: expecting , near r2
+.*illegal_pseudoinst.s:16: Error: missing argument
+.*illegal_pseudoinst.s:17: Error: missing argument
+.*illegal_pseudoinst.s:18: Error: missing argument
+.*illegal_pseudoinst.s:19: Error: missing argument
+.*illegal_pseudoinst.s:22: Error: missing argument
+.*illegal_pseudoinst.s:23: Error: missing argument
+.*illegal_pseudoinst.s:24: Error: missing argument
+.*illegal_pseudoinst.s:25: Error: missing argument
+.*illegal_pseudoinst.s:26: Error: missing argument
+.*illegal_pseudoinst.s:27: Error: missing argument
+.*illegal_pseudoinst.s:28: Error: missing argument
+.*illegal_pseudoinst.s:29: Error: missing argument
+.*illegal_pseudoinst.s:30: Error: missing argument
+.*illegal_pseudoinst.s:31: Error: missing argument
+.*illegal_pseudoinst.s:34: Error: missing argument
+.*illegal_pseudoinst.s:35: Error: missing argument
+.*illegal_pseudoinst.s:36: Error: unknown register 
+.*illegal_pseudoinst.s:37: Error: missing argument
+.*illegal_pseudoinst.s:38: Error: missing argument
+.*illegal_pseudoinst.s:41: Error: missing argument
+.*illegal_pseudoinst.s:42: Error: missing argument
+.*illegal_pseudoinst.s:43: Error: missing argument
diff --git a/gas/testsuite/gas/nios2/illegal_pseudoinst.s b/gas/testsuite/gas/nios2/illegal_pseudoinst.s
new file mode 100644
index 0000000..94b48cb
--- /dev/null
+++ b/gas/testsuite/gas/nios2/illegal_pseudoinst.s
@@ -0,0 +1,45 @@
+# Source file used to test missing (and illegal) operands for pseudo-instructions.
+
+foo:
+# nios2_modify_arg
+	cmpgti r2, r3,
+	cmpgtui r2, r2
+	cmplei r2, r3,
+	cmpleui r2, r2
+	cmpgti ,,
+	cmplei ,
+	cmpleui
+
+# nios2_negate_arg
+	subi Lorem ipsum dolor sit amet, consectetur adipiscing elit,
+	subi r2, r2,
+	subi r2, r2
+	subi ,,
+	subi ,
+	subi
+
+# nios2_swap_args
+	bgt r0, r2,
+	bgtu ,,
+	ble , r0,
+	bleu foo,,
+	cmpgt r2, r3,
+	cmpgtu r2,,
+	cmple , r3,
+	cmpleu ,,
+	bgtu ,
+	ble
+
+# nios2_insert_arg
+	movi  ,
+	movhi r0,
+	movui ,r2
+	movia ,
+	movi
+
+# nios2_append_arg
+	mov r0,
+	mov ,
+	mov
+
+
diff --git a/gas/testsuite/gas/nios2/nios2.exp b/gas/testsuite/gas/nios2/nios2.exp
index 061d610..e2cd332 100644
--- a/gas/testsuite/gas/nios2/nios2.exp
+++ b/gas/testsuite/gas/nios2/nios2.exp
@@ -22,6 +22,7 @@ if { [istarget nios2-*-*] } {
     run_dump_tests [lsort [glob -nocomplain $srcdir/$subdir/*.d]]
     
     run_list_test "illegal" ""
+    run_list_test "illegal_pseudoinst" ""
     run_list_test "warn_nobreak" ""
     run_list_test "warn_noat" ""
     run_list_test "movi" ""