This is the mail archive of the gdb-patches@sources.redhat.com 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]

Re: S390 gdb patches


DJBARROW@de.ibm.com wrote:

> D.J. Barrow Linux for S/390 kernel developer
> eMail: djbarrow@de.ibm.com,barrow_dj@yahoo.com
> Phone: +49-(0)7031-16-2583
> IBM Germany Lab, Schönaicherstr. 220, 71032 Böblingen
> 
>   ------------------------------------------------------------------------

The GDB group is not able to comment on the following:

>    ChangeLog.301000.bfd    Type: unspecified type (application/octet-stream)
>    ChangeLog.301000.opcodes    Type: unspecified type (application/octet-stream)
>    gdb.s390.301000.binutils.config.diff    Type: unspecified type (application/octet-stream)
>    gdb.s390.301000.binutils.core.diff    Type: unspecified type (application/octet-stream)
>    gdb.s390.301000.binutils.makefile.diff    Type: unspecified type (application/octet-stream)
>    ChangeLog.301000.include    Type: unspecified type (application/octet-stream)
>    gdb.s390.301000.config.diff    Type: unspecified type (application/octet-stream)

The change:

>    gdb.s390.301000.readline.config.diff    Type: unspecified type (application/octet-stream)

is too small to need any sort of copyright transfer.  You've CC'd the
bash maintainer so I'm assuming that it is approved.

It lacks a ChangeLog entry.


The changes:

>    ChangeLog.301000.gdb    Type: unspecified type (application/octet-stream)
>    ChangeLog.301000.gdbroot    Type: unspecified type (application/octet-stream)
>    gdb.s390.301000.core.diff    Type: unspecified type (application/octet-stream)

are (unfortunatly) still not approved.

Going through the changes:

gdb/ChangeLog et.al.

	Please format your ChangeLog entries in
	a manner that is consistent with the GNU
	coding style.

	Reviewers will typically dismiss out-of-hand
	any submition that doesn't include a reasonable
	ChangeLog entry.

	This is because those reviewing/committing the
	code have enough things to do without writing
	everyone elses ChangeLog entries.

	
coding standard

	The submitted code does not comply with
	the GNU/GDB coding standards.  In particular
	the folloing problems are common.

	Embedded tabs/spaces in statements. The decl
	  +extern int       watchAreaCnt;
	is wrong.

	Function declarations with the function name at
	the start of the line instead of on the same line
	as the return type.  vis:
	  extern int
	  s390_insert_watchpoint(...)
	vs (correct):
	  extern int s390_insert_watchpoint (...);

	Incorrect formatting of function calls - the
	lparen should be preceeded by a space.  The
	comma should be followed by a space vis:
	  foo(a,b,c);
	vs (correct)
	  foo (a, b, c);

	Bad choice of variable names.  Variables and
	functions should be in lowercase with a ``_''
	separating each word. Vis:
	  watchAreaCnt;
	vs correct:
	  watch_area_count;

	Lack of spaces in statemets.  Especially if
	and assignment.
	Wrong:
	  if(((...
	right:
	  if (((...
	wrong:
	  return(-1);
	right
	  return -1;
	wrong:
	  instrlen=s390_instrlen
	right:
	  instrlen=s390_instrlen

	Name of function needs to be at the start
	of a line in a function definition.
	wrong:
	  void foo(void)
	  {...
	right:
	  void
	  foo (void)
	  {...
	    
	You can assume ISO-C so things like:
	  #ifdef __STDC__
	are no longer needed.

	Please do not define or use either:
	  +#define TRUE 1
	or
	  +#define FALSE 0

	The GNU coding standard puts operators
	at the start (and not the end of the line)
	when an expression is spread across several
	lines (no matter how stupid you think this
	looks :-).  Wrong:
	  (xxxx &&
	   yyyy)
	right:
	  (xxxx
	    && yyyy)

	The closing ``*/'' on a multi-line comment
	is on the last line.  Wrong:
	  /* first line
	     second line
	  */
	right:
	  /* First line
	     second line */

	__u8 et.al. are used liberally.  Within GDB
	I believe that ``bfd_byte'' is used.


Files taken from Linux kernel?

	I'm not clear on the copyright status of the files:
		s390-gdbregs.h
		s390-regs-common.h
		sigcontext.h
	which were lifted from the Linux kernel.

	Does IBM currently control the copyright on those
	files?

	More generally, other targets haven't found it
	necessary to include these files in their GDB
	port.  Is it really necessary?


s390-tdep.c vs s390-tdep-multiarch.c

	There should only be one file: s390-tdep.c.


tm-s390.h

	If I'm reading the code right, much of this
	file is conditional on WANT_S390_TGT_DEFS.

	That code should be deleted.

s390-tdep.c includes <netinet/in.h>

	I can't see any reason for this (well except
	to get at the htonl() macros and you shouldn't
	be using them).

s390-tdep.c and #if TDEP_DEBUG

	There is no reason to conditionally compile
	in debug code.  Instead always include the code
	adding a ``set debug s390'' flag.  See mips-tdep.c
	for an example.

	Debug output must be written using the gdb_stdlog
	object and _not_ printf().

s390-tdep.c and ``volatile union ... u''

	The constructs:
	  volatile union
	  {
	    double double_arg;
	    long long long_long_arg;
	  } u; 
	and:
	  u.double_arg=*((float *)VALUE_CONTENTS(arg))
	are extreemly non-portable.

	The test ``len==sizeof(float)'' is the same.
	You should use TARGET_???_BIT macros.

gdbarch.sh:

	I'll figure out what macros you needed and multi
	arch them my self (avoid assignment :-)

DWARF2_REG_TO_REGNUM & dwarf2read.c 

	I suspect this is a good change.  However,
	it will need to be submitted as a separate
	patch so it can be reviewed by the symtab
	maintainers.

If you can address the coding standard problems then it should be
possible to accept most of the GDB changes.

	Andrew

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