Re: [PATCH] arm reversible : <phase_2_complete>

[paawan oza <paawan1982 at yahoo dot com>]

Hi Tom, Petr and Yao,

I am working on following:

1) petr's comments, which I am planning to make new function now.

2) yao's comment on Changelog detailed stuff which I shall prepare seeing the guidelines.

3) Tom's comments are already fixed ?(hope there are not much formatting issues).

will post the patch in a day or so.?
Thanks for your comments to make this patch look ok.

Regards,
Oza.


________________________________
From: Petr Hluzín <petr.hluzin@gmail.com>
To: oza Pawandeep <oza.pawandeep@gmail.com> 
Cc: Tom Tromey <tromey@redhat.com>; paawan oza <paawan1982@yahoo.com>; "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>; chandra krishnappa <chandra_roadking@yahoo.com> 
Sent: Sunday, 4 December 2011 2:00 AM
Subject: Re: [PATCH] arm reversible : <phase_2_complete>

On 3 December 2011 20:01, oza Pawandeep <oza.pawandeep@gmail.com> wrote:
> Hi Tom and Petr,
>
> This patch includes both of your comments; I have worked both on
> formatting and comments, and try to make the patch look ok.
> the patch is derived from gdb-7.3.50.20111203 current snapshot.
>

In function decode_insn:
> +
> + ?struct
> + ? ?{
> + ? ? ?gdb_byte buf[insn_size];
> + ? ?} u_buf;
> +
> + ?uint32_t ret=0, insn_id = 0;
> +
> + ?memset (&u_buf, 0, sizeof(u_buf));
> + ?if (target_read_memory (arm_record->this_addr, &u_buf.buf[0], insn_size))

I wonder why is there a `struct u_buf'. Having local variable
`buf[insn_size];' would be sufficient and obvious. I am sorry to not
discover that earlier. The same thing applies to arm_process_record().

In
arm_process_record ()
> +
> + ?struct
> + ? ? ?{
> + ? ? ? ?gdb_byte buf[2];
> + ? ? ?} u_buf;
> +
> + ?...
> +
> + ?arm_record.arm_insn = (uint32_t) extract_unsigned_integer (&u_buf.buf[0],
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? THUMB_INSN_SIZE_BYTES ,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gdbarch_byte_order (arm_record.gdbarch));

Well, when I said that you probably forgot to copy
extract_unsigned_integer() I should have also said that you should
have also copied the line
target_read_memory (arm_record->this_addr, &u_buf.buf[0], insn_size)
Right now extract_unsigned_integer() reads an uninitialized buffer. :-T

-- 
Petr
Hluzin