This is the mail archive of the
gdb@sourceware.org
mailing list for the GDB project.
Re: [PATCH] arm reversible : progress <patch_1_phase_2>
- From: Tom Tromey <tromey at redhat dot com>
- To: paawan oza <paawan1982 at yahoo dot com>
- Cc: "gdb\ at sourceware dot org" <gdb at sourceware dot org>
- Date: Thu, 03 Mar 2011 11:32:44 -0700
- Subject: Re: [PATCH] arm reversible : progress <patch_1_phase_2>
- References: <341905.10459.qm@web112513.mail.gq1.yahoo.com>
>>>>> ">" == paawan oza <paawan1982@yahoo.com> writes:
>> I am done with the deisng for both thumb and arm framework.
Thanks for your work on this.
>> having some doubts as of now.
I can't help with the ARM-specific stuff, but I have a few more general
comments.
>> diff -urN arm_new/arm-linux-tdep.c arm_orig/arm-linux-tdep.c
The patch is reversed.
>> - /* Enable process record */
>> - set_gdbarch_process_record(gdbarch, arm_process_record);
There are a bunch of minor coding style problems in the patch.
You'll have to fix them all before submitting.
In the quoted lines: the comment must end with a period and two spaces,
and there is a space missing before the open paren. That is, the code
should look like:
/* Enable process record. */
set_gdbarch_process_record (gdbarch, arm_process_record);
These two problems appear frequently.
>> -int arm_handle_data_proc_misc_load_str_insn (void*);
I did not read closely, but I think most or maybe all of the functions
declared here should be static. This applies to some other objects as
well, like arm_handle_insn.
Actually, in most cases, I think the code could be rearranged so that
you don't need forward declarations at all.
>> - else if(ARM_INSN_SIZE_BYTES == insn_size)
Space after the "if" -- this happens a lot.
>> - struct gdbarch_tdep *tdep = gdbarch_tdep ((struct gdbarch*) arm_insn_r-> gdbarch);
I think you don't need the cast here.
I think this occurs in a few places.
>> - struct regcache *reg_cache = (struct regcache*) arm_insn_r->regcache;
Or here. But if you needed something like this, there would have to be
a space before the "*".
>> - /* data processing insn /multiply sinsn */
>> - if(9 == arm_insn_r->decode)
One thing I dislike about the x86 record code is the huge number of
constants. I think this sort of thing is better if you use some
symbolic name instead.
One idea would be to share things with opcodes/arm-dis.c somehow.
I don't know if that is practical or not.
Actually, I have never understood why the process record stuff doesn't
share more code with the simulators. Maybe that is just too much work
somehow.
And, whatever happened to the QEMU-based replay approach? That seemed
promising to me.
>> - arm_insn_r->arm_regs = (uint32_t*)xmalloc (sizeof(uint32_t)*3);
I think it is a little clearer to use the libiberty macros, e.g.:
arm_insn_r->arm_regs = XNEWVEC (uint32_t, 3);
>> - uint32_t reg_val1=0,reg_val2=0;
Spaces around the "=" signs. This occurs in a few places.
Tom