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

[Henry Wong <henry at stuffedcow dot net>]

On 10/16/2017 11:54 PM, Sandra Loosemore wrote:
> 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
>
Ah, I see. Looks good, thanks! :)