Re: [PATCH] arm: correct barrier immediate checks

[Richard Earnshaw <rearnsha at arm dot com>]

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.