[PATCH] nios2: Fix parsing of pseudo-instructions

[Henry Wong]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-nios2-Fix-parsing-of-pseudo-instructions-with-missin.patch
Type: text/x-patch
Size: 5165 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20171003/54473a4f/attachment.bin>