This is the mail archive of the
libc-ports@sources.redhat.com
mailing list for the libc-ports project.
Re: [PATCH v2 2/2] Add MicroBlaze Port
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: David Holsgrove <david dot holsgrove at xilinx dot com>
- Cc: <libc-ports at sourceware dot org>, <john dot williams at xilinx dot com>, <edgar dot iglesias at xilinx dot com>, <vinod dot kathail at xilinx dot com>, <vidhumouli dot hunsigida at xilinx dot com>, <nagaraju dot mekala at xilinx dot com>, <tom dot shui at xilinx dot com>
- Date: Tue, 2 Apr 2013 23:49:41 +0000
- Subject: Re: [PATCH v2 2/2] Add MicroBlaze Port
- References: <1364652885-28535-1-git-send-email-david dot holsgrove at xilinx dot com> <e57570bc-a47e-4918-8dc7-a12548f571f2 at CO1EHSMHS028 dot ehs dot local>
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