Re: [PATCH v2 2/2] Add MicroBlaze Port

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

This patch is a lot closer to being ready for inclusion than the previous 
one, but some further changes are still needed, including some I mentioned 
in my previous review 
<http://sourceware.org/ml/libc-ports/2012-11/msg00139.html> which you 
might wish to check again (and respond individually to any points where 
you think the change I indicate is wrong).

(The .abilist files and libm-test-ulps will of course need routine updates 
in the next version to account for recent libc changes.)

On Sun, 31 Mar 2013, David Holsgrove wrote:

> +	* ChangeLog.microblaze: New file

The ChangeLog itself doesn't get mentioned in the ChangeLog entries.

> +	* sysdeps/microblaze/Implies: New file

"." at end of each ChangeLog entry.

> +$(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

As noted, work out with libc-alpha how not to need this in the port.

> diff --git a/ports/sysdeps/microblaze/backtrace.c b/ports/sysdeps/microblaze/backtrace.c

My previous comment about needing reformatting of this file and 
backtrace_linux.c in accordance with the GNU Coding Standards still 
applies.

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

Previously made point about two spaces after "." in comments still 
applies.

> +/* MicroBlaze supports only round-to-nearest.  The software
> +   floating-point support also acts this way.  */
> +enum
> +  {
> +    __FE_UNDEFINED = 0,
> +
> +    FE_TONEAREST =
> +#define FE_TONEAREST	0x1
> +      FE_TONEAREST,
> +  };
> +
> +/* Define bits representing the exception.  We use the bit positions
> +   of the appropriate bits in the FPU control word.  */
> +
> +#ifndef FE_INVALID
> +# define FE_INVALID   __FE_UNDEFINED
> +#endif

There's something confused about the types here.  You've defined 
__FE_UNDEFINED as an invalid *rounding mode*, so it's wrong to be using it 
to define *exceptions*.

If MicroBlaze does not support an exception, the header must not define 
the corresponding macro at all.  If it doesn't support any exceptions, 
define FE_ALL_EXCEPT to 0 (not __FE_UNDEFINED) while not defining 
individual exception macros.  If this causes issues with code expecting 
some such macros to be defined, follow what Tile's math_private.h does to 
avoid such issues.

> diff --git a/ports/sysdeps/microblaze/configure.in b/ports/sysdeps/microblaze/configure.in
> new file mode 100644
> index 0000000..bb14721
> --- /dev/null
> +++ b/ports/sysdeps/microblaze/configure.in
> @@ -0,0 +1,8 @@
> +GLIBC_PROVIDES dnl See aclocal.m4 in the top level source directory.
> +# Local configure fragment for sysdeps/microblaze/elf.
> +
> +dnl It is always possible to access static and hidden symbols in an
> +dnl position independent way.
> +dnl NOTE: This feature was added by the GCC TLS patches.  We should test for
> +dnl it.  Until we do, don't define it.
> +#AC_DEFINE(PI_STATIC_AND_HIDDEN)

Sounds like a cargo-culted comment to me.  You're assuming TLS support in 
GCC.  So define or not this macro in whatever way is correct for the 
minimum required GCC version.  A commented out definition like this might 
make sense if there is a feature yet to be added to GCC but intended to be 
added, but not normally.

> diff --git a/ports/sysdeps/microblaze/crti.S b/ports/sysdeps/microblaze/crti.S
> new file mode 100644
> index 0000000..bc3ac1a
> --- /dev/null
> +++ b/ports/sysdeps/microblaze/crti.S
> @@ -0,0 +1,72 @@
> +/* Special .init and .fini section support for MicroBlaze.
> +   Copyright (C) 1995-2013 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.

Plus the exception wording used on other versions of crti.S (and, 
generally, files that get statically linked into user programs even when 
linking dynamically with most of glibc).

> diff --git a/ports/sysdeps/microblaze/crtn.S b/ports/sysdeps/microblaze/crtn.S

Likewise.

> +    PUT_REL_64(reloc_addr, map->l_addr + reloc->r_addend);

Generally, for calls to functions and function-like macros, there should 
be a space before the open parenthesis.  This is just one example where 
it's missing; please review the whole patch for it.

> diff --git a/ports/sysdeps/microblaze/start.S b/ports/sysdeps/microblaze/start.S

License exception notice.

> +#define MICROBLAZE_HIDDEN_DEF_REAL(x) \
> +hidden_def(x)
> +
> +#define MICROBLAZE_HIDDEN_DEF(x) MICROBLAZE_HIDDEN_DEF_REAL(C_SYMBOL_NAME(x))

Why do you need these?  What's wrong with just using hidden_def?

> diff --git a/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.c b/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.c

What are the differences from the generic 
nptl/sysdeps/unix/sysv/linux/lowlevellock.c, and have you made sure there 
is a genuine MicroBlaze-specific purpose to those differences?  If there 
is, include comments by each difference explaining its rationale.  We'd 
like to merge the different lowlevellock.c versions as far as possible....

> +#ifdef PIC
> +#define SYSCALL_ERROR_LABEL 0f
> +#else
> +#define SYSCALL_ERROR_LABEL __syscall_error
> +#endif

Space after "#" inside #if to indicate the level of #if nesting, so 
"# define" here.  Check for other instances in the patch as well.

-- 
Joseph S. Myers
joseph@codesourcery.com