search

DynamoRIO / dynamorio

Dynamic Instrumentation Tool Platform
Other
2.65k stars 560 forks source link

AArch64 decoder: Add all memory-touching instructions #4847

Open derekbruening opened 3 years ago

derekbruening commented 3 years ago

Splitting sub-pieces off from the master issue #2626 on finishing the AArch64 decoder. This piece covers ensuring that all instructions with memory operands are fully decoded and encoded. Xref the instruction lists here: https://github.com/DynamoRIO/dynamorio/issues/2626#issuecomment-771605153

AssadHashmi commented 3 years ago

Hi @derekbruening, do you have any specific instances of memory instructions which are/were failing?

derekbruening commented 3 years ago

I do not know of any cases where a memory operand was missed (there are known cases of the decoder decoding the wrong address into the IR and clients having to take extra steps to get the right address: #4400), but it is the kind of thing that would go silently missing and would not be noticed up front, leading to unknowingly inaccurate traces. Hence we're relying on whatever validation steps can be taken over the decoder.

derekbruening commented 3 years ago

If more might be missing, the DC ZVA case may point to extra inspection warranted in areas of the ISA where loads or stores might be lurking but disguised as sub-opcodes of system or other instructions.

AssadHashmi commented 3 years ago

Hence we're relying on whatever validation steps can be taken over the decoder.

We're identifying gaps in our test coverage of memory instructions and will shortly be working on PRs to test and fix failures.

AssadHashmi commented 3 years ago

Missing opcodes that touch memory: 0 of 48 as at 17-06-21.

derekbruening commented 2 years ago

Are adr and adrp supported? The tests for them are still disabled in ir_aarch64.c:

#if 0 /* TODO i#4847: add memory touching instructions */
    adr(dcontext);
    adrp(dcontext);
#endif
derekbruening commented 2 years ago

Also:

#if 0 /* TODO i#4847: address memory touching instructions that fail to encode */
        ldr_base_literal(dc);
#endif

For #3995 we will use opnd_create_rel_addr for the drbbdup case encoding: I'm worried it's not going to encode, given the above disabled test for this operand type.

derekbruening commented 2 years ago

We want to have the encoder handle ABS_ADDR_kind as well by treating it like REL_ADDR_kind: #5295

derekbruening commented 2 years ago

We want to have the encoder handle ABS_ADDR_kind as well by treating it like REL_ADDR_kind: #5295

This is still up in the air: see #5295. It may be better to just not support abs-addr for AArchXX.

derekbruening commented 2 years ago

For #3995 we will use opnd_create_rel_addr for the drbbdup case encoding: I'm worried it's not going to encode, given the above disabled test for this operand type.

Confirmed: OP_ldr with a pc-relative opnd fails to encode. It results in an assert when logging: #5316.

We will have to try to work around this for drbbdup on aarchxx #4134.