Re: [PATCH] arm reversible : <phase_2_complete>

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Oza" == paawan oza <paawan1982@yahoo.com> writes:

Oza> PS: I have added REG_ALLOC and MEM_ALLOC macors to accumulate
Oza> xmallocs, but I am not sure whether to go for that macros or leave
Oza> the implementation of reg/mem allcoation scattered across code
Oza> calling xmalloc.

I saw one use of REG_ALLOC and none of MEM_ALLOC.
If you intend to just have a single use it is probably better to just
drop the macros.

But, maybe you intend to use these universally through the code.
That would be fine.

Oza> +  /* Enable process record */

Period plus two spaces at the end of a comment.

Oza> +  set_gdbarch_process_record(gdbarch, arm_process_record);

Space before an open paren.

There are a lot of small formatting problems in this patch.

Oza>  #include "features/arm-with-m.c"
 
Oza> +
Oza>  static int arm_debug;

Adding a blank line for no reason.

Oza> +/* arm-reversible process reacord data structures.  */

Surely it should be "ARM".

Oza> +struct arm_mem_r
Oza> +{
Oza> +    uint32_t len;
Oza> +    CORE_ADDR addr;
Oza> +};

Wrong formatting.  The struct needs documentation and so do its fields.

Oza> +
Oza> +static int
Oza> +SBO_SBZ (uint32_t insn, uint32_t bit_num, uint32_t len, uint32_t sbo)

A new function needs a documentation comment.
Uppercase function names need special pleading.

Oza> +  uint32_t ONES = bits (insn, bit_num - 1, (bit_num -1) + (len - 1));

Likewise locals.

Oza> +static int 
Oza> +arm_handle_data_proc_misc_load_str_insn   \
Oza> +(insn_decode_record *arm_insn_r)

Bad formatting.

Oza> +  union
Oza> +    {
Oza> +      uint32_t s_word;
Oza> +      gdb_byte buf[4];
Oza> +    } u_buf[2];

There are lots of these little unions in the code.
>From what I can tell, this is mostly to read target memory into the
array, then convert to a scalar like:

Oza> +      GET_REG_VAL (reg_cache, reg_src1, &u_buf[0].buf[0]);
[...]
Oza> +      arm_insn_r->arm_mems[1].addr = u_buf[0].s_word;

It seems to me that extract_typed_address or something similar would be
better.

I'm not very familiar with the target record stuff; but if it is
possible to cross-debug and use it, then this code does not seem to
account for host/target endian differences.

Oza> +       /* SPSR is going to be changed.  */
Oza> +       /* Oza: FIX ME ? how to read SPSR value?  */

There are a few comments in this style.

First, don't put your name into comments.

Second, I think instead of new "FIXME" comments, new code should either
just work, or call error and have an explanatory comment.


I can't really comment on the details of the implementation.  I don't
know much about either ARM or process record.  It seems basically
reasonable; I prefer a somewhat more symbolic style, but I understand
that the x86 process record code is also written with many manifest
constants.

Tom