This is the mail archive of the
libc-ports@sources.redhat.com
mailing list for the libc-ports project.
Re: [PATCH 1/2] AArch64 glibc port
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Marcus Shawcroft <marcus dot shawcroft at linaro dot org>
- Cc: <libc-ports at sourceware dot org>
- Date: Wed, 3 Oct 2012 22:11:43 +0000
- Subject: Re: [PATCH 1/2] AArch64 glibc port
- References: <CABXK9nfac4O=A0SMe_3FvTnqjpTDF0bDkd6inTBeSbK6DUtWOw@mail.gmail.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