Re: [PATCH 1/2] AArch64 glibc port

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Mon, 1 Oct 2012, Marcus Shawcroft wrote:

> Proposed ChangeLog.aarch64

Please send a revised patch to libc-ports for review.

> 2012-10-01  Marcus Shawcroft  <marcus.shawcroft@linaro.org>

I think there should be a sotruss-lib.c file for this port.

I don't see a get-rounding-mode.h file here, as I would expect to be 
needed given recent changes.

You also need to update the patch for the recent change

2012-10-02  Siddhesh Poyarekar  <siddhesh@redhat.com>

        * sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h: Fix clone
        flag name in comment to CLONE_CHILD_CLEARTID.

(at least).

>         * sysdeps/aarch64/fclrexcpt.c: New file.

Normally such files would be in the fpu/ directory.  (ARM is a special 
case because of the runtime detection of whether an FPU is present, but I 
don't see such an issue here.)

> +ifeq ($(subdir),debug)
> +CFLAGS-backtrace.c += -funwind-tables
> +CFLAGS-tst-backtrace2.c += -funwind-tables
> +CFLAGS-tst-backtrace3.c += -funwind-tables
> +CFLAGS-tst-backtrace4.c += -funwind-tables
> +CFLAGS-tst-backtrace5.c += -funwind-tables
> +CFLAGS-tst-backtrace6.c += -funwind-tables
> +endif

None of these tst-backtrace*.c files exist in glibc, so these CFLAGS 
definitions should not be present.

> +ENTRY (__longjmp)
> +#ifdef CHECK_SP
> +	ldr	x5, [x0, #JB_SP<<3]
> +	CHECK_SP (x5)
> +#endif

Such a conditional only makes sense if you are providing a .S version of 
____longjmp_chk.  But you don't seem to be doing so; instead you're using 
the generic C version, complete with a helper macro in jmpbuf-offsets.h.

> +#define __arch_compare_and_exchange_bool_8_acq(mem, newval, oldval) \
> +  (!__sync_bool_compare_and_swap ((mem), (oldval), (newval)))

GCC 4.7 and later have __atomic_* intrinsics with better control over 
barrier semantics.  Since you can assume a GCC version with that support 
for AArch64, you should use __atomic_* instead of the legacy __sync_*.

> +/* Define bits representing exceptions in the FPSR status word.  */
> +#define FE_INVALID     1
> +#define FE_DIVBYZERO   2
> +#define FE_OVERFLOW    4
> +#define FE_UNDERFLOW   8
> +#define FE_INEXACT    16

Until we've worked out the right way to fix bug 3439, so that the values 
remain in debug info as well as being usable in #if, please follow the 
same approach as in other versions of this header to defining these 
values; all architectures can then be fixed at once to conform to ISO C 
when bug 3439 is fixed.

> +/* Type representing floating-point environment.  */
> +typedef struct
> +  {
> +    unsigned int fpcr;
> +    unsigned int fpsr;
> +  }
> +fenv_t;

The names "fpcr" and "fpsr" are in the user's namespace; you need to use 
__fpcr and __fpsr here, as this is an installed header.

> +/* The GCC 4.6 compiler will define __FP_FAST_FMA{,F,L} if the fma{,f,l}
> +   builtins are supported.  */
> +# if __FP_FAST_FMA
> +#  define FP_FAST_FMA 1
> +# endif
> +
> +# if __FP_FAST_FMAF
> +#  define FP_FAST_FMAF 1
> +# endif
> +
> +# if __FP_FAST_FMAL
> +#  define FP_FAST_FMAL 1
> +# endif

Does the AArch64 architecture define that fma instructions are always 
available?  If so, you can just define FP_FAST_FMA and FP_FAST_FMAF 
unconditionally (and FP_FAST_FMAL never).  Certainly a comment referring 
to GCC 4.6 is inappropriate, since such a version doesn't exist for 
AArch64.

> +/* Use this to access DT_TLSDESC_PLT and DT_TLSDESC_GOT.  */
> +#ifndef ADDRIDX
> +#define ADDRIDX(tag) (DT_NUM + DT_THISPROCNUM + DT_VERSIONTAGNUM \
> +		      + DT_EXTRANUM + DT_VALNUM + DT_ADDRTAGIDX (tag))
> +#endif

It's now obsolete for ports to define their own ADDRIDX, VALIDX or 
VERSYMIDX macros, so remove this macro definition.

> diff -x .git -uNr glibc.orig/ports/sysdeps/aarch64/libm-test-ulps glibc/ports/sysdeps/aarch64/libm-test-ulps
> --- glibc.orig/ports/sysdeps/aarch64/libm-test-ulps	1970-01-01 01:00:00.000000000 +0100
> +++ glibc/ports/sysdeps/aarch64/libm-test-ulps	2012-09-28 12:55:43.000000000 +0100

This file appears out of date (e.g. no ulps listed for the clog tests I 
recently added).

> +Function: "sqrt":
> +ildouble: 1
> +ldouble: 1

If you have any ulps for sqrt, something is wrong.  Maybe this file is 
*very* out of date and was generated before my patch

2012-05-31  Joseph Myers  <joseph@codesourcery.com>

        * math/math.h (M_El): Use two more decimal places.
        (M_LOG2El): Likewise.
        (M_LOG10El): Likewise.
        (M_LN2l): Likewise.
        (M_LN10l): Likewise.
        (M_PIl): Likewise.
        (M_PI_2l): Likewise.
        (M_PI_4l): Likewise.
        (M_1_PIl): Likewise.
        (M_2_PIl): Likewise.
        (M_2_SQRTPIl): Likewise.
        (M_SQRT2l): Likewise.
        (M_SQRT1_2l): Likewise.

(which dealt with one possible cause of spurious sqrt ulps)?

> diff -x .git -uNr glibc.orig/ports/sysdeps/aarch64/soft-fp/sfp-machine.h glibc/ports/sysdeps/aarch64/soft-fp/sfp-machine.h
> --- glibc.orig/ports/sysdeps/aarch64/soft-fp/sfp-machine.h	1970-01-01 01:00:00.000000000 +0100
> +++ glibc/ports/sysdeps/aarch64/soft-fp/sfp-machine.h	2012-09-24 12:41:10.000000000 +0100

Missing FP_TRAPPING_EXCEPTIONS.  You should check all your files for any 
relevant changes to glibc since whenever you started the port....

> +aarch64.*-.*-linux.*	DEFAULT			GLIBC_2.16

Use GLIBC_2.17, since this port wasn't in 2.16.

> +#define PLTJMP(_x)	_x

As far as I can see, PLTJMP is only used in architecture-specific .S files 
(and other macros for use in such files), for a few architectures.  If an 
architecture has a null definition of it, there's no real point in having 
the macro existing at all - so I advise removing it, and all calls to it 
in AArch64 code.  Please check other internal macros to make sure there is 
actually some point to defining them.

> +# In order for unwinding to fail when it falls out of main, we need a
> +# cantunwind marker.  There's one in start.S.  To make sure we reach it, add
> +# unwind tables for __libc_start_main.
> +CFLAGS-libc-start.c += -fexceptions

I see no mention of "cantunwind" in start.S or elsewhere in the patch 
except here.  This looks like a relic of ARM EH, irrelevant to AArch64, 
which should be removed.

> +ifeq ($(subdir),io)
> +CFLAGS-creat.c += -fexceptions
> +CFLAGS-open.c += -fexceptions
> +CFLAGS-open64.c += -fexceptions
> +endif

Why is this needed?  If it's needed for some AArch64-specific reason, 
please add comments to explain.  If the issue is more generic, but latent 
on other platforms, send a patch to libc-alpha for a libc makefile with 
appropriate rationale.  (If there is a reason to need to build these 
functions with exceptions enabled, I'd think that need is generic, even if 
it only causes issues with the linux-generic versions of them, or 
something like that.)

> +ifeq ($(subdir),misc)
> +sysdep_routines += epoll_create epoll_wait inotify_init

ports/sysdeps/unix/sysv/linux/generic/Makefile has this; you shouldn't 
need to duplicate it.

> +CFLAGS-select.c += -fexceptions

Same comments as before apply.

> +ifeq ($(subdir),nptl)
> +CFLAGS-open.c += -fexceptions
> +CFLAGS-open64.c += -fexceptions
> +CFLAGS-pause.c += -fexceptions
> +CFLAGS-send.c += -fexceptions
> +CFLAGS-recv.c += -fexceptions
> +endif

Likewise.

> diff -x .git -uNr glibc.orig/ports/sysdeps/unix/sysv/linux/aarch64/configure.in glibc/ports/sysdeps/unix/sysv/linux/aarch64/configure.in
> --- glibc.orig/ports/sysdeps/unix/sysv/linux/aarch64/configure.in	1970-01-01 01:00:00.000000000 +0100
> +++ glibc/ports/sysdeps/unix/sysv/linux/aarch64/configure.in	2012-09-24 12:41:10.000000000 +0100
> @@ -0,0 +1,4 @@
> +GLIBC_PROVIDES dnl See aclocal.m4 in the top level source directory.
> +# Local configure fragment for sysdeps/unix/sysv/linux/aarch64.
> +
> +arch_minimum_kernel=2.6.39

No, please use 3.7.0 here as that's the actual version of the support 
going in.  Likewise in kernel-features.h.

> +GLIBC_2.16

Apart from updating the baselines to GLIBC_2.17, have you checked that the 
set of symbols exported (ignoring versions) agrees with other 64-bit 
platforms to the extent expected - that there's nothing accidentally 
missing?

> diff -x .git -uNr glibc.orig/ports/sysdeps/unix/sysv/linux/aarch64/nptl/not-cancel.h glibc/ports/sysdeps/unix/sysv/linux/aarch64/nptl/not-cancel.h

Why do you need this file rather than one of the generic versions - how 
does this differ from what's used for other linux-generic architectures?

> diff -x .git -uNr glibc.orig/ports/sysdeps/unix/sysv/linux/aarch64/syscalls.list glibc/ports/sysdeps/unix/sysv/linux/aarch64/syscalls.list

Why do you need this file?  It looks rather like it duplicates 
ports/sysdeps/unix/sysv/linux/generic/syscalls.list, and any entries that 
do duplicate that file should be avoided.

> +/* Don't use stime, even if the kernel headers define it.  We have
> +   settimeofday.  Similarly use setitimer to implement alarm.  */
> +#undef __NR_time
> +#undef __NR_umount
> +#undef __NR_stime
> +#undef __NR_alarm
> +#undef __NR_utime
> +#undef __NR_select
> +#undef __NR_readdir
> +#undef __NR_socketcall
> +#undef __NR_ipc

Not OK.  Get the kernel to define the set of syscall macros you want, and 
not to define any syscalls that aren't actually wanted.  No such #undef 
should need to be present for a port that is new in both kernel and glibc 
like this.

> +#if !defined __NR_pread && defined __NR_pread64
> +# define __NR_pread __NR_pread64
> +#endif
> +
> +#if !defined __NR_pwrite && defined __NR_pwrite64
> +# define __NR_pwrite __NR_pwrite64
> +#endif

Likewise, such conditionals should not be needed.  With 3.7 as minimum 
kernel version we should know what syscalls the kernel defines in its 
headers.

-- 
Joseph S. Myers
joseph@codesourcery.com