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

[Henry Wong <henry at stuffedcow dot net>]

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...

>From eb88d111b13dc5d71d500bfc7fd5ef97b1c601fa Mon Sep 17 00:00:00 2001
From: Henry Wong <henry@stuffedcow.net>
Date: Tue, 3 Oct 2017 00:41:50 -0400
Subject: [PATCH] nios2: Fix parsing of pseudo-instructions with missing
 arguments

---
 gas/config/tc-nios2.c                        | 18 +++++++++++
 gas/testsuite/gas/nios2/illegal_pseudoinst.l | 35 ++++++++++++++++++++++
 gas/testsuite/gas/nios2/illegal_pseudoinst.s | 45 ++++++++++++++++++++++++++++
 gas/testsuite/gas/nios2/nios2.exp            |  1 +
 4 files changed, 99 insertions(+)
 create mode 100644 gas/testsuite/gas/nios2/illegal_pseudoinst.l
 create mode 100644 gas/testsuite/gas/nios2/illegal_pseudoinst.s

diff --git a/gas/config/tc-nios2.c b/gas/config/tc-nios2.c
index 4ac3eaaf..24006f60 100644
--- a/gas/config/tc-nios2.c
+++ b/gas/config/tc-nios2.c
@@ -3157,6 +3157,15 @@ nios2_modify_arg (char **parsed_args, const char *modifier,
 		  int unused ATTRIBUTE_UNUSED, int ndx)
 {
   char *tmp = parsed_args[ndx];
+  int i;
+
+  /* Don't modify past end of argument list.  */
+  for (i=0;i<=ndx;i++)
+    if (parsed_args[i] == NULL)
+      {
+        parsed_args[ndx] = NULL;   /* Don't try to free parsed_args[ndx].  */
+        return;
+      }
 
   parsed_args[ndx] = concat (tmp, modifier, (char *) NULL);
 }
@@ -3167,6 +3176,15 @@ nios2_negate_arg (char **parsed_args, const char *modifier ATTRIBUTE_UNUSED,
 		  int unused ATTRIBUTE_UNUSED, int ndx)
 {
   char *tmp = parsed_args[ndx];
+  int i;
+
+  /* Don't modify past end of argument list.  */
+  for (i=0;i<=ndx;i++)
+    if (parsed_args[i] == NULL)
+      {
+        parsed_args[ndx] = NULL;   /* Don't try to free parsed_args[ndx].  */
+        return;
+      }
 
   parsed_args[ndx] = concat ("~(", tmp, ")+1", (char *) NULL);
 }
diff --git a/gas/testsuite/gas/nios2/illegal_pseudoinst.l b/gas/testsuite/gas/nios2/illegal_pseudoinst.l
new file mode 100644
index 00000000..7d4ffdff
--- /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 00000000..94b48cbe
--- /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 061d610c..e2cd332a 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" ""
-- 
2.13.5