arm: Add ARM_OP_ADDR

[akihikodaki]

ATTENTION: THIS CHANGE BREAKS COMPATIBILITY

I found the assumption that address is stored as ARM_OP_IMM is actually already broken with #762. However, it is no longer possible to put it to imm since address can be greater than INT32_MAX. Doing so can result in anything, especially faults in arithmetic operations.

Moreover, the old binding in Ocaml ignores bit 31 since the length of int in the language is 31 bits. It means addresses should be treated differently in the binding.

Though the series of changes is a bug fix, breaking compatibility is inevitable.

See also #765.

[radare]

please rebase

[radare]

ok to break compat for me unless it introduces regressions

[pranith]

Can you please generate a new PR on libcapstone?

[akihikodaki]

Rebased to commit 3f46b83 with commit 2e6575f, and it is mirrored to libcapstone/libcapstone#3.

[pranith]

@aquynh LGTM.

[XVilka]

Relevant to the ARM auto-sync work #1949 and #2045

[Rot127]

Indeed it is. But I would not add ARM_OP_ADDR as additional operand type. The underlying problem is more that the immediate type is int32_t and not uint64_t + some information how to interpret it.

The new operand mapping tables contain the type of each operand. (although they seem to be off. E.g. the address is signed. Maybe we need to fix the td files for this:

Here an example:

{ /* ARM_VLD1LNdAsm_16 (384) - ARM_INS_VLD1 - vld1${p}.16	$list, $addr */
{
  { CS_OP_REG, CS_AC_READ, { CS_DATA_TYPE_f64, CS_DATA_TYPE_v8i8, CS_DATA_TYPE_v4i16, CS_DATA_TYPE_v2i32, CS_DATA_TYPE_v1i64, CS_DATA_TYPE_v2f32, CS_DATA_TYPE_v4f16, CS_DATA_TYPE_v4bf16, CS_DATA_TYPE_LAST } }, /* list - DPR */
  { CS_OP_IMM, CS_AC_READ, { CS_DATA_TYPE_i32, CS_DATA_TYPE_LAST } }, /* list - i32imm */
  { CS_OP_MEM | CS_OP_REG, CS_AC_READ, { CS_DATA_TYPE_i32, CS_DATA_TYPE_LAST } }, /* addr - GPR */
  { CS_OP_MEM | CS_OP_IMM, CS_AC_READ, { CS_DATA_TYPE_i32, CS_DATA_TYPE_LAST } }, /* addr - i32imm */
  { CS_OP_IMM, CS_AC_READ, { CS_DATA_TYPE_i32, CS_DATA_TYPE_LAST } }, /* p - i32imm */
  { CS_OP_IMM, CS_AC_READ, { CS_DATA_TYPE_i32, CS_DATA_TYPE_LAST } }, /* p - i32imm */
  { 0 }
}},

So I would propose to solve this problem by making arm_op->imm an uint64_t and add a helper function like

typedef union {
  uint32_t u32;
  int32_t s32;
  float f32;
  ...
} cast_result;

/// Cast the given @imm to its data type specified by LLVM.
/// It returns the cs_data_type the @imm was casted to.
/// The corresponding field in @result is set to the casted value.
cs_data_type map_cast_operand(uint64_t imm, cast_result *result);

[akihikodaki]

I'm closing this pull request. As @Rot127 says, the sign of the immediate should be determined by the instruction.
ARM_OP_ADDR clarifies an immediate is unsigned if it's PC-relative, but it cannot tell the sign of the immediate otherwise. There should be some common mechanism to determine the sign of the immediate, and such a mechanism can be implemented easily with a table @Rot127 suggests.

However I suggest to leave imm 32-bit instead of converting it to uint64_t. There is no need for extending it to 64-bit.

[Rot127]

@akihikodaki Would you mind open an issue about it? Just so we do not forget it and we can assign it to a milestone.

However I suggest to leave imm 32-bit instead of converting it to uint64_t. There is no need for extending it to 64-bit.

Will include this suggestion in #1949

[akihikodaki]

@akihikodaki Would you mind open an issue about it? Just so we do not forget it and we can assign it to a milestone.

Done: #2056