i#1569 AArch64: Fix emit_special_ibl_xfer.

Commit log for first patchset:
---------------
i#1569 AArch64: Fix emit_special_ibl_xfer.
---------------

[bruening, 2016-07-21 04:53:00]

I'm a little confused and have some questions

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

https://codereview.appspot.com/299700043/diff/1/core/arch/emit_utils_shared.c...
core/arch/emit_utils_shared.c:5210: /* For AArch64, SCRATCH_REG0 is reserved for
link stub */
r0/xax is used for *direct* stubs for all arches, but r1/xbx is used for
*indirect*, and that's what this is emulating.

https://codereview.appspot.com/299700043/diff/1/core/arch/emit_utils_shared.c...
core/arch/emit_utils_shared.c:5212: /* For AArch64, the opnd_t tgt might use
TLS_REG1_SLOT */
Why would this be the case for A64 and not for another arch?

[bruening, 2016-07-21 04:54:52]

> i#1569 AArch64: Fix emit_special_ibl_xfer.

nit: "fix" implies there was a bug in the initial A64 port?

[Kevin Zhou, 2016-07-21 08:52:21]

On 2016/07/21 04:53:00, bruening wrote:
> I'm a little confused and have some questions
> 
> https://codereview.appspot.com/299700043/diff/1/core/arch/emit_utils_shared.c
> File core/arch/emit_utils_shared.c (right):
> 
>
https://codereview.appspot.com/299700043/diff/1/core/arch/emit_utils_shared.c...
> core/arch/emit_utils_shared.c:5210: /* For AArch64, SCRATCH_REG0 is reserved
for
> link stub */
> r0/xax is used for *direct* stubs for all arches, but r1/xbx is used for
> *indirect*, and that's what this is emulating.

The ibl routine on AArch64 requires x0 to have the linkstub (spilled in SLOT 0),
x1 is reserved for the indirect branch to ibl routines (spilled in SLOT 1) and
x2 contains the app target (spilled in SLOT 2).
See aarch64/emit_utils.c:emit_indirect_branch_lookup. We keep this format since
all stub branches are in the form of "br x1"

While ARM needs the linkstub in the r1 and app targets in r2. There is no need
for spilling r0 since we can directly modify the pc without clobbering an extra
register.
I don't think this code is related to direct stubs, it is for transferring from
the app to ibl via emitting an indirect stub. 

> 
>
https://codereview.appspot.com/299700043/diff/1/core/arch/emit_utils_shared.c...
> core/arch/emit_utils_shared.c:5212: /* For AArch64, the opnd_t tgt might use
> TLS_REG1_SLOT */
> Why would this be the case for A64 and not for another arch?
See definition of SPILL_SLOT_REDIRECT_NATIVE_TGT. We found TLS slot 1 conflicts
on AArch64 but there are no conflicts on other archs. This is because AArch64
uses 3 TLS slots and other uses only two.

[bruening, 2016-07-22 20:20:42]

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

https://codereview.appspot.com/299700043/diff/1/core/arch/emit_utils_shared.c...
core/arch/emit_utils_shared.c:5210: /* For AArch64, SCRATCH_REG0 is reserved for
link stub */
Please clarify the comment to say "even for indirect exits" or sthg.

https://codereview.appspot.com/299700043/diff/1/core/arch/emit_utils_shared.c...
core/arch/emit_utils_shared.c:5212: /* For AArch64, the opnd_t tgt might use
TLS_REG1_SLOT */
It's best to reply to the comments at the point in the code, so the context is
kept and comments can be seen at the appropriate patchset (much easier to review
multi-patchset diffs), rather than at the top-level.  Pasting:

On 2016/07/21 04:53:00, bruening wrote:
> > Why would this be the case for A64 and not for another arch?
> See definition of SPILL_SLOT_REDIRECT_NATIVE_TGT. We found TLS slot 1
conflicts on AArch64 but there are no conflicts on other archs. This is
because AArch64 uses 3 TLS slots and other uses only two.

I see:
#define SPILL_SLOT_REDIRECT_NATIVE_TGT SPILL_SLOT_1

That's TLS_REG3_SLOT.  I don't see any calls to emit_special_ibl_xfer() that
pass any other slots?

Could you clarify where A64 is using 3 slots and how that relates to the "tgt"
here?

[Edmund.Grimley.Evans, 2016-07-26 10:48:23]

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

https://codereview.appspot.com/299700043/diff/1/core/arch/emit_utils_shared.c...
core/arch/emit_utils_shared.c:5212: /* For AArch64, the opnd_t tgt might use
TLS_REG1_SLOT */
On 2016/07/22 20:20:42, bruening wrote:
> It's best to reply to the comments at the point in the code, so the context is
> kept and comments can be seen at the appropriate patchset (much easier to
review
> multi-patchset diffs), rather than at the top-level.  Pasting:
> 
> On 2016/07/21 04:53:00, bruening wrote:
> > > Why would this be the case for A64 and not for another arch?
> > See definition of SPILL_SLOT_REDIRECT_NATIVE_TGT. We found TLS slot 1
> conflicts on AArch64 but there are no conflicts on other archs. This is
> because AArch64 uses 3 TLS slots and other uses only two.
> 
> I see:
> #define SPILL_SLOT_REDIRECT_NATIVE_TGT SPILL_SLOT_1
> 
> That's TLS_REG3_SLOT.  I don't see any calls to emit_special_ibl_xfer() that
> pass any other slots?
> 
> Could you clarify where A64 is using 3 slots and how that relates to the "tgt"
> here?

These are difficult questions...

I can confirm that client.drwrap-test fails without this change.

As I see it, this change is necessary to conform to the protocol that has
already been established for the indirect branch lookup code. Whether that
protocol could or should be improved is harder to say... Kevin might be able to
contribute a better explanation in a few hours.

(The root cause of AArch64 being different is, of course, that we can't load
into the PC.)

[bruening, 2016-07-26 14:37:13]

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

https://codereview.appspot.com/299700043/diff/1/core/arch/emit_utils_shared.c...
core/arch/emit_utils_shared.c:5212: /* For AArch64, the opnd_t tgt might use
TLS_REG1_SLOT */
On 2016/07/26 10:48:23, Edmund.Grimley.Evans wrote:
> On 2016/07/22 20:20:42, bruening wrote:
> > It's best to reply to the comments at the point in the code, so the context
is
> > kept and comments can be seen at the appropriate patchset (much easier to
> review
> > multi-patchset diffs), rather than at the top-level.  Pasting:
> > 
> > On 2016/07/21 04:53:00, bruening wrote:
> > > > Why would this be the case for A64 and not for another arch?
> > > See definition of SPILL_SLOT_REDIRECT_NATIVE_TGT. We found TLS slot 1
> > conflicts on AArch64 but there are no conflicts on other archs. This is
> > because AArch64 uses 3 TLS slots and other uses only two.
> > 
> > I see:
> > #define SPILL_SLOT_REDIRECT_NATIVE_TGT SPILL_SLOT_1
> > 
> > That's TLS_REG3_SLOT.  I don't see any calls to emit_special_ibl_xfer() that
> > pass any other slots?
> > 
> > Could you clarify where A64 is using 3 slots and how that relates to the
"tgt"
> > here?
> 
> These are difficult questions...
> 
> I can confirm that client.drwrap-test fails without this change.
> 
> As I see it, this change is necessary to conform to the protocol that has
> already been established for the indirect branch lookup code. Whether that
> protocol could or should be improved is harder to say... Kevin might be able
to
> contribute a better explanation in a few hours.
> 
> (The root cause of AArch64 being different is, of course, that we can't load
> into the PC.)

So this comment "the opnd_t tgt might use TLS_REG1_SLOT" is incorrect and should
be deleted, right?

And the names "scratch_reg" and "scratch_slot" should instead be "stub_reg" and
"stub_slot".

[Kevin Zhou, 2016-07-26 15:02:48]

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

https://codereview.appspot.com/299700043/diff/1/core/arch/emit_utils_shared.c...
core/arch/emit_utils_shared.c:5212: /* For AArch64, the opnd_t tgt might use
TLS_REG1_SLOT */
On 2016/07/26 14:37:13, bruening wrote:
> On 2016/07/26 10:48:23, Edmund.Grimley.Evans wrote:
> > On 2016/07/22 20:20:42, bruening wrote:
> > > It's best to reply to the comments at the point in the code, so the
context
> is
> > > kept and comments can be seen at the appropriate patchset (much easier to
> > review
> > > multi-patchset diffs), rather than at the top-level.  Pasting:
> > > 
> > > On 2016/07/21 04:53:00, bruening wrote:
> > > > > Why would this be the case for A64 and not for another arch?
> > > > See definition of SPILL_SLOT_REDIRECT_NATIVE_TGT. We found TLS slot 1
> > > conflicts on AArch64 but there are no conflicts on other archs. This is
> > > because AArch64 uses 3 TLS slots and other uses only two.
> > > 
> > > I see:
> > > #define SPILL_SLOT_REDIRECT_NATIVE_TGT SPILL_SLOT_1
> > > 
> > > That's TLS_REG3_SLOT.  I don't see any calls to emit_special_ibl_xfer()
that
> > > pass any other slots?
> > > 
> > > Could you clarify where A64 is using 3 slots and how that relates to the
> "tgt"
> > > here?
> > 
> > These are difficult questions...
> > 
> > I can confirm that client.drwrap-test fails without this change.
> > 
> > As I see it, this change is necessary to conform to the protocol that has
> > already been established for the indirect branch lookup code. Whether that
> > protocol could or should be improved is harder to say... Kevin might be able
> to
> > contribute a better explanation in a few hours.
> > 
> > (The root cause of AArch64 being different is, of course, that we can't load
> > into the PC.)
> 
> So this comment "the opnd_t tgt might use TLS_REG1_SLOT" is incorrect and
should
> be deleted, right?
> 
> And the names "scratch_reg" and "scratch_slot" should instead be "stub_reg"
and
> "stub_slot".
Yes the comment is incorrect, this code was patched quite early and I was
treating SPILL_SLOT_1 the same as TLS_REG1_SLOT. The correct comment should be
"For AArch64, the linkstub has to be in X0 and the app's X0 needs to be spilled
in TLS_REG0_SLOT before calling the ibl routine."

[Edmund.Grimley.Evans, 2016-07-26 15:21:35]

Commit log for latest patchset:
---------------
i#1569 AArch64: Adapt emit_special_ibl_xfer for AArch64 ibl routine.

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

[bruening, 2016-07-26 15:56:58]

Please explain in the commit message what tests now work, as there are no test
changes in the CL.

Else LGTM

[Edmund.Grimley.Evans, 2016-07-27 08:25:51]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/6f9a7a597872f8e280480d1f467cd07...

Final commit log: 
---------------
i#1569 AArch64: Adapt emit_special_ibl_xfer for AArch64 ibl routine.

This is required for client.drwrap-test.

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