Re: [PATCH v3 1/6] Implements support for recording arm/thumb mode coprocessor instructions

[Pedro Alves <palves at redhat dot com>]

Hi Omair,

On 08/27/2014 10:07 AM, Omair Javaid wrote:
> On 13 August 2014 19:10, Will Newton <will.newton@linaro.org> wrote:
>> On 13 August 2014 14:12, Omair Javaid <omair.javaid@linaro.org> wrote:
>>> gdb:
>>>

>From the nit department:

>>> 2014-08-13  Omair Javaid  <omair.javaid@linaro.org>
>>>
>>>         * arm-tdep.c (arm_record_coproc_data_proc): Updated.

please use imperative, present tense in logs.  Also in the $subject
line when that gets into the commit log: s/Implements/Implement/.

And, updated how?  What changed?  Please write something
like:

	* arm-tdep.c (arm_record_coproc_data_proc): Handle foo instructions.

with 'foo' replaced, of course.  :-)

>>>         (arm_record_asimd_vfp_coproc): Added record handler for asimd, vfp
>>>         and coprocessor insn.

'Add'.

>>>         (thumb2_record_coproc_insn): New function.
>>>         (thumb2_record_decode_insn_handler): Updated.

Likewise, updated how?

>>>         (decode_insn): Updated.

Say something like:
	
	(decode_insn): Install arm_record_asimd_vfp_coproc as handler for
	opcode 110 insns.

>>> +/* Handling opcode 110 insns.  */
>>> +
>>> +static int
>>> +arm_record_asimd_vfp_coproc (insn_decode_record *arm_insn_r)
>>> +{
>>> +  uint32_t op, op1, op1_sbit, op1_ebit, coproc;
>>> +
>>> +  coproc = bits (arm_insn_r->arm_insn, 8, 11);
>>> +  op1 = bits (arm_insn_r->arm_insn, 20, 25);
>>> +  op1_sbit = bit (arm_insn_r->arm_insn, 24);
>>> +  op1_ebit = bit (arm_insn_r->arm_insn, 20);
>>> +  op = bit (arm_insn_r->arm_insn, 4);
>>
>> op abd op1_sbit appear to be unused.

> Ping! Kindly provide your feedback and help me approve this patch series.

I didn't see a reply to Will's comment.  Should op1_sbit haven been used?

This patch is OK once Will is happy with it.

Thanks,
Pedro Alves