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

[Tea <teawater@gmail.com>]

Hi Thiago,

I am Hui Zhu. I am at home so I use the Email of myself.

Thanks for your mail very much. It help me a lot. I am beginning to
modify the patch according to your mail.
I am not very clear about " FSF copyright assignmentin place".Could
you tell me something about it?
 BTW: I didn't have any experience on this work. I will have a lot of
questions about it. Please help me. Thanks a lot.

Thanks,
Teawater

On Thu, Apr 17, 2008 at 11:27 PM, Thiago Jung Bauermann
<bauerman@br.ibm.com> wrote:

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