Re: [PATCH] MicroBlaze Port

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

First some general comments about things missing from the port:

* You don't have a shlib-versions file.  A new port should generally set 
the minimum symbol version to that of the next glibc release, so either 
GLIBC_2.17 or GLIBC_2.18 depending on when this port gets in, which is 
done with a DEFAULT line in a shlib-versions file.

* You don't have a kernel-features.h file.  There are various features 
that the generic kernel-features.h file only enables for particular 
architectures, not unconditionally for all kernels with a certain version 
or later.  As for other architectures, you need to go through all those 
macros and define them in the MicroBlaze kernel-features.h for the 
relevant kernel.org kernel versions in which the feature was added 
(unconditionally, if the feature was present for MicroBlaze in kernel 
2.6.30 when MicroBlaze support was added).

* You don't have a libm-test-ulps file, and should add one.  A review of 
test results should have shown that such a file was missing.

* You don't have a sotruss-lib.c file, and should add one.  A review of 
compiler warnings should have shown that such a file was missing.

* You don't have a tst-audit.h file, and should add one.

Now more specific comments on the changes:

> +++ b/ports/sysdeps/microblaze/Makefile
> @@ -0,0 +1,31 @@
> +pic-ccflag = -fPIC

This appears to be the default.

> +# Make sure setjmp.c is compiled with a frame pointer
> +CFLAGS-setjmp.c := -fno-omit-frame-pointer

Comments (which should end with ".") should not just repeat the plain 
meaning of the source code.  Instead, explain *why* a frame pointer is 
needed (on this architecture).

> +$(objpfx)libm.so: $(elfobjdir)/ld.so
> +$(objpfx)libcrypt.so: $(elfobjdir)/ld.so
> +$(objpfx)libresolv.so: $(elfobjdir)/ld.so
> +$(objpfx)libnss_dns.so: $(elfobjdir)/ld.so
> +$(objpfx)libnss_files.so: $(elfobjdir)/ld.so
> +$(objpfx)libnss_db.so: $(elfobjdir)/ld.so
> +$(objpfx)libnss_nis.so: $(elfobjdir)/ld.so
> +$(objpfx)libnss_nisplus.so: $(elfobjdir)/ld.so
> +$(objpfx)libnss_hesiod.so: $(elfobjdir)/ld.so
> +$(objpfx)libnss_compat.so: $(elfobjdir)/ld.so
> +$(objpfx)libanl.so: $(elfobjdir)/ld.so
> +$(objpfx)libnsl.so: $(elfobjdir)/ld.so
> +$(objpfx)libcidn.so: $(elfobjdir)/ld.so
> +$(objpfx)libutil.so: $(elfobjdir)/ld.so

I don't think it's a good idea to put this sort of thing in sysdeps files; 
certainly not when you need to name every library, probably including new 
ones as and when added to glibc.

Instead, note that on x86_64, for example, the installed libc.so linker 
script contains:

GROUP ( /lib64/libc.so.6 /usr/lib64/libc_nonshared.a  AS_NEEDED ( /lib64/ld-linux-x86-64.so.2 ) )

I think we should make all the miscellaneous shared libraries and .so 
files be linked more like normal libraries linked with a shared glibc 
after installation.  That is, --start-group libc.so libc_nonshared.a 
--as-needed ld.so --no-as-needed --end-group.  And with that change, 
remove all the miscellaneous rules in various makefiles (sysdeps and 
otherwise) to link this or that library with ld.so or libc_nonshared.a.

That sort of architecture-independent change does have risks that would 
put it off past 2.17 now we're in the freeze, but it seems the right way 
to do this rather than duplicating a long list of libraries in sysdeps 
files.

> diff --git a/ports/sysdeps/microblaze/Versions b/ports/sysdeps/microblaze/Versions
> new file mode 100644
> index 0000000..2b020f8
> --- /dev/null
> +++ b/ports/sysdeps/microblaze/Versions
> @@ -0,0 +1,6 @@
> +libc {
> +  GLIBC_2.0 {
> +    # Functions from libgcc.
> +    __divdi3; __moddi3; __udivdi3; __umoddi3;
> +  }
> +}

Will not be needed when you use GLIBC_2.18 in shlib-versions; functions 
from libgcc should only be exported like this for backwards binary 
compatibility in very old ports that have been around since 2.0.

> +++ b/ports/sysdeps/microblaze/asm-syntax.h

> +#ifdef HAVE_ELF

HAVE_ELF is obsolete.  glibc only supports ELF; remove all conditionals on 
it.

> +/* For ELF we need the `.type' directive to make shared libs work right.  */
> +#define PROLOG(name) .type name,@function
> +#define EPILOG(name) .size name,.-name

Nothing uses these macros.  Remove them.

> +/* For ELF we need to prefix register names and local labels.  */
> +#ifdef __STDC__

__STDC__ conditionals are obsolete, remove them.  Remove the conditioned 
macros as well unless there is a real purpose to them once they only have 
a single definition.
> +++ b/ports/sysdeps/microblaze/backtrace.c

Any new file of more than ten lines needs a copyright and license notice.  
This file needs reformatting in accordance with the GNU Coding Standards.

> +++ b/ports/sysdeps/microblaze/backtrace_linux.c

Likewise.

> +++ b/ports/sysdeps/microblaze/bits/atomic.h

> +/*
> + * Microblaze does not have byte and halfword forms of load and reserve and
> + * store conditional. So for microblaze we stub out the 8- and 16-bit forms.

GNU Coding Standards formatting (no "*" on successive comment lines, two 
spaces after "." in comments).

> +                "   addc    r0, r0, r0;"    /* clean carry bit*/                              \

Full sentence comments ending with ".  ".

> +                    : "=&r" (__tmp),        /* %0 */                                          \

If you use symbolic names for your operands, as supported by all GCC 
versions that can be used to build glibc, you don't need these comments 
explaining the numbers.

> +#define __arch_atomic_exchange_32_acq(mem, value)                                             \

Same comments as above apply here.

> +#define __arch_atomic_exchange_and_add_32(mem, value)                                         \

Likewise.  Please review the whole patch for the above issues; I won't 
mention them where they appear subsequently.

> +/* Define bits representing the exception.  We use the bit positions
> +   of the appropriate bits in the FPU control word.  */
> +enum
> +  {
> +    FE_INEXACT = 0x04,
> +#define FE_INEXACT	FE_INEXACT
> +    FE_UNDERFLOW = 0x08,
> +#define FE_UNDERFLOW	FE_UNDERFLOW
> +    FE_OVERFLOW = 0x10,
> +#define FE_OVERFLOW	FE_OVERFLOW
> +    FE_DIVBYZERO = 0x20,
> +#define FE_DIVBYZERO	FE_DIVBYZERO
> +    FE_INVALID = 0x40,
> +#define FE_INVALID	FE_INVALID
> +  };

This is not the style now used to define constants in enums, where the C 
standard requires them to be usable in #if; please update this code.

> +enum
> +  {
> +    FE_TONEAREST = 0x0,
> +#define FE_TONEAREST	FE_TONEAREST
> +    FE_TOWARDZERO = 0x1,
> +#define FE_TOWARDZERO	FE_TOWARDZERO
> +    FE_UPWARD = 0x2,
> +#define FE_UPWARD	FE_UPWARD
> +    FE_DOWNWARD = 0x3
> +#define FE_DOWNWARD	FE_DOWNWARD

Likewise.  The comment about MIPS also seems suspicious.  And, since you 
define exceptions and rounding modes, you need implementations of the fe* 
fenv.h functions - you don't have them.

> diff --git a/ports/sysdeps/microblaze/bits/wordsize.h b/ports/sysdeps/microblaze/bits/wordsize.h
> new file mode 100644
> index 0000000..23d379a
> --- /dev/null
> +++ b/ports/sysdeps/microblaze/bits/wordsize.h
> @@ -0,0 +1,3 @@
> +/* Determine the wordsize from the preprocessor defines.  */
> +
> +# define __WORDSIZE	32

You don't need this file for an architecture with only one wordsize; 
delete it, and sysdeps/wordsize-32/bits/wordsize.h will be used.

> +++ b/ports/sysdeps/microblaze/bsd-_setjmp.S
> @@ -0,0 +1,22 @@
> +/* BSD `_setjmp' entry point to `sigsetjmp (..., 0)'.
> +   Copyright (C) 1997, 1998, 2002, 2008 Free Software Foundation, Inc.

Use <year>-2012 on all new files.  Not mentioned again where the issue 
appears for other files, please check and fix the whole patch for it.

> +++ b/ports/sysdeps/microblaze/configure.in
> @@ -0,0 +1,32 @@
> +GLIBC_PROVIDES dnl See aclocal.m4 in the top level source directory.
> +# Local configure fragment for sysdeps/microblaze/elf.
> +
> +if test "$usetls" != no; then
> +# Check for support of thread-local storage handling in assembler and
> +# linker.

TLS configure checks are no longer used and have been removed; remove them 
from this file.

> +/* Return nonzero iff ELF header is compatible with the running host.  */
> +static inline int
> +elf_machine_matches_host (const Elf32_Ehdr *ehdr)
> +{
> +  return (ehdr->e_machine == EM_MICROBLAZE ||
> +		ehdr->e_machine == EM_NEW_MICROBLAZE ) ;

Given the use of GLIBC_2.18 versions, compatibility with any legacy 
binaries shouldn't arise so you should only need to check for the 
officially allocated value.

> +#if (!defined RTLD_BOOTSTRAP || USE___THREAD)

USE___THREAD is obsolete and has been removed.

> +++ b/ports/sysdeps/microblaze/nptl/pthread_spin_lock.c

Is there an advantage to this file over defining 
SPIN_LOCK_READS_BETWEEN_CMPXCHG to some suitable value and then including 
<sysdeps/../nptl/pthread_spin_lock.c> as various other ports do?  If so, 
please include comments explaining it.

> +++ b/ports/sysdeps/microblaze/nptl/pthread_spin_trylock.c

Again, why not the generic version?

> +# ifdef HAVE_ELF

Obsolete conditionals.

> +#  define ASM_TYPE_DIRECTIVE(name,typearg) .type name,typearg

ASM_TYPE_DIRECTIVE has been removed.

> +# ifdef	NO_UNDERSCORES

Obsolete conditional.

> diff --git a/ports/sysdeps/unix/sysv/linux/microblaze/Makefile b/ports/sysdeps/unix/sysv/linux/microblaze/Makefile
> new file mode 100644
> index 0000000..3cf2b6a
> --- /dev/null
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/Makefile
> @@ -0,0 +1,12 @@
> +ifeq ($(subdir),misc)
> +sysdep_routines += mremap
> +endif

You don't have a mremap.S, so this makes no sense.

> +ifeq ($(subdir),elf)
> +sysdep-others += lddlibc4
> +install-bin += lddlibc4
> +endif

This also makes no sense - MicroBlaze certainly never had a.out support in 
kernel.org Linux.

> diff --git a/ports/sysdeps/unix/sysv/linux/microblaze/Versions b/ports/sysdeps/unix/sysv/linux/microblaze/Versions
> new file mode 100644
> index 0000000..cf363fa
> --- /dev/null
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/Versions

Given GLIBC_2.18 versions, you should only need an entry for fallocate64, 
nothing else.

> diff --git a/ports/sysdeps/unix/sysv/linux/microblaze/alphasort64.c b/ports/sysdeps/unix/sysv/linux/microblaze/alphasort64.c
> new file mode 100644
> index 0000000..0b5ae47
> --- /dev/null
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/alphasort64.c
> @@ -0,0 +1 @@
> +#include <sysdeps/unix/sysv/linux/i386/alphasort64.c>

Lots of files like this should not be needed in new ports, only very old 
ones.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/bits/fcntl.h

> +# define O_DIRECTORY    0200000 /* Must be a directory.  */
> +# define O_NOFOLLOW     0400000 /* Do not follow links.  */
> +# define O_DIRECT        040000 /* Direct disk access.  */
> +#ifdef __USE_LARGEFILE64
> +# define O_LARGEFILE    0100000
> +#endif

That's not how bits/fcntl.h files using fcntl-linux.h should work.  
Instead, define __O_*, unconditionally; fcntl-linux.h then defines the 
public O_* names.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/bits/msq.h

How does this differ from the generic sysdeps/unix/sysv/linux/bits/msq.h - 
why is it needed?

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/bits/sem.h

Likewise.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/bits/shm.h

Likewise, other than that you're missing SHM_EXEC and SHM_NORESERVE and 
shouldn't be?

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/configure.in
> @@ -0,0 +1,5 @@
> +GLIBC_PROVIDES dnl See aclocal.m4 in the top level source directory.
> +# Local configure fragment for sysdeps/unix/sysv/linux/microblaze.
> +
> +libc_cv_gcc_unwind_find_fde=yes

Inappropriate in a new port.

> +arch_minimum_kernel=2.0.10

2.6.16 is the global minimum now, you should use 2.6.30 or newer since 
MicroBlaze kernel support first appeared in 2.6.30.

> diff --git a/ports/sysdeps/unix/sysv/linux/microblaze/dl-librecon.h b/ports/sysdeps/unix/sysv/linux/microblaze/dl-librecon.h
> new file mode 100644
> index 0000000..dbb4e75
> --- /dev/null
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/dl-librecon.h
> @@ -0,0 +1 @@
> +#include <sysdeps/unix/sysv/linux/i386/dl-librecon.h>

Not relevant.  This is about libc5 and a.out.  libc5 didn't support 
MicroBlaze, only i386 and m68k.

> diff --git a/ports/sysdeps/unix/sysv/linux/microblaze/fchown.c b/ports/sysdeps/unix/sysv/linux/microblaze/fchown.c
> new file mode 100644
> index 0000000..3a69ecc
> --- /dev/null
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/fchown.c
> @@ -0,0 +1 @@
> +#include <sysdeps/unix/sysv/linux/i386/fchown.c>

Where the i386 file you're including says "Consider moving to 
syscalls.list.", please create a syscalls.list entry for MicroBlaze 
instead of using the i386 file, unless there is a reason that won't work 
in which case you should change the comment in the i386 file to explain 
why syscalls.list can't be used.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/fchownat.c
> @@ -0,0 +1 @@
> +#include <sysdeps/unix/sysv/linux/i386/fchownat.c>

Inappropriate, since 2.6.30 had the fchownat syscall for MicroBlaze and 
all the special backwards compatibility should be irrelevant.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/getdents64.c
> @@ -0,0 +1 @@
> +#include <sysdeps/unix/sysv/linux/i386/getdents64.c>

Appears only to be relevant with old symbol versions.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/getegid.c
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/geteuid.c
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/getgid.c

See comments about using syscalls.list.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/getgroups.c
> @@ -0,0 +1,2 @@
> +/* We also have to rewrite the kernel gid_t to the user land type.  */

That doesn't make sense to me.  That's not what the i386 file does.  And 
see the comment about using syscalls.list.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/getresgid.c
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/getresuid.c

syscalls.list.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/getrlimit64.c

Irrelevant without old symbol versions.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/getuid.c

syscalls.list.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/ldconfig.h

Inappropriate given the lack of libc5.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/msgctl.c

Most of this seems irrelevant compatibility code.

> +__poll GLIBC_2.1 w ? D .text 00000000

Although of course versions will change to GLIBC_2.18, these entries also 
indicate something wrong with the ABI check parsing for MicroBlaze, which 
needs to be fixed.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.h

Define lll_futex_timed_wait_bitset.

> +/* The kernel notifies a process which uses CLONE_CLEARTID via futex

You're missing the typo fix from commit 
adcdc775e11f6fc788448b9e2b0b4ef08579e463.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/oldgetrlimit64.c

Should not be relevant.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/readdir64.c
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/readdir64_r.c
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/scandir64.c
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/semctl.c

More files I think can only be relevant to very old ports or that are 
mainly compatibility code.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/setfsgid.c
> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/setfsuid.c

syscalls.list.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/setgroups.c
> @@ -0,0 +1,2 @@
> +/* We also have to rewrite the kernel gid_t to the user land type.  */

Comment seems inappropriate.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/setrlimit.c

syscalls.list.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/shmctl.c

Mainly compatibility code.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/sys/user.h

> +  struct user_fpregs_struct	i387;

Really?  How can this make sense on a non-x86 architecture?

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/syscalls.list

> +oldgetrlimit	EXTRA	getrlimit	i:ip	__old_getrlimit	getrlimit@GLIBC_2.0
> +oldsetrlimit	EXTRA	setrlimit	i:ip	__old_setrlimit	setrlimit@GLIBC_2.0

These bits should no longer be relevant.

> +#ifdef __STDC__

Obsolete conditional.

> +++ b/ports/sysdeps/unix/sysv/linux/microblaze/versionsort64.c

Should not be relevant.

-- 
Joseph S. Myers
joseph@codesourcery.com