Re: [PATCH 1a/9] Tilera (and Linux asm-generic) support for glibc

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

On Fri, 4 Nov 2011, Chris Metcalf wrote:

> diff --git a/sysdeps/tile/Versions b/sysdeps/tile/Versions
> new file mode 100644
> index 0000000..b275d7f
> --- /dev/null
> +++ b/sysdeps/tile/Versions
> @@ -0,0 +1,6 @@
> +libc {
> +  GLIBC_2.12 {
> +    # name requested by gcc community.
> +    __mcount;
> +  }

GLIBC_2.12 does not make sense for a new symbol being introduced in 2.15; 
GLIBC_2.15 would be appropriate.

How important is binary compatibility with existing binaries using shared 
libraries?  Because a new port should generally have a shlib-versions file 
with a DEFAULT line setting the minimum symbol version - thus the natural 
thing to do would be to set that to GLIBC_2.15 and exclude all 
compatibility for earlier versions than that.  But I don't see such a file 
here at all (even for an earlier version from which the port was started), 
although I doubt that there really ever existed glibc 2.0 (for example) 
for these targets.

> +#define atomic_full_barrier() __insn_mf ()

It seems unfortunate for a new port to define atomics in terms of 
target-specific intrinsics - or, worse, asms.  I'm pretty sure the new 
__atomic_* built-in functions in the cxx-mem-model code now being merged 
for GCC 4.7 can represent all the variants used in glibc.  So implement 
the new patterns for your GCC ports which should also be merged for 4.7 
and you could have the glibc ports just use essentially generic 
definitions in terms of __atomic_*.

> diff --git a/sysdeps/tile/bits/byteswap.h b/sysdeps/tile/bits/byteswap.h

There's nothing *wrong* with this file; it just seems:

* probably unnecessary (current GCC can detect bswap patterns that should 
include those used by the generic bits/byteswap.h);

* generic (it would be better to make libc's bits/byteswap.h use the 
built-ins if __GNUC_PREREQ (4, 3).

> +/* Test for negative number.  Used in the signbit() macro.  */
> +__MATH_INLINE int
> +__NTH (__signbitf (float __x))
> +{
> +  __extension__ union { float __f; int __i; } __u = { __f: __x };
> +  return __u.__i < 0;
> +}
> +__MATH_INLINE int
> +__NTH (__signbit (double __x))
> +{
> +  __extension__ union { double __d; long long __i; } __u = { __d: __x };
> +  return __u.__i < 0;
> +}

Using __builtin_signbit / __builtin_signbitf would seem better than code 
using unions (though it ought to expand the same).  (If GCC PR 36757 were 
fixed then changing the definition of the signbit macro just to call 
__builtin_signbit for newer GCC would be the right approach, but that's a 
separate matter.)

> +#ifndef __tilegx__
> +  /* Updating a PLT entry atomically on TILEPro is very tricky, since
> +     bundles are 8 bytes, we can only atomically write four bytes at a
> +     time, and jump instructions span both words of the bundle.
> +
> +     We carefully update one word and then the other in an order
> +     such that the intermediate values seen by any reader are
> +     all valid code sequences. It is fortuitous that such a sequence
> +     exists.

What, your ABI is using writable-and-executable PLTs?  That's a rather 
unfortunate choice for a new ABI (and I didn't see a patch to 
check-textrel.c in the libc changes to allow it, not that I'm sure such a 
change would actually be accepted for a new architecture); it's generally 
considered a bad idea to have any segments that are both writable and 
executable.

> diff --git a/sysdeps/tile/preconfigure b/sysdeps/tile/preconfigure
> new file mode 100644
> index 0000000..61e1f96
> --- /dev/null
> +++ b/sysdeps/tile/preconfigure
> @@ -0,0 +1,14 @@
> +# This is a -*- sh -*-
> +case "$machine" in
> +    tilepro)
> +	base_machine=tile machine=tile/tilepro ;;
> +    tilegx)
> +	base_machine=tile machine=tile/tilegx/tilegx64 ;;
> +    tilegx32)
> +	base_machine=tile machine=tile/tilegx/tilegx32 ;;

I don't see any sign of distinct tilegx/tilegx32 target triplets in the 
GCC port (as in, I don't see any sign of code to change the default based 
on the target triplet).  config.guess appears to go purely on what uname 
reports.  It's not wrong to condition on the target triplet here, but it 
might be friendlier to users to check how the compiler ($CC $CFLAGS 
$CPPFLAGS) is actually configured.

> +/* Since C identifiers are not normally prefixed with an underscore
> +   on this system, the asm identifier `syscall_error' intrudes on the
> +   C name space.  Make sure we use an innocuous name.  */
> +#define	syscall_error	__syscall_error
> +#define mcount		_mcount

__mcount?

-- 
Joseph S. Myers
joseph@codesourcery.com