i#1569 AArch64: Add enums and struct members for memory operands.

Commit log for first patchset:
---------------
i#1569 AArch64: Add enums and struct members for memory operands.

We add dr_extend_type_t, DR_OPND_IS_EXTEND and these fields to base_disp:
pre_index, extend_type, scaled. Functions to access these fields are
provided.
---------------

[bruening, 2016-08-10 02:33:42]

With new API functions, best to add a test (to api/ir_aarch64.c or somewhere) in
the same CL

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd.h
File core/arch/opnd.h (right):

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd.h#newcode735
core/arch/opnd.h:735: DR_EXTEND_UXTB = 0,
These all need doxygen descriptions

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd.h#newcode761
core/arch/opnd.h:761: * on its own. The same flag is also used for extended
values in AArch64.
This seems odd -- wouldn't it be better to have DR_OPND_EXTENDED and set it to
equal DR_OPND_SHIFTED if they're the same value?  Also, make it clear whether
all of the stated shift caveats apply.

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd.h#newcode771
core/arch/opnd.h:771: /** This immed integer operand should be interpreted as an
ARM/AArch64 shift type. */
Seems best to spell out "immediate"

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd.h#newcode775
core/arch/opnd.h:775: /** This immed integer operand should be interpreted as an
AArch64 extend type. */
Seems best to spell out "immediate".

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd.h#newcode854
core/arch/opnd.h:854: byte extend_type : 2;
Do you mean "byte/*dr_extend_type_t*/"?  How can 2 bits handle the 8 enum
values?

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd.h#newcode1204
core/arch/opnd.h:1204: *
Add a note saying that it's AARCH64-only.  (Look at the generated docs and
you'll see why -- there are no ifdefs there.)

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd.h#newcode1755
core/arch/opnd.h:1755: * If there is no extension and no shift the values
returned will be DR_EXTEND_UXTX,
Use # to get a doxygen link

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd.h#newcode1757
core/arch/opnd.h:1757: */
Missing note

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd.h#newcode1766
core/arch/opnd.h:1766: * by is determined by the size of the memory operand.
Missing note

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd_shared.c
File core/arch/opnd_shared.c (right):

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd_shared.c#newco...
core/arch/opnd_shared.c:737: static uint opnd_size_to_extend_amount(opnd_size_t
size)
style violation

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd_shared.c#newco...
core/arch/opnd_shared.c:763: extend = (t & 2) << 1 | 2 | (t & 1);
It is not clear what this is doing: needs explanation

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd_shared.c#newco...
core/arch/opnd_shared.c:783: if ((extend | 5) != 7) {
Can we arrange the enum to make this a simple <= or >= check?  I don't see any
change to the encoder, so I assume the enum values are not coming from actual
encoding bits and thus can be changed.

https://codereview.appspot.com/308050043/diff/1/core/arch/opnd_shared.c#newco...
core/arch/opnd_shared.c:787: opnd->value.base_disp.extend_type = (extend >> 1 &
2) | (extend & 1);
?  This field needs explanation.

[Edmund.Grimley.Evans, 2016-08-10 16:09:03]

Commit log for latest patchset:
---------------
i#1569 AArch64: Add enums and struct members for memory operands.

We add dr_extend_type_t, DR_OPND_IS_EXTEND and these fields to base_disp:
pre_index, extend_type, scaled. Functions to access these fields are
provided.

Review-URL: https://codereview.appspot.com/308050043
---------------

[bruening, 2016-08-11 04:30:23]

I'm wondering whether we can just put the dr_extend_type in opnd_t?  We have the
space, and everything would be much simpler.

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h
File core/arch/opnd.h (right):

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h#newcode755
core/arch/opnd.h:755: * This register's value is shifted or extended prior to
use in the containing
I don't understand why these are being merged: is there no such thing as a
shifted register in A64?  Even if so, what is the value in squishing them
together?  What current code looks for DR_OPND_SHIFTED that should also apply to
extensions?

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h#newcode771
core/arch/opnd.h:771: /** This immediate integer operand should be interpreted
as an ARM/AArch64 shift type.
Style violation

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h#newcode776
core/arch/opnd.h:776: /** This immediate integer operand should be interpreted
as an AArch64 extend type. */
Style violation

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h#newcode854
core/arch/opnd.h:854: /* Bits 0 and 2 from dr_extend_type_t. Access this only
using
Why just 0 and 2?  I don't understand, which means that other readers of the
code may also find it confusing.

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd_shared.c
File core/arch/opnd_shared.c (right):

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd_shared.c#n...
core/arch/opnd_shared.c:764: * Return one of DR_EXTEND_{UXTW,UXTX,SXTW,SXTX}.
It sounds like the B and H ones are not supported for index registers.  But
we're not short on bits in opnd_t, right?  Wouldn't it be simpler, with clearer
code, to just store the 3 bits of the value and avoid all of this bit
recombining?

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd_shared.c#n...
core/arch/opnd_shared.c:786: if ((extend | 5) != 7) {
Clearer, more readable to have "if (TEST(1, extend))"

https://codereview.appspot.com/308050043/diff/20001/suite/tests/api/ir_aarch64.c
File suite/tests/api/ir_aarch64.c (right):

https://codereview.appspot.com/308050043/diff/20001/suite/tests/api/ir_aarch6...
suite/tests/api/ir_aarch64.c:47: #endif
This is quite old: not necessary.

https://codereview.appspot.com/308050043/diff/20001/suite/tests/api/ir_aarch6...
suite/tests/api/ir_aarch64.c:53: #define VERBOSE 0
Unused

[Edmund.Grimley.Evans, 2016-08-11 07:33:06]

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h
File core/arch/opnd.h (right):

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h#newcode755
core/arch/opnd.h:755: * This register's value is shifted or extended prior to
use in the containing
On 2016/08/11 04:30:23, bruening wrote:
> I don't understand why these are being merged: is there no such thing as a
> shifted register in A64?  Even if so, what is the value in squishing them
> together?  What current code looks for DR_OPND_SHIFTED that should also apply
to
> extensions?

An A64 ADD instruction can have a shifted or an extended register operand. The
instructions can be distinguished, of course, by looking at the shift/extend
operand; we don't need DR_OPND_SHIFTED for that. In fact, it's hard to see why
DR_OPND_SHIFTED is needed at all. The only use seems to be in arm/disassemble.c,
where it's rather unclear exactly what it does (in opnd_disassemble_noimplicit).
However, I'd expect "shifted" and "extended" to be treated the same way in A64
disassembly, if DR_OPND_SHIFTED is referenced at all.

Are we short of bits? Not very: we're using 7 out of 16 (ushort).

So the options are:

1. Get rid of DR_OPND_SHIFTED: big patch as there are lots of places that set
the flag even if there is only one place that reads it.

2. Rename it to DR_OPND_MODIFIED and use it for both things: big patch again.

3. Leave DR_OPND_SHIFTED as ARM-only. A slightly irritating inconsistency
between ARM and AArch64, and there's a risk it might turn out to be useful after
all, though it could be added later.

4. Add DR_OPND_EXTENDED with the same numerical value as DR_OPND_SHIFTED. That
would almost certainly work, but would be confusing, I think, as we will be
using both for A64.

5. Add DR_OPND_EXTENDED as a separate flag. We'd then be using 8 of the 16 bits.
A comment could point out that two of them are probably not really necessary, in
case we run out of bits later.

Options 3 and 5 look like the best ones. What do you think? How useful is it to
have this almost unused flag "for informational purposes only"?

I'm inclined to go with 5, though.

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd_shared.c
File core/arch/opnd_shared.c (right):

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd_shared.c#n...
core/arch/opnd_shared.c:764: * Return one of DR_EXTEND_{UXTW,UXTX,SXTW,SXTX}.
On 2016/08/11 04:30:23, bruening wrote:
> It sounds like the B and H ones are not supported for index registers.  But
> we're not short on bits in opnd_t, right?  Wouldn't it be simpler, with
clearer
> code, to just store the 3 bits of the value and avoid all of this bit
> recombining?

Quite possibly. I was originally motivated by the shortage of bits when there
was no union/ifdef. Then I thought I'd leave it that way because in some ways
it's simpler if there's no way of even representing the things that we can't
handle. However, presumably the only place where we'd have to check for and
reject the B and H extensions would be in the encoder, where we already do lots
of checks and know how to fail. So I think I'll try rewriting it with 3 bits
instead of 2 for the dr_extend_type_t.

[Edmund.Grimley.Evans, 2016-08-12 11:39:17]

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h
File core/arch/opnd.h (right):

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h#newcode776
core/arch/opnd.h:776: /** This immediate integer operand should be interpreted
as an AArch64 extend type. */
On 2016/08/11 04:30:23, bruening wrote:
> Style violation

How?

[Edmund.Grimley.Evans, 2016-08-12 12:59:04]

Commit log for latest patchset:
---------------
i#1569 AArch64: Add enums and struct members for memory operands.

We add dr_extend_type_t, DR_OPND_IS_EXTEND and these fields to base_disp:
pre_index, extend_type, scaled. Functions to access these fields are
provided.

Review-URL: https://codereview.appspot.com/308050043
---------------

[bruening, 2016-08-15 19:31:44]

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h
File core/arch/opnd.h (right):

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h#newcode755
core/arch/opnd.h:755: * This register's value is shifted or extended prior to
use in the containing
DR_OPND_SHIFTED is required for disassembly in ARM format.  Secondarily it helps
indicate the semantics of shifted immeds  to tools, but like with most semantic
issues tools still likely need to dispatch on opcode.  Just like for x86,
disassembly in non-DR format is not given first-class support, and we're ok
failing to do it correctly if someone sets IR components without going through
"normal" channels.  If the plan is to do something similar for A64,
DR_OPND_EXTENDED should be a separate field (option 5); if instead disassembly
in non-DR is given better support, it may not be needed at all, depending on
whether a hint to tools looking at several immeds in a row is considered
important.

[Edmund.Grimley.Evans, 2016-08-24 11:07:41]

I think all comments have been addressed in the latest patch set.

[bruening, 2016-08-26 00:30:06]

LGTM, but please hold off on commit until Mon as I hope to put out a quick new
release for Windows 1607 support and it will make it easier to test if there are
no other changes in the middle

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h
File core/arch/opnd.h (right):

https://codereview.appspot.com/308050043/diff/20001/core/arch/opnd.h#newcode776
core/arch/opnd.h:776: /** This immediate integer operand should be interpreted
as an AArch64 extend type. */
On 2016/08/12 11:39:17, Edmund.Grimley.Evans wrote:
> On 2016/08/11 04:30:23, bruening wrote:
> > Style violation
> 
> How?

Err, no idea, must have mis-read it and thought it was like the one above

[Edmund.Grimley.Evans, 2016-08-30 08:53:48]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/bed62b27ffeaf22851e3fce1c205356...

Final commit log: 
---------------
i#1569 AArch64: Add enums and struct members for memory operands.

We add dr_extend_type_t, DR_OPND_IS_EXTEND and these fields to base_disp:
pre_index, extend_type, scaled. Functions to access these fields are
provided.

Review-URL: https://codereview.appspot.com/308050043
---------------