This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: S390 gdb patches
- To: DJBARROW at de dot ibm dot com
- Subject: Re: S390 gdb patches
- From: Andrew Cagney <ac131313 at cygnus dot com>
- Date: Tue, 21 Nov 2000 05:06:52 +1100
- Cc: gdb-patches at sourceware dot cygnus dot com, BOAS at de dot ibm dot com,gaburke at us dot ibm dot com, s390-patches at gnu dot org, bug-binutils at gnu dot org,binutils at sourceware dot cygnus dot com, bfd-local at cygnus dot com,bash-maintainers at gnu dot org
- References: <C1256988.0053884D.00@d12mta09.de.ibm.com>
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