This is the mail archive of the mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] arm: correct barrier immediate checks

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.

2013-04-08  Jan Beulich <>

	* gas/config/tc-arm.c (do_barrier): Correct constraint()
	(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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]