This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] arm reversible : <phase_2_complete>

I did not review the 2011-11-19 nor 2011-11-09 patch.

> Yeah, it is a bit of a pain.  I wish we had a tool that could reformat
> the code according to our standards.

I guess 'indent' could do that. Its default settings formats using GNU
Some editors can be configured to automatically format new lines and
have a button to reformat existing lines. I suppose no editor can
autodetect the style from file yet.

Review of the 2011-11-03 patch:

In arm_process_record():
Expression `insn_id = bits (arm_record.arm_insn, 11, 15);' uses value
of `arm_record.arm_insn'
which is always zero. When you moved the test from decode_insn() I
guess forgot to copy these lines:
+      arm_record->arm_insn = (uint32_t) extract_unsigned_integer
+           THUMB_INSN_SIZE_BYTES, gdbarch_byte_order (arm_record->gdbarch));

Remaining from previous reviews:

I suggested INSN_IS_RECORDED to make it clear it is boolean, Tom
Tromey did not comment on that, Oza left the name as is. Well, it is
not a big deal.
However arguments of macros should be surrounded by parentheses - like this:
This is necessary if INSN_RECORDED() is passed more complex expression
when the order of evaluation matters. This is a common C issue and
customary solution, it is not GDB specific.

> Petr >>In arm_record_strx()
> Petr >>Why don't you use `record_buf_mem[0]' and `record_buf_mem[1]' syntax?
> because calling function takes care of REG_ALLOC routine calls.
> I did not want ALLOC calls to be scattered into any other function
> other than main decoding functions.

The lines can be converted from `*(record_buf_mem + 1)' to
`record_buf_mem[1]' with no changes to REG_ALLOC() calls. The
allocation calls would remain unscattered in main decoding functions.
The code is correct, but I do not understand why you use the unusual

Have you considered adding the assertions I suggested in [2]? They are
not necessary but they improve understanding of code.


Petr Hluzin

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]