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

[paawan oza <paawan1982 at yahoo dot com>]

Hi Tom and Michael,

First of all; thanks for your comments.


1)   diff will be changed as you suggested.
2)   coding guidelines will be further more corrected.
3)   function will be made static and prototype will be removed.
4)  casting problems and space problems will be corrected.

Tom: >> 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.


Oza: if you see x86 record code; it doesnt look to be sharing many things.
same case with arm because the way we need to decode the insn for record-replay 
and extract registers and memory and 

linux ABI; probably sharing code is much more clumsy and unclean than writing 
few K new lines.

may I have some idea about QEMU based approach ?
I did not get your hint to incorporate into gdb record-replay.

PS: if somebody could clarify.
1) what is the API to read arm coprocessor registers ?
2) how to read SPSR value ?
3) there are couple of FIX me in code.

Regards,
Oza.


----- Original Message ----
From: Tom Tromey <tromey@redhat.com>
To: paawan oza <paawan1982@yahoo.com>
Cc: "gdb@sourceware.org" <gdb@sourceware.org>
Sent: Fri, March 4, 2011 12:02:44 AM
Subject: Re: [PATCH] arm reversible : progress <patch_1_phase_2>

>>>>> ">" == 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