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

Pedro Alves
2014-08-27T10:05:30Z

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