i#2491 ARM unallocated encodings: Better handling of undecoded instrs.

[egrimley]

In check_encode_decode_consistency(), do not try to reencode an
OP_UNDECODED instruction.

In instr_encode_arch(), use instr->opcode, not instr_get_opcode(instr),
as the latter can call the decoder, which is unhelpful when we are
trying to encode.

Fixes #2491

Change-Id: I66c42dc87268f1722eae4600b364026896796fc8

[egrimley]

AppVeyor failure is #2286.

[derekbruening]

It seems better to match what x86 does: move handling of raw-bits-valid instrs up above this decode_info_t init. That will be more efficient and more symmetric. Then we have an invariant that it's level 3+ by the time we get here, which the operands_valid assert will double-check.

[egrimley]

I presume you mean: copy the 7 lines starting with "/* first handle the already-encoded instructions */" from x86/encode.c and insert them at line 2767. I'll test that tomorrow.

[derekbruening]

No, not from x86, just reorder the lines in this routine.

[egrimley]

Note the comment at line 2776: "We need to track the IT block state even for raw-bits-valid instrs." So we can't significantly reorder this stuff.

[egrimley]

I think there are perhaps two general, cross-architecture things here which I don't want to address in this commit:

• instr_get_opcode looks like a harmless accessor but may invoke the (full) decoder.

• OP_UNDECODED might mean that we haven't got round to decoding this instruction, or it might mean that we have tried and found that the encoding does not correspond to any known instruction (except on AArch64 where the decoder always succeeds). Perhaps this distinction is well-defined somehow, but it's not clear to me, at least.

[egrimley]

Ah, the original problem... I think it's that INSTR_RAW_BITS_VALID gets cleared by the use of instr_get_opcode, perhaps by a call to instr_reuse.

[egrimley]

Or perhaps because of the early return from decode_opcode.

[egrimley]

The x86 code has an early return from decode_opcode, before the call to instr_set_raw_bits, but the x86 code does not call instr_get_opcode from instr_encode_arch before testing instr_raw_bits_valid. An awful lot of twiddling of INSTR_RAW_BITS_VALID can occur because of an innocent-looking instr_get_opcode...

[derekbruening]

encode_track_it_block_di() needs to know whether this is OP_it, so it seems like it has to call the decoder from this encode routine if the instr is undecoded.

For instructions that the manual calls "unpredictable": see #1685.

[derekbruening]

Ah, the original problem... I think it's that INSTR_RAW_BITS_VALID gets cleared by the use of instr_get_opcode, perhaps by a call to instr_reuse.

Looks like decode_opcode should set the raw bits before returning NULL on an invalid instr (and probably that CLIENT_ASSERT should not be there since it's called internally like this). Messy -- but so is the IT block tracking so I don't think there's a simpler solution.

Why is it OP_UNDECODED in the first place -- ARM doesn't have a fast decoder so isn't the opcode always set or set to OP_INVALID?

[fhahn]

Could this be tested in dis-armA32? Maybe I am missing something, but as long as the binary is not executed, could we add an invalid instruction encoding to the test binary? The decoder should use OP_UNDECODED and not encode it again (and not crash at it does now I assume).

[egrimley]

There are already several invalid encodings in dis-armA32. The crash only happens when you try to execute one of them under DynamoRIO.

[egrimley]

Is the real, original problem, perhaps, that the A32 decoder is returning OP_UNDECODED instead of OP_INVALID for 0xf2800f00?

[derekbruening]

Is the real, original problem, perhaps, that the A32 decoder is returning OP_UNDECODED instead of OP_INVALID for 0xf2800f00?

Do we know why this happened? ARM doesn't have a fast decoder so isn't the opcode always set or set to OP_INVALID?

[egrimley]

Correction: the decoder originally gives OP_INVALID, but bb_process_invalid_instr(), called from build_bb_ilist(), changes it to OP_UNDECODED by calling instr_set_raw_bits(). It's a maze of twisty little passages, most of them fiddling with INSTR_RAW_BITS_VALID.