From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11287 invoked by alias); 3 Oct 2012 22:11:58 -0000 Received: (qmail 11262 invoked by uid 22791); 3 Oct 2012 22:11:55 -0000 X-SWARE-Spam-Status: No, hits=-4.8 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 03 Oct 2012 22:11:48 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1TJXAR-0005ZR-Mt from joseph_myers@mentor.com ; Wed, 03 Oct 2012 15:11:47 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Wed, 3 Oct 2012 15:11:47 -0700 Received: from digraph.polyomino.org.uk (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.1.289.1; Wed, 3 Oct 2012 23:11:44 +0100 Received: from jsm28 (helo=localhost) by digraph.polyomino.org.uk with local-esmtp (Exim 4.76) (envelope-from ) id 1TJXAN-0000Zb-SB; Wed, 03 Oct 2012 22:11:43 +0000 Date: Wed, 03 Oct 2012 22:11:00 -0000 From: "Joseph S. Myers" To: Marcus Shawcroft CC: Subject: Re: [PATCH 1/2] AArch64 glibc port In-Reply-To: Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Mailing-List: contact libc-ports-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: libc-ports-owner@sourceware.org X-SW-Source: 2012-10/txt/msg00011.txt.bz2 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 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 * 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 * 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