Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AArch64 fails to encode OP_ldr with pc-relative operand #5316

Open
derekbruening opened this issue Feb 1, 2022 · 2 comments
Open

AArch64 fails to encode OP_ldr with pc-relative operand #5316

derekbruening opened this issue Feb 1, 2022 · 2 comments

Comments

@derekbruening
Copy link
Contributor

For porting drbbdup to AArchXX for #4134 I changed the drbbdup tests to use rel-addr operands instead of abs-addr operands (via OPND_CREATE_ABSMEM) (xref #5295) and it all worked on AArch64: the tests pass. But they fail on ARM trying to encode the load of the rel-addr since it does not reach:

ERROR: Could not find encoding for: ldr    <rel> 0xb6fb2008[4byte] -> %r0

When I went to see how a64 worked: when logging is enabled it hits an assert:

SYSLOG_ERROR: Application /home/derek/dr/build/suite/tests/bin/simple_app (3952044).  Internal Error: DynamoRIO debug check failure: /home/derek/dr/
src/core/ir/disassemble_shared.c:1520 nxt_pc != NULL

Yet without logging it works!

In the debugger we have:

(gdb) x/90i targetf->start_pc
   0xffffb3fb0008:	ldp	x0, x1, [x28]
   0xffffb3fb000c:	str	x1, [x28, #360]
   0xffffb3fb0010:	mrs	x0, nzcv
   0xffffb3fb0014:	str	x0, [x28, #344]
   0xffffb3fb0018:	mov	x0, #0xe080                	// #57472
   0xffffb3fb001c:	movk	x0, #0xb3f9, lsl #16
   0xffffb3fb0020:	movk	x0, #0xffff, lsl #32
   0xffffb3fb0024:	ldr	x0, [x0]
   0xffffb3fb0028:	cmp	x0, #0x1
   0xffffb3fb002c:	b.ne	0xffffb3fb0048  // b.any

The instrumentation had just a ldr with a rel-addr opnd: so who inserted the mov;movk;movk?

Disabling the disassembly assert (looks like it only really needs it for the length: so maybe we should remove the encode there for a64) logging now works

after instrumentation:
TAG  0x0000ffffb38840c0
 +0    m4 @0x0000fffd6f8e50e8  6d6d7564   str    %x1 -> +0x0168(%x28)[8byte]
 +4    m4 @0x0000fffd6f8e4da0  6d6d7564   mrs    %nzcv -> %x0
 +8    m4 @0x0000fffd6f8e4cd8  6d6d7564   str    %x0 -> +0x0158(%x28)[8byte]
 +12   m4 @0x0000fffd6f8e4c58  6d6d7564   ldr    <rel> 0x0000ffff6f856080[8byte] -> %x0
 +16   m4 @0x0000fffd6f8e4e20  6d6d7564   <label>
 +20   m4 @0x0000fffd6f8e4bd8  6d6d7564   subs   %x0 $0x0000000000000001 lsl $0x0000000000000000 -> %xzr
 +24   m4 @0x0000fffd6f8e4b10  6d6d7564   b.ne   @0x0000fffd6f8e4fa0[8byte]
...

bb ilist after mangling:
TAG  0x0000ffffb38840c0
 +0    m4 @0x0000fffd6f8e50e8  6d6d7564   str    %x1 -> +0x0168(%x28)[8byte]
 +4    m4 @0x0000fffd6f8e4da0  6d6d7564   mrs    %nzcv -> %x0
 +8    m4 @0x0000fffd6f8e4cd8  6d6d7564   str    %x0 -> +0x0158(%x28)[8byte]
 +12   m4 @0x0000fffd6f8e4418  6d6d7564   movz   $0x6080 lsl $0x00 -> %x0
 +16   m4 @0x0000fffd6f8e4350  6d6d7564   movk   %x0 $0x6f85 lsl $0x10 -> %x0
 +20   m4 @0x0000fffd6f8e42d0  6d6d7564   movk   %x0 $0xffff lsl $0x20 -> %x0
 +24   m4 @0x0000fffd6f8e4208  6d6d7564   ldr    (%x0)[8byte] -> %x0
 +28   m4 @0x0000fffd6f8e4e20  6d6d7564   <label>
 +32   m4 @0x0000fffd6f8e4bd8  6d6d7564   subs   %x0 $0x0000000000000001 lsl $0x0000000000000000 -> %xzr
 +36   m4 @0x0000fffd6f8e4b10  6d6d7564   b.ne   @0x0000fffd6f8e4fa0[8byte]
...

It looks like the a64 mangling does the immediate-into-reg expansion.
But this is tool code: why is it being mangled?
Here we go:

        if (instr_has_rel_addr_reference(instr)
            /* XXX i#1834: it should be up to the app to re-relativize, yet on amd64
             * our own samples are relying on DR re-relativizing (and we just haven't
             * run big enough apps to hit reachability problems) so for now we continue
             * mangling meta instrs for x86 builds.
             */
            IF_ARM(&&instr_is_app(instr))) {
            instr_t *res = mangle_rel_addr(dcontext, ilist, instr, next_instr);

So the IF_ARM doesn't apply to a64.

#1834 is confusingly closed: I re-opened it since it does not seem completely resolved.

This issue covers the disas assert and specific aarchxx issues. The rest overlaps with #1834.

@derekbruening derekbruening self-assigned this Feb 1, 2022
@derekbruening derekbruening changed the title AArch64 meta pc-rel addr hits ASSERT in debug yet mysteriously works in release, while ARM fails AArch64 meta pc-rel addr hits ASSERT with logging yet mysteriously works without, while ARM fails Feb 1, 2022
@derekbruening
Copy link
Contributor Author

The disassembly assert is an encoding failure not due to ignoring the !check_reachable but due to lack of pc-relative support in general for OP_ldr. Xref #4847 (comment) and #5295.

@derekbruening
Copy link
Contributor Author

The issue of whether DR should mangle tool instructions, including the current state of doing so for all but ARM, is under #1834.

So I think that means this issue should change to cover just the lack of pc-relative encoding for OP_ldr on AArch64.

@derekbruening derekbruening changed the title AArch64 meta pc-rel addr hits ASSERT with logging yet mysteriously works without, while ARM fails AArch64 fails to encode OP_ldr with pc-relative operand Feb 2, 2022
derekbruening added a commit that referenced this issue Feb 2, 2022
Enables building drbbdup and its tests on AArchXX.

Adds case comparison, conditional branch, and TLS access code for both
AArch64 and ARM.  Uses optimizations for comparing small case values;
a second scratch register is needed for larger values.

Ports the tests by using OPND_CREATE_ABSMEM (xref #5295).  To make
these pc-relative case encoding loads work on AArchXX in the presence
of AArch64 encoder bugs (#5316) and to avoid a more-complex interface, drbbdup
auto-inserts loading the target address into its scratch register to
avoid reachability problems (xref #1834 on having DR do this during
mangling).

Avoids storing block translations if there is no dynamic handling,
improving robustness and reducing memory usage.

Adds the tests to the passes-under-QEMU list for cross-compilation.
Additionally, I tested manually on native ARM and AArch64 machines.

Issue: #4134, #5295, #1834, #3995, #5316
derekbruening added a commit that referenced this issue Feb 2, 2022
Makes ARM behave like x86 and AArch64 by having DR mangle pc-relative
operands in tool instructions on all architectures.

Full support in DR's translation code is still needed.

Similarly mangling stolen register and segment references is also
still undone; it may require a new bit identifying "app operands".

Issue: #1834, #5316
derekbruening added a commit that referenced this issue Feb 2, 2022
Makes ARM behave like x86 and AArch64 by having DR mangle pc-relative
operands in tool instructions on all architectures.

Full support in DR's translation code is still needed.

Similarly mangling stolen register and segment references is also
still undone; it may require a new bit identifying "app operands".

Issue: #1834, #5316
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants