This is the mail archive of the
mailing list for the binutils project.
Re: [PATCH] arm: correct barrier immediate checks
- From: Richard Earnshaw <rearnsha at arm dot com>
- To: Jan Beulich <JBeulich at suse dot com>
- Cc: "paul at codesourcery dot com" <paul at codesourcery dot com>, "nickc at redhat dot com" <nickc at redhat dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Mon, 08 Apr 2013 11:28:53 +0100
- Subject: Re: [PATCH] arm: correct barrier immediate checks
- References: <5162A94702000078000CB40D at nat28 dot tlf dot novell 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.
2013-04-08 Jan Beulich <email@example.com>
* gas/config/tc-arm.c (do_barrier): Correct constraint()
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
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.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
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.