Re: S390 gdb patches

[Andrew Cagney]

address@hidden wrote:

> D.J. Barrow Linux for S/390 kernel developer
> eMail: address@hidden,address@hidden
> 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