[PATCH] arm: correct barrier immediate checks
Richard Earnshaw
rearnsha@arm.com
Mon Apr 8 10:29:00 GMT 2013
On 08/04/13 10:25, Jan Beulich wrote:
> Both do_barrier() and do_t_barrier() used && instead of || in a constraint()
> invocation. While fixing this, I also noticed that the mask used in the first
> part of the condition was too strict - according to the specification, only
> 2 bits should really be looked at.
>
> gas/
> 2013-04-08 Jan Beulich <jbeulich@suse.com>
>
> * gas/config/tc-arm.c (do_barrier): Correct constraint()
> argument.
> (do_t_barrier): Likewise.
>
Thanks for the patch. I agree that something is wrong here, but I don't
think this is quite right. However, I've been struggling to understand
what the code is trying to do in the first place.
There shouldn't be a need for a simple test that part of the instruction
is valid, that would be pointless -- it can't happen via a user error
and internal errors should be handled by assertions when necessary, not
user errors.
Looking at the patch history gives a bit of insight, before the latest
change to this code, the test looked like:
constraint ((inst.instruction & 0xf0) != 0x40
&& inst.operands[0].imm != 0xf,
_("bad barrier type"));
Which makes a bit more sense -- it's saying if it's not a particular
type of barrier and the option field is not 0xf, then something is
wrong. However, even that doesn't map to the current ARM ARM
specification (for which the test would need to be against ISB, which
only supports SY (=0xf)).
I think (but I haven't tested the code just now) that the intended logic
should be:
If it's symbolic, validate it against the barrier type and only allow
permitted combinations. If the operand is numeric, accept the value
without question (we presume the user knows what they are doing).
Symbolic tests should be performed elsewhere. As such, I think the test
here no-longer needs a test on inst.instruction and that should be removed.
Please could make that change and also add some additional tests that
further validate the error checking.
R.
More information about the Binutils
mailing list