[PATCH] Fix implementation for Thumb ADR instruction

[Mihail Popa]

Hi Tim.

Nice to hear from you again. It's been a while.

Let me start by stating the more general problem we are having. Currently
there is no range checking
of any kind done for PC relative offsets. This leads to some pretty
hilarious results. One can apparently
encode gigantic branches in meagre 16-bit instructions. For example:

echo "b #4294967295" | build/bin/llvm-mc -triple thumbv7 -show-encoding
.text
b #4294967295             @ encoding: [0xff,0xe7]

This happens because Thumb and Thumb2 have multiple encodings for certain
instructions. The flavours
vary, but in brief there are four main variates:

1. Thumb 16-bit predicable instruction taking short offset
2. Thumb 16-bit non predicable instruction taking medium offset
3. Thumb2 32-bit predicable instruction taking larger offset
4. Thumb2 32-bit non-predicable instruction taking really large offset

One good example of such instruction is the branch which exhibits all these
variants (encodings T1 through T4).
in be absence of range checking, MC is free to pick any encoding for any
offset size. In the case of branch, it
always picks encoding T1 or T2. There are many instructions which have
multiple PC-relative forms. A few of
the top of my head: branch, branch linked, load literal, store literal,
adr, preload instruction.

This seems rather general given that it's just used once. I'd suggest
either stripping it back or doing a separate refactoring to
consolidate things.

The template function I am introducing is simply building infrastructure
for this. At this time it is used only once,
but it will be used by later patches with different template arguments. Had
I fixed definition for multiple instructions
in one patch you would have rightly objected and asked me to split them.
However, it is true I should have dropped
a note about the intended use of the template function.

Also, unless I'm mistaken its constraints are appropriate for 2s
complement arithmetic while ARM usually uses sign/magnitude for its
offsets. At the very least this assumption should be commented.

Please correct me if I'm wrong, but ARM does indeed use 2's complement
arithmetic! There is indeed one slight
deviation in that INT32_MIN is sometimes regarded as negative zero (maybe
that's a relic for a distant dinosaur
that used direct representation?) Either way, all ARM cores that I can
think of use 2's complement with this little
occasional quirk. Am I wrong here?

In terms of offset encoding, let's look :

b #-4                     @ encoding: [0xfe,0xe7]
b #-2                     @ encoding: [0xff,0xe7]
b #0                      @ encoding: [0x00,0xe0]
b #2                      @ encoding: [0x01,0xe0]
b #4                      @ encoding: [0x02,0xe0]

Keeping in mind that that branch offsets are scaled by one, what is
represented are actually -2, -1, 0, 1 and 2
in precise 2's complement representation.

+  return MO.getImm() >> 2;

I think the preferred approach is to use an already-shifted MCOperand
so that invalid instructions can't exist by design. That removes the
need for the decoder, but makes your AsmParser render methods (which
currently seem pretty much identical to addImm) useful again. CodeGen
may also need changing if it ever generates a truly immediate ADR
(doubtful).

I don't disagree that having the operand pre-shifted doesn't make it more
difficult to generate invalid MCInsts.
But that doesn't mean that invalid instructions can't exist "by design". It
is true that "unaligned" immediate
operands can't exist by design, but an operand can still be invalid while
being aligned - for example by being out
of range.

The only way to guarantee that, at any point in its lifetime a MCInst is
not invalid involves:

1. creating it in a single place, with a single well tested method
2. emitting it from a single place with a single tested method
3. keeping the instruction constant throughout the lifetime of the MC
component

Guaranteeing this is indeed a tall order! The point here is that whether
keeping an integer shifted or unshifted
really doesn't do much for the correctness of anything.

Leaving this argument aside, I believe that it is actually better to keep
the true number of the field in an MCInst
rather than its machine representation, but I will grant you this is sort
of religious issue. It's a war I am not
prepared to fight :)

For a more practical reasoning please look at the immediate form of AND. It
uses "modified immediate" fields.
These operands are in fact represented as "normal" numbers and then
rendered as "modified" 12 or 8 bit fields
at encode time. There is a good reason for it too - imagine debugging MC
and wanting to check the value of
such a field using a debugger! You would stand to chance. It's no longer an
issue of shifting some bits, or masking
some bits out. It's is really complex and makes no immediately apparent
sense! Also the logic for "decoding" these
numbers should be moved to the printer instead of the encoder -- but this
is an encoding problem!

In the interest of uniformity and readability, the decision of how to
represent immediate fields should have a single
answer. I believe that that answer is "straight, natural" representation,
but as I said, I'm not looking to fight a war
over this. Just adding my two pence to the discussion.

Regards,
Mihai


On Wed, Jun 26, 2013 at 6:47 PM, Jim Grosbach <grosbach at apple.com> wrote:

>
> On Jun 26, 2013, at 9:37 AM, Tim Northover <t.p.northover at gmail.com>
> wrote:
>
> Hi Mihail,
>
> +  template<unsigned width, unsigned scale, bool positive>
> +  bool isMemoryOffset() const {
>
> This seems rather general given that it's just used once. I'd suggest
> either stripping it back or doing a separate refactoring to
> consolidate things.
>
> Also, unless I'm mistaken its constraints are appropriate for 2s
> complement arithmetic while ARM usually uses sign/magnitude for its
> offsets. At the very least this assumption should be commented.
>
> +  return MO.getImm() >> 2;
>
> I think the preferred approach is to use an already-shifted MCOperand
> so that invalid instructions can't exist by design. That removes the
> need for the decoder, but makes your AsmParser render methods (which
> currently seem pretty much identical to addImm) useful again. CodeGen
> may also need changing if it ever generates a truly immediate ADR
> (doubtful).
>
>
> Yes, exactly right.
>
> -Jim
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130627/c798f278/attachment.html>