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

[Thiago Jung Bauermann <bauerman@br.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2429 bytes --]

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 #2: record_patch_comments.txt --]
[-- Type: text/plain, Size: 2360 bytes --]

> Signed-off-by: Hui Zhu <hui.zhu@windriver.com>

I believe this has no meaning outside of the Linux kernel community, so
there's no need to sign off here (this purpose is filled by the FSF
copyright assignment paperwork).

> +/*teawater rec begin----------------------------------------------------------*/
<snip>
> +/*teawater rec end------------------------------------------------------------*/

All these markers all have to go. They also make patch reviewing more
cumbersome, IMHO.

> +#define RECORD_SIZE__old_kernel_stat	32
> +#define RECORD_SIZE_tms	16
> +#define RECORD_SIZE_loff_t	8
<snip>
> +#define RECORD_SYS_SOCKET	1
> +#define RECORD_SYS_BIND		2
> +#define RECORD_SYS_CONNECT	3
<snip>
> +#define RECORD_SEMOP		1
> +#define RECORD_SEMGET		2
> +#define RECORD_SEMCTL		3
<snip>
> +#define RECORD_Q_GETFMT		0x800004
> +#define RECORD_Q_GETINFO	0x800005
> +#define RECORD_Q_GETQUOTA	0x800007
> +#define RECORD_Q_XGETQSTAT	(('5'<<8)+(5))
> +#define RECORD_Q_XGETQUOTA	(('3'<<8)+(3))

Comments should be added explaining the purpose of these constants, and
where the numbers come from.

> +#if 0
> +		if (override) {
> +			/* *addr += ((uint32_t)read_register (override)) << 4; */
> +			printf_unfiltered (_("record: cann't get the value of the segment register.\n"));
> +			return (-1);
> +		}
> +#endif

This should be removed. There are two other snippets of code which
are #ifed out and should go too.

> +/*teawater rec begin----------------------------------------------------------*/
> +	int (*intx80_record) (void);
> +	int (*sysenter_record) (void);
> +/*teawater rec end------------------------------------------------------------*/

The names above are specific to the x86 architecture. tdep members
should have a name which is meaningful accross different architectures.

>  SHELL = @SHELL@
> -EXEEXT = @EXEEXT@
> +#teawater rec begin-------------------------------------------------------------
> +EXEEXT = @EXEEXT@record
> +#teawater rec end---------------------------------------------------------------

This should be removed.

> +typedef struct	record_s {
> +	struct	record_s	*prev;
> +	struct	record_s	*next;
> +	int	type;	/* 1 reg 2 mem 0 end */
> +	union {
> +		/* reg */
> +		record_reg_t	reg;
> +		/* mem */
> +		record_mem_t	mem;
> +		/* end */
> +		int		need_dasm;
> +	} u;
> +} record_t;

I suggest changing type to an enum.