Re: glibc 2.1.92 patch for S/390.

[Andreas Jaeger <aj at suse dot de>]

Hi Martin,

>>>>> Martin Schwidefsky  writes:

 > Hi,
 > I finally have the new glibc working on S/390. To make old binaries work,
 > I had to copy a lot of these lovely compatibility files from i386. Luckily
 > almost all of the i386 files are perfectly suitable for s390 too.
 > I discussed the Versions issue with some of my collegues here in the lab
 > and we came to the conclusion that we want to have ONE libc for all of the
 > binaries - old and new ones. Since the glibc-2.1.3 had a Versions file with
 > GLIBC_2.0 symbols the current Versions file still contains these.

 > I hope that our mail gateway won't eat my patch for lunch...

It ate some too long lines.  Could you try to generate a proper
attachment, please?

 > 2000-08-21  Martin Schwidefsky  <schwidefsky@de.ibm.com>

 >      * shlib-versions: Added a rule for S/390 to the libm version list.
 >      * sysdeps/s390/__longjmp.c: Removed unused variable result.
 >      * sysdeps/s390/fpu/bits/fenv.h: Added FPC_*_MASK definitions.
 >      * sysdeps/s390/fpu/fegetenv.c: New file.
 >      * sysdeps/s390/fpu/feholdexcpt.c: New file.
 >      * sysdeps/s390/fpu/fesetenv.c: New file.
 >      * sysdeps/s390/fpu/fesetround.c: Reformatted.
 >      * sysdeps/s390/fpu/feupdateenv.c: New file.
 >      * sysdeps/s390/fpu/fgetexcptflg.c: Reformatted.
 >      * sysdeps/s390/fpu/fpu_control.h: Corrected header.
 >      * sysdeps/s390/fpu/fraiseexcpt.c: New file.
 >      * sysdeps/s390/fpu/fsetexcptflg.c: New file.
 >      * sysdeps/s390/fpu/ftestexcept.c: New file.
 >      * sysdeps/s390/fpu/libm-test-ulps: New file.
 >      * sysdeps/s390/gmp-mparam.h: Added end of comment.
 >      * sysdeps/s390/initfini.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/Dist: Added oldgetrlimit64.c and
 >      sys/procfs.h.
 >      * sysdeps/unix/sysv/linux/s390/Makefile: Removed sys/reg.h and
 >      added oldgetrlimit64.
 >      * sysdeps/unix/sysv/linux/s390/Versions: New file.
 >      * sysdeps/unix/sysv/linux/s390/alphasort64.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/bits/stat.h: New file.
 >      * sysdeps/unix/sysv/linux/s390/chown.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/fxstat.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/getdents64.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/getmsg.c: Removed.
 >      * sysdeps/unix/sysv/linux/s390/getpmsg.c: Removed.
 >      * sysdeps/unix/sysv/linux/s390/getrlimit.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/getrlimit64.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/lchown.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/lxstat.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/oldgetrlimit64.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/putmsg.c: Removed.
 >      * sysdeps/unix/sysv/linux/s390/putpmsg.c: Removed.
 >      * sysdeps/unix/sysv/linux/s390/readdir64.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/readdir64_r.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/scandir64.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/setrlimit.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/sigaction.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/sys/elf.h: Moved elf definitions to
 >      sys/procfs.h as proposed by Mark Kettenis.
 >      * sysdeps/unix/sysv/linux/s390/sys/procfs.h: New file.
 >      * sysdeps/unix/sysv/linux/s390/syscalls.list: New file.
 >      * sysdeps/unix/sysv/linux/s390/versionsort64.c: New file.
 >      * sysdeps/unix/sysv/linux/s390/xstat.c: New file.
[...]
 > diff -u -r --new-file libc/sysdeps/s390/fpu/bits/fenv.h
 > libc-s390/sysdeps/s390/fpu/bits/fenv.h
 > --- libc/sysdeps/s390/fpu/bits/fenv.h    Wed Aug 16 16:12:09 2000
 > +++ libc-s390/sysdeps/s390/fpu/bits/fenv.h    Mon Aug 21 12:45:13 2000
 > @@ -23,6 +23,14 @@
 >  # error "Never use <bits/fenv.h> directly; include <fenv.h> instead."
 >  #endif

 > +/*
 > + * Definitions from asm/s390-regs-common.h that are needed in th glibc.
 > + */
 > +#define FPC_DXC_MASK            0x0000FF00
 > +#define FPC_EXCEPTION_MASK      0xF8000000
 > +#define FPC_FLAGS_MASK          0x00F80000
 > +#define FPC_RM_MASK             0x00000003
 > +
You're violating the namespace here.  If you need those defines only
internally, add an extra file and include it where needed - see for
example sysdeps/alpha/fpu/fenv_libc.h.

 >  /* Define bits representing the exception.  We use the bit positions
 >     of the appropriate bits in the FPU control word.  */
 >  enum
 > diff -u -r --new-file libc/sysdeps/s390/fpu/fegetenv.c
 > libc-s390/sysdeps/s390/fpu/fegetenv.c
 > --- libc/sysdeps/s390/fpu/fegetenv.c     Thu Jan  1 01:00:00 1970
 > +++ libc-s390/sysdeps/s390/fpu/fegetenv.c     Mon Aug 21 12:45:13 2000
 > @@ -0,0 +1,44 @@
 > +/* Store current floating-point environment.
 > +   Copyright (C) 2000 Free Software Foundation, Inc.
 > +   This file is part of the GNU C Library.
 > +   Contributed by Denis Joseph Barrow (djbarrow@de.ibm.com)
One nit: add a period to finish the sentence.
 > +
 > +   The GNU C Library is free software; you can redistribute it and/or
 > +   modify it under the terms of the GNU Library General Public License as
 > +   published by the Free Software Foundation; either version 2 of the
 > +   License, or (at your option) any later version.
 > +
 > +   The GNU C Library is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 > +   Library General Public License for more details.
 > +
 > +   You should have received a copy of the GNU Library General Public
 > +   License along with the GNU C Library; see the file COPYING.LIB.  If
 > not,
Upps, why is this wrapped around?  This occurs in a number of places.:-(
 > +   write to the Free Software Foundation, Inc., 59 Temple Place - Suite
 > 330,
 > +   Boston, MA 02111-1307, USA. */
 > +
 > +#include <fenv.h>
 > +#include <fpu_control.h>
 > +#include <stddef.h>
 > +#include <asm/ptrace.h>
 > +#include <sys/ptrace.h>
 > +#include <unistd.h>
 > +
 > +int fegetenv (fenv_t *envp)
Please follow the coding conventions, they're described in the info
file "Standards (GNU coding standards)":
int
fegetenv (fenv_t *envp)

 > +{
 > +  /*
 > +   * The S/390 IEEE fpu doesn't keep track of the ieee instruction
 > pointer.
 > +   * To get around that the kernel will store the address of the last
 > +   * fpu fault to the process structure. This ptrace call reads this value
 > +   * from the kernel space. That means the ieee_instruction_pointer is
 > +   * only correct after a fpu fault. That's the best we can do, there is
 > +   * no way to find out the ieee instruction pointer if there was no
 > fault.
 > +   */
Make comments of the form - without the extra stars at the left and
with two spaces after periods and before the closing */
/* bla bla .... bla.  */
 > +  _FPU_GETCW(envp->fpc);
 > +  envp->ieee_instruction_pointer =
 > +    ptrace(PTRACE_PEEKUSER, getpid(), PT_IEEE_IP);
Please one space before each opening brace.
 > +
 > +  /* Success.  */
 > +  return 0;
 > +}
 > diff -u -r --new-file libc/sysdeps/s390/fpu/feholdexcpt.c
 > libc-s390/sysdeps/s390/fpu/feholdexcpt.c
 > --- libc/sysdeps/s390/fpu/feholdexcpt.c  Thu Jan  1 01:00:00 1970
 > +++ libc-s390/sysdeps/s390/fpu/feholdexcpt.c  Mon Aug 21 12:45:13 2000
 > @@ -0,0 +1,35 @@
 > +/* Store current floating-point environment and clear exceptions.
 > +   Copyright (C) 2000 Free Software Foundation, Inc.
 > +   This file is part of the GNU C Library.
 > +   Contributed by Denis Joseph Barrow (djbarrow@de.ibm.com)
 > +
 > +   The GNU C Library is free software; you can redistribute it and/or
 > +   modify it under the terms of the GNU Library General Public License as
 > +   published by the Free Software Foundation; either version 2 of the
 > +   License, or (at your option) any later version.
 > +
 > +   The GNU C Library is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 > +   Library General Public License for more details.
 > +
 > +   You should have received a copy of the GNU Library General Public
 > +   License along with the GNU C Library; see the file COPYING.LIB.  If
 > not,
 > +   write to the Free Software Foundation, Inc., 59 Temple Place - Suite
 > 330,
 > +   Boston, MA 02111-1307, USA. */
 > +
 > +#include <fenv.h>
 > +#include <fpu_control.h>
 > +
 > +int feholdexcept (fenv_t *envp)
 > +{
 > +  /* Store the environment.  */
 > +  fegetenv(envp);
 > +  /* Clear the current sticky bits */
 > +  /* as more than one exception may be generated */
This way:
/* Clear the current sticky bits  as more than one exception may be generated.  */

or this one:
      /* Clear the current sticky bits as more than one exception may
         be generated.  */

 > +  envp->fpc &= ~(FPC_FLAGS_MASK | FPC_DXC_MASK);
 > +  /* Hold from generating fpu exceptions temporarily.  */
 > +  /* as the HP man pages suggest. */
Ignore the HP man page - please follow ISO C 99.

 > +  _FPU_SETCW((envp->fpc & ~(FE_ALL_EXCEPT << FPC_EXCEPTION_MASK_SHIFT)));
 > +  return 0;
 > +}
 > diff -u -r --new-file libc/sysdeps/s390/fpu/fesetenv.c
 > libc-s390/sysdeps/s390/fpu/fesetenv.c
 > --- libc/sysdeps/s390/fpu/fesetenv.c     Thu Jan  1 01:00:00 1970
 > +++ libc-s390/sysdeps/s390/fpu/fesetenv.c     Mon Aug 21 12:45:13 2000
 > @@ -0,0 +1,57 @@
 > +/* Install given floating-point environment.
 > +   Copyright (C) 2000 Free Software Foundation, Inc.
 > +   This file is part of the GNU C Library.
 > +   Contributed by Denis Joseph Barrow (djbarrow@de.ibm.com)
 > +
 > +   The GNU C Library is free software; you can redistribute it and/or
 > +   modify it under the terms of the GNU Library General Public License as
 > +   published by the Free Software Foundation; either version 2 of the
 > +   License, or (at your option) any later version.
 > +
 > +   The GNU C Library is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 > +   Library General Public License for more details.
 > +
 > +   You should have received a copy of the GNU Library General Public
 > +   License along with the GNU C Library; see the file COPYING.LIB.  If
 > not,
 > +   write to the Free Software Foundation, Inc., 59 Temple Place - Suite
 > 330,
 > +   Boston, MA 02111-1307, USA. */
 > +
 > +#include <fenv.h>
 > +#include <fpu_control.h>
 > +#include <stddef.h>
 > +#include <asm/ptrace.h>
 > +#include <sys/ptrace.h>
 > +#include <unistd.h>
 > +
 > +int fesetenv (const fenv_t *envp)
 > +{
 > +  fenv_t env;
 > +
 > +  if (envp == FE_DFL_ENV) {
 > +    env.fpc = _FPU_DEFAULT;
 > +    env.ieee_instruction_pointer = 0;
 > +  } else if (envp == FE_NOMASK_ENV) {
 > +    env.fpc = FPC_EXCEPTION_MASK;
 > +    env.ieee_instruction_pointer = 0;
 > +  } else
The braces on a single line.
if (envp == FE_DFL_ENV)
  {
    ...
  }
else if (...)
  {
  }

[some stuff removed - my comments might apply to the removed stuff]

 > diff -u -r --new-file libc/sysdeps/s390/fpu/fesetround.c
 > libc-s390/sysdeps/s390/fpu/fesetround.c
 > --- libc/sysdeps/s390/fpu/fesetround.c   Wed Aug  2 16:19:40 2000
 > +++ libc-s390/sysdeps/s390/fpu/fesetround.c   Mon Aug 21 12:45:13 2000
 > @@ -24,11 +24,10 @@
 >  int
 >  fesetround (int round)
 >  {
 > -  if ((round|FPC_RM_MASK) != FPC_RM_MASK)
 > -    {
 > -      /* ROUND is not a valid rounding mode.  */
 > -      return 1;
 > -    }
 > +  if ((round|FPC_RM_MASK) != FPC_RM_MASK) {
 > +    /* ROUND is not a valid rounding mode.  */
 > +    return 1;
 > +  }
No!  That patch is wrong according to the coding conventions.
 > diff -u -r --new-file libc/sysdeps/s390/fpu/feupdateenv.c
 > libc-s390/sysdeps/s390/fpu/feupdateenv.c
 > --- libc/sysdeps/s390/fpu/feupdateenv.c  Thu Jan  1 01:00:00 1970
 > +++ libc-s390/sysdeps/s390/fpu/feupdateenv.c  Mon Aug 21 12:45:13 2000
[...]
 > +
 > +/*
 > +  This implementation is quite different to Intel it is based on
 > +  my interpretation of the hp manpages for feholdenv & feupdateenv.
Remove this - and check the standard instead of some man page.

 > diff -u -r --new-file libc/sysdeps/unix/sysv/linux/s390/sigaction.c
 > libc-s390/sysdeps/unix/sysv/linux/s390/sigaction.c
 > --- libc/sysdeps/unix/sysv/linux/s390/sigaction.c  Thu Jan  1 01:00:00 1970
 > +++ libc-s390/sysdeps/unix/sysv/linux/s390/sigaction.c  Mon Aug 21 12:45:13

What's the difference between this version and the generic version?
You removed the __ASSUME_REALTIME_SIGNALS, in that case you can
further simplify (you seem to assume the rt interface), have a look at
the generic version.  Your version looks broken to me.


Martin, can you please reformat the code, check sigaction and then
send a complete new patch without those additional line breaks in it?

Thanks,
Andreas
-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.inka.de
    http://www.suse.de/~aj