Re: [PATCH] arm reversible : <phase_2_complete>

[Petr HluzÃn <petr dot hluzin at gmail dot com>]

On 15 October 2011 05:45, paawan oza <paawan1982@yahoo.com> wrote:
> please find the patch below.

I did not verify the style guidelines (spaces etc.), I am not familiar enough.
However some places have a comma ',' at the beginning of new line - I
think the "operator at newline" guideline does not apply to commas.

I did not check ARM semantics. It looks plausible, though.

The current patch (2011-10-15) is definitely an improvement to 2011-05-12.
Only the assertion thing got worse:

In arm_handle_ld_st_imm_offset_insn()
>+   switch (arm_insn_r->opcode)
In arm_handle_ld_st_reg_offset_insn():
>+   switch (arm_insn_r->opcode)
>+   switch (offset_12)
>+   switch (arm_insn_r->opcode)
In arm_handle_ld_st_multiple_insn()
>+   switch (addr_mode)

These switches seem to have `return -1;' in cases which are not
reachable, therefore shoudl have gdb_assert_not_reached().
The guideline - which I think Tom was reffering to - is that
impossible states and coding bugs in gdb should trigger assertions
however user's input (no matter how malformed) should trigger warning
or error messages.
(This would mean to turn the lines with `default:' to the previous
revision. I understand this sucks.)

Some situations are difficult to decide whether they are trigger-able
by user input or not.
If my code is not coded or intended to handle such situations I prefer
to kill the process (or whatever are assertions configured to do) and
get feedback from user.
I am not familiar with GDB customs, though. Tom?

Oza> +      gdb_assert_not_reached ("no decoding pattern found");
>
Tom> It seems wrong to use an assert in this code.  At least, it is not
Tom> obvious to me that this represents a logic error in gdb as opposed to a
Tom> merely unrecognized instruction.  An unrecognized instruction can occur
Tom> for many reasons, e.g., a bad jump.

The switch variable `arm_insn_r->opcode' cannot be initialized to any
value different from 0..15 because of the expression:
arm_insn_r->opcode = bits (arm_insn_r->arm_insn, 21, 24).
The switch variable `offset_12' cannot be initialized to any value
different from 0..3 because of the expression: offset_12 = bits
(arm_insn_r->arm_insn, 5, 6).
The switch variable `addr_mode' cannot be initialized to any value
different from 0..3 because of the expression: addr_mode = bits
(arm_insn_r->arm_insn, 23, 24).
It would be bit easier to see that if the variables were local (as I suggested).
Other values are not possible to create, even with corrupted input or
unrecognized instructions.

Paawan: If Tom and I give you contradicting suggestions then you
should complain.


Issues remaining from my previous reviews:

In arm_handle_coproc_data_proc_insn()
+   if (15 == arm_insn_r->opcode)
...
+        /* Handle arm syscall insn.  */
+        if (tdep->arm_swi_record != NULL)
+          {
+            tdep->arm_swi_record(reg_cache);
+          }
...
+   return -1;

The function still returns -1 even when the instruction was recognized
(15) and OS syscall support function is not NULL.
Yes, there is no way to set it to non-NULL value yet but when it is
implemented then you would have to do this change anyway:
-   tdep->arm_swi_record(reg_cache);
+   return tdep->arm_swi_record(reg_cache);

I guess the function should use `arm_insn_r' argument to record the
changes - which is missing.
In thumb_handle_swi_insn() the situation is the opposite: it returns 0
no matter what the arm_swi_record() returns.


In arm_handle_data_proc_misc_ld_str_insn()
>> +  struct
>> +    {
>> +      ULONGEST unsigned_regval;
>> +    } u_buf[2];
>>
>> You can get the same result (and simpler code) with
>> ULONGEST u_buf[2];
>> or maybe also better name with
>> ULONGEST u_regvals[2];
>>
>> The same applies to other functions.
>
> Oza: It is correct, it was mis-evolved as inistially it was union with 2 members
> and I fixed Tomâs review comments for endianness. I will change this, but pelase
> do not mind if it is not there in the immediate versions of patch, eventually
> after testing it will be changed.

This is still true.


> Oza: please report as I donât have any automated tool which reports them, if it
> is not effort for you please report all if possible. Gcc also give some warning
> about unused one, but not sure about gdb warning suppression.

Unused local variables, as requested:
arm_handle_ld_st_imm_offset_insn: reg_src2, immed_high, immed_low
arm_handle_ld_st_reg_offset_insn: immed_high, immed_low
arm_handle_ld_st_multiple_insn: shift_imm, reg_src2
arm_handle_coproc_data_proc_insn - all of them: shift_imm, reg_src1,
reg_src2, addr_mode, start_address
thumb_handle_ld_st_imm_offset_insn: reg_val1
thumb_handle_ld_st_stack_insn: reg_val1
thumb_handle_misc_insn: reg_val1, reg_src1 - write-only in case `(2 ==
opcode)', immed_8, immed_5
thumb_handle_swi_insn: reg_val1
thumb_handle_branch_insn: reg_val1, reg_src1, opcode, immed_5

Typos:
preccedded -> precceded (3x), Accorindly -> Accordingly (2x),
addresing -> addressing, optmization -> optimization, Wihtout
optmization  -> Without optimization,

Otherwise the patch looks good.

-- 
Petr Hluzin