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

[Chris Metcalf <cmetcalf at tilera dot com>]

On 11/4/2011 5:45 PM, Joseph S. Myers wrote:
> 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.

In the version we're releasing to customers (our upcoming 4.0 release) we
are based on RHEL 6 with their glibc 2.12, so we'll need to keep the symbol
at 2.12 for that.  Does it therefore make sense for us to use the same
value in the sources we're returning to the community?  We can certainly do
it either way.

> 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.

I think we would like to be binary compatible with our 4.0 customer
release.  I hadn't noticed shlib-versions before!  In any case we seem to
match all the defaults:

.*-.*-linux.*           libm=6
.*-.*-linux.*           libc=6
.*-.*-.*                ld=ld.so.1
.*-.*-.*                libdl=2
etc.

But it sounds like you're saying we should have a
ports/sysdeps/tile/shlib-versions with just the single "DEFAULT" line
specifying GLIBC_2.15 (or GLIBC_2.12 depending on what makes sense).  Is
that right?

>> +#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.

We could certainly use __sync_synchronize() here.  The existing __sync_xxx
support, of course, is in some places heavier-weight than we would want in
terms of the __sync_xxx requirements for barriers.

> 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_*.

I'll talk to our compiler folks about this, and certainly if our tile gcc
community return includes that (or could easily be made to include it), it
might make sense to make use of it in our tile/bits/atomic.h.

However, I know that the compiler guys are also on a limited time budget in
terms of community return effort vs customer support issues, and I would
hate to block both the gcc and glibc return on that issue.   And I worry a
little that our customers using our gcc 4.4-based systems might want to
build a newer glibc from the community version, but not want to have to
install a new gcc as well.  So perhaps we could use __sync builtins for now
where it makes sense, and leave the rest of the atomic support the way it is? 

For the longer-term, it does seem like modifying the core "bits/atomic.h"
file to test for gcc version and use the cxx-mem-model stuff might make
sense, but I suspect that has the makings of a long discussion on the glibc
mailing lists, so I'd rather keep it as a separate follow-on item if that
seems to make sense.  I'm also not sure exactly what the glibc build model
would be to use target-specific versions for older gcc's, and the generic
version for newer gcc's; perhaps it's just an "#if __GNU_PREREQ /
#include_next" in the target-specific files?

>> 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).

Apparently our gcc port does not recognize these patterns (I just checked);
I've filed a bug internally.  So I can certainly submit a change to the
core file instead.  As you can imagine, my instincts tended more towards
avoiding changes to the core code when possible, but you're certainly right
that this would be a cleaner change overall.  And since our shipping gcc
4.4 supports it, it's certainly no problem for any customer trying to build
the community glibc.

>> +/* 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;
>> +}
>>
> Using __builtin_signbit / __builtin_signbitf would seem better than code 
> using unions (though it ought to expand the same).

I wasn't aware of these; they must have been introduced after gcc 4.4. 
This feels like the answer should be the same as the __atomic_* stuff,
perhaps, i.e. maybe try to change all the platforms at once to do an
#include_next based on __GNUC_PREREQ?

>> +#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.
>>
> 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.

Yeah, my mistake.  I realized after I submitted the patch that that stuff
was still there.  If you look, you'll see it's all the "old_xxx" functions;
we eliminated our dependence on RWX pages between the 3.0 and 4.0 releases
of our development environment.  I'll remove the "old" functions in the
next patch set.

>> 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.

Interesting thought!  I assume I would do something like the following?

tilegx)
  base_machine=tile
  case $($CC $CFLAGS $CPPFLAGS -dM -E -xc /dev/null | grep SIZEOF_POINTER) in
  *8) machine=tile/tilegx/tilegx64 ;;
  *4) machine=tile/tilegx/tilegx32 ;;
  esac ;;


>> +/* 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?

Oops, good catch.

Thanks for the feedback.  I'll work my way through your other comments this
weekend as time allows, or on Monday, and if I haven't heard any other
feedback I'll put out another patch set mid-week or so.  I really
appreciate the time you're taking for this!
-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com