Re: GDB record patch 0.1.3 for GDB-6.8 release (It make I386-Linux GDB support Reversible Debugging)

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

Hi,

On Wed, 2008-04-16 at 15:00 +0800, Hui Zhu wrote:
> GDB record patch make GDB support Reversible Debugging.
> It make GDB disassemble the instruction that will be executed to get which memory and register will be changed and record them to record all program running message. Through these on the use of this information to achieve the implementation of the GDB Reversible Debugging function.

Thanks for the patch! Do you have already the FSF copyright assignment
in place? You will need it for a significant patch like this to get in.

I didn't have a detailed look at it yet, so these are general comments
and initial issues that in my opinion need to be adressed before the
meat of the patch gets discussed.

First, you need to make your code conform to the GNU Coding Standard
(http://www.gnu.org/prep/standards/html_node/Writing-C.html) so that it
blends uniformly with the rest of the GDB code. Especially regarding
indentation (use two spaces per level) and curly bracket positioning.

Also, there's almost no comments in the code, which makes it hard to
to review the patch for inclusion (that's basically why I didn't look
at it in more detail yet). There should be a comment for each function
you add to GDB (unless it's very small and obvious, I think) describing
its purpose, arguments and return value. And a comment for each
structure you add, describing its purpose and that of its elements.
Tricky or subtle parts of the code also benefit from some comments
explaining why they are needed, or the assumptions they make. Also, in
the beginning of gdb/record.c there should be a big comment explaining
the general idea and workings of the record mechanism. This will also
help us when reviewing the patch, and discuss the approach and its pros
and cons.

Several places in your patch use current_gdbarch. GDB is currently
moving away from it, and there are folks actively working on removing
it 
entirely. The reason is that a single program could have more than one
architecture (see the Cell processor and its PPU and SPUs for example).
I think you will need help to decide where to get a valid gdbarch from,
depending on the context that you need it.

I hope this is enough to get the ball rolling. :-)

I attached some comments on specific parts of the patch (man, I think I
need to start using mutt for reading mailing lists).
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

Attachment: record_patch_comments.txt
Description: Text document