[PATCH] ARM: gas not detecting invalid operands for Thumb2 ADD{S} and SUB{S}

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] ARM: gas not detecting invalid operands for Thumb2 ADD{S} and SUB{S}

Paul Carroll-3
(I don't have write permission into the Binutils CVS, so someone else
will be merging the final patch.)

In ARMv6T2 and ARMv7 Thumb2, the ADD, ADDS, SUB, and SUBS instructions
added several new instruction forms. One of the new forms allowed is:

    ADD{S}<c>.W <Rd>,SP,<Rm>{,<shift>}
    SUB{S}<c>.W <Rd>,SP,<Rm>{,<shift>}

According to DDI-0406B page A8-30

    d = UInt(Rd); m = UInt(Rm); setflags = (S == '1');
    (shift_t, shift_n) = DecodeImmShift(type, imm3:imm2);
    if d == 13&&   (shift_t != SRType_LSL || shift_n>   3) then
UNPREDICTABLE;
    if d == 15 || BadReg(m) then UNPREDICTABLE;

The tc-arm.c file in the gas/config directory was already detecting the
'd==15' condition. But, there was no validation of the shift type or
shift value when the first register specified was SP.
This patch adds that check.

The patch could be reorganized a little, to move the added constraints
out of
encode_thumb32_shifted_operand() and into do_t_add_sub().  That would
get rid of
passing an argument into encode_thumb32_shifted_operand() but add some
minimal duplicate
code into do_t_add_sub().  Either would produce identical behavior.
I will leave it to others as to whether the patch should be reorganized.

I also have 3 new test cases for the gas/arm test suite - add,
addthumb2, and addthumb2err. The 'add' and 'addthumb2' test cases
represented valid versions of ADD, ADDS, SUB, and SUBS. The
'addthumb2err' test case represents operand combinations that should
have generated an error. These invalid tests use SP for the first 2
operands and r0 as the 3rd operand. The LSR, ASR, ROR, and RRX shift
types are used with valid shift values, which is not legal. Then the LSL
shift type is used with a shift value of 4, which is also not legal.

2011-02-25  Paul Carroll <[hidden email]>

         gas/
         * config/tc-arm.c (encode_thumb32_shifted_operand): Added
         constraints for shift type and value for Thumb2 ADD{S} and
         SUB{S} instructions.
         (do_t_add_sub): Adding argument to encode calls to indicate if
         first 2 operands are both SP.
         (do_t_arit3, do_t_arit3c, do_t_mov_cmp, do_t_mvn_tst, do_t_orn)
         (do_t_rsb, do_t_shift): Adding FALSE argument to encode calls
         since that argument is not used by them.

         gas/testsuite/
         * gas/arm/add.s: new test, allowable ADD{S} and SUB{S} insns.
         * gas/arm/add.d: new result file.
         * gas/arm/addthumb32.s: new test, allowable ADD{S} and SUB{S}
insns.
         * gas/arm/addthumb32.d: new result file.
         * gas/arm/addthumb32err.s: new test, bad ADD{S} and SUB{S} insns.
         * gas/arm/addthumb32err.d: new result file.
         * gas/arm/addthumb32err.l: new error file.

ARM_ADD.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ARM: gas not detecting invalid operands for Thumb2 ADD{S} and SUB{S}

Paul Brook
> In ARMv6T2 and ARMv7 Thumb2, the ADD, ADDS, SUB, and SUBS instructions
> added several new instruction forms. One of the new forms allowed is:
>
>     ADD{S}<c>.W <Rd>,SP,<Rm>{,<shift>}
>     SUB{S}<c>.W <Rd>,SP,<Rm>{,<shift>}

The "added new forms" is mainly an artifact of how the new documentation is
structured.  If you're starting from the ARM instruction set (or from the
encodings) then it's actually new restrictions on where r13 (aka SP) may be
used.

The patch looks ok, though I think the testcases could use some
reorganisation.  I'd prefer two assembly files, one with insns valid in both
ARM and Thumb mode, the other which is only valid in ARM mode.  Assemble both
in both modes.  There should already be existing tests
(e.g. sp-pc-usage-t) that cover the former.

> +@ test case of ADD{S} and SUB{S} instructions in ARM mode

Too vague.  You're testing use of SP in these insns.

> + .file "s6163c.c"

Looks bogus.

Paul
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ARM: gas not detecting invalid operands for Thumb2 ADD{S} and SUB{S}

Paul Carroll-3
On 3/11/2011 6:26 AM, Paul Brook wrote:
> The patch looks ok, though I think the testcases could use some
> reorganisation.  I'd prefer two assembly files, one with insns valid in both
> ARM and Thumb mode, the other which is only valid in ARM mode.  Assemble both
> in both modes.  There should already be existing tests
> (e.g. sp-pc-usage-t) that cover the former.

Maybe the best thing to do is just remove the 2 positive test cases and
keep only the 1 negative test case.
I'll just assume the test suite does enough validation with existing tests.

>> +@ test case of ADD{S} and SUB{S} instructions in ARM mode
> Too vague.  You're testing use of SP in these insns.

For the negative test case, I can adjust the comment to this:
     # Test of invalid operands for ADD{S} and SUB{S} instructions
     # in Thumb2 mode.  The instruction form being testing
     # involves having the first 2 operands be SP.

>> + .file "s6163c.c"
> Looks bogus.

That can easily be removed from the negative test case.