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 opnd_create_abs_addr() succeeds but opnd_is_memory_reference() returns false #5295

Open
derekbruening opened this issue Jan 27, 2022 · 3 comments

Comments

@derekbruening
Copy link
Contributor

For #4134 and #3995 I'm porting drbbdup to arm and its tests call opnd_create_abs_addr() to create the runtime_case_opnd. DR lets it create an absolute addr opnd of type ABS_ADDR_kind on AArch64; but then opnd_is_memory_reference returns false on AArch64 because the check for opnd_is_abs_addr() is limited to IF_X86_64. Basically, DR isn't sure it wants to support absolute address operands for AArch64, since the addressing modes there do not have that sort of thing. Maybe users are expected to use opnd_create_rel_addr() instead (AArch64 does have #define OPND_CREATE_ABSMEM(addr, size) opnd_create_rel_addr(addr, size)): but the docs for opnd_create_abs_addr() say it will auto-convert into a rel addr.

The rest of the code for abs addr handling in opnd_shared.c is just gated by X64, so opnd_is_memory_reference looks like an anomaly. So maybe the solution is to fix opnd_is_memory_reference to return true, and ensure the AArch64 and ARM encoders handle ABS_ADDR_kind and treat it just like rel-addr (the alternative of converting to rel-addr at creation time might confuse users who would query their own just-created abs-addr opnd and it would return false for opnd_is_abs_addr()).

I went to check whether the AArch64 encoder might already handle abs-addr: but it looks like it doesn't even handle rel-addr! See
#4847 (comment)

@derekbruening
Copy link
Contributor Author

For 32-bit ARM: from a quick glance at the encoder, it looks like it does not think abs-addr is valid and expects only rel-addr.

@derekbruening
Copy link
Contributor Author

derekbruening commented Jan 27, 2022

I had thought since abs-addr was defined on a32 it was auto-converted there, and that's why we should have a64 do the same -- but if a32 doesn't really support it, an alternative is to go and make abs-addr x86-only entirely in the docs and w/ ifdefs in the headers and code. That would then require no encoder changes. Users could still use OPND_CREATE_ABSMEM for cross-platform code and it would turn into rel-addr at creation time.

@AssadHashmi AssadHashmi self-assigned this Jan 27, 2022
@derekbruening
Copy link
Contributor Author

@abhinav92003 is voting for making abs-addr x86-only. Looking at 32-bit arm implementation notes: I do not see any mention of it so I assume there was no real plan for it; so making abs-addr operands x86-only seems reasonable.

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 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.

Tested manually on a native ARM machine.

Issue: #4134, #5295, #1834, #3995
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
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