Re: [PATCH] arm reversible : progress <patch_1_phase_2>

[Tom Tromey <tromey at redhat dot 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