This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] nios2: Fix parsing of pseudo-instructions
- From: Sandra Loosemore <sandra at codesourcery dot com>
- To: Henry Wong <henry at stuffedcow dot net>, <binutils at sourceware dot org>
- Date: Mon, 16 Oct 2017 21:54:21 -0600
- Subject: Re: [PATCH] nios2: Fix parsing of pseudo-instructions
- Authentication-results: sourceware.org; auth=none
- References: <ae0deb04-186c-3e63-b045-32692e1c2f95@stuffedcow.net> <59D2CAA7.6090703@codesourcery.com> <a62c8fe1-ea6a-d7cd-b308-787866d9cfce@stuffedcow.net>
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" ""