Re: [PATCH v2 5/7] Adds support for thumb32 instructions recording

Pedro Alves
2014-01-06T19:52:31Z

On 01/03/2014 07:15 PM, Omair Javaid wrote:
> + switch (op1)
> + {
> + /* Store byte instructions. */
> + case 4:
> + case 0:
> + record_buf_mem[0] = 1;
> + break;

"break;" should be aligned with the above statement,
not with "case". Several instances of this.

> + /* Swap first half of 32bit thumb instruction with second half. */
> + arm_record->arm_insn = (arm_record->arm_insn >> 16) |
> + (arm_record->arm_insn << 16);

'|' goes on the next line. But then you'd need to wrap the whole
thing with an extra set of parens. Write instead:

arm_record->arm_insn
= (arm_record->arm_insn >> 16) | (arm_record->arm_insn << 16);

> +
> + insn_id = thumb2_record_decode_insn_handler (arm_record);
> +
> + if (insn_id != ARM_RECORD_SUCCESS)
> + {
> + arm_record_unsupported_insn(arm_record);

Missing space before parens.

> + ret = -1;
> + }
> }
> else

Otherwise looks fine to me.

Thanks,
--
Pedro Alves