Re: [PATCH 1/2] AArch64 glibc port

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

On Fri, 2 Nov 2012, Marcus Shawcroft wrote:

> This revised patch addresses the comments raised by Carlos, Andreas
> and Joseph on the original aarch64 port submission.

This is a lot closer to being ready, but there are still some issues to 
address:

Did you check the set of exported symbols against other 64-bit platforms 
as I suggested?  I note, for example, that you're missing an 
__sqrtl_finite symbol in libm.

> * Copyright years have been merged inline with the documentation
> Carlos highlighted.

With that done, "Free Software Foundation, Inc." can now always go on the 
same line as the "Copyright (C) <years>"; it was only long lists of years 
that required having it sometimes on a separate line.

> * libm-test-ulps regenerated, bringing in clog definitions and setting
> sqrt ulps to 0.

You still have sqrt ulps listed.  That still suggests something is wrong 
(either not removing the old ulps properly before regenerating, or an 
actual problem with sqrtl).

> * FE_INVALID and friends defined using the same mechanism adopted in
> other ports.

Now that I've fixed bug 3439, this will need another update to follow the 
new mechanism now used for these macros.

(Depending on the order in which the patches go in, one of the other of us 
will also need to do an fclrexcpt.c update in line with my patch 
<http://sourceware.org/ml/libc-alpha/2012-11/msg00057.html>.)

> Joseph's review pointed out that the port should switch from __sync to
> __atomic primitives, work on this is in progress, but not included in
> this patch.

That really shouldn't be a hard conversion.

> * test-double.out
>   38 instances of underflow not set on fma related tests.

Since you've removed the optimized functions so they can be dealt with 
after the main port is in, you'll be using 
sysdeps/ieee754/ldbl-128/s_fma.c (which is generally unlikely to be a good 
idea, except on any systems implementing binary128 floating-point in 
hardware - most architectures using ldbl-128 in software make sure to 
override the choice of fma implementation, with the dbl-64 version if they 
don't have hardware fma).

While the optimized fma using fma instructions would no doubt fix those 
failures, they do suggest you have a bug in your sfp-machine.h - either it 
isn't setting INEXACT exceptions correctly from the addition, so 
fetestexcept fails to detect them, or it isn't handling underflow 
correctly on conversion to double, or both.  So if you investigate these 
failures before adding optimized fma, they may also solve some of your 
test-ldouble failures as well.

> * test-ldouble.out
>   27 instances of underflow set.  These all appear to be related to
> scalbn setting underflow when not expected.
> 
> * test-ildoubl.out
>   3 errors for ULPs on csqrt tests.

However, it's possible that even with a fully correct soft-fp port there 
might be failures for these tests (that is, that there are currently 
unfixed bugs relating to ldbl-128 and shown by the testsuite) - I don't 
know whether these tests have clean results for other ldbl-128 
architectures right now or not.

> +#define FE_DFL_ENV     ((fenv_t *) -1l)

Missing const - should have type "const fenv_t *".  I just added conform/ 
baselines for fenv.h which would detect such a type error.  Unfortunately 
(a) conform/ doesn't yet support cross-testing and (b) it would be 
necessary to compare results between architectures to detect any 
AArch64-specific problems it shows up, since the expected results aren't 
yet very clean.  So it may be premature to try to identify header problems 
that way.

> +#ifdef __AARCH64EB__
> +#define __BYTE_ORDER __BIG_ENDIAN
> +#else

In general we indent preprocessor directives inside conditionals, so
"# define" inside #if.

> +# define FE_NOMASK_ENV  ((__const fenv_t *) -2)

Since 2012-01-07 we no longer use __const, just plain const.

> +   __builtin_frame_address (0) returns the value of the hard frame
> +   pointer, which will point at the location of the saved PC on the
> +   stack.  Below this in memory is the remainder of the linkage info,
> +   occupying 12 bytes.  Therefore in order to address from
> +   CURRENT_STACK_FRAME using "struct layout", we need to have the macro
> +   return the hard FP minus 12.  Of course, this makes no sense
> +   without the obsolete APCS stack layout...  */

References to "obsolete APCS stack layout" make no sense in a new port.

> +#define __ASSUME_PREAD                 1
> +#define __ASSUME_PWRITE                        1

Nothing seems to test these macros, so remove them.

-- 
Joseph S. Myers
joseph@codesourcery.com