From: "Joseph S. Myers" <joseph@codesourcery.com>
To: David Holsgrove <david.holsgrove@xilinx.com>
Cc: <libc-ports@sourceware.org>, <john.williams@xilinx.com>,
<edgar.iglesias@xilinx.com>, <vinod.kathail@xilinx.com>,
<vidhumouli.hunsigida@xilinx.com>, <nagaraju.mekala@xilinx.com>,
<tom.shui@xilinx.com>
Subject: Re: [PATCH v2 2/2] Add MicroBlaze Port
Date: Tue, 02 Apr 2013 23:49:00 -0000 [thread overview]
Message-ID: <Pine.LNX.4.64.1304022325380.24069@digraph.polyomino.org.uk> (raw)
In-Reply-To: <e57570bc-a47e-4918-8dc7-a12548f571f2@CO1EHSMHS028.ehs.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
next prev parent reply other threads:[~2013-04-02 23:49 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1364652885-28535-1-git-send-email-david.holsgrove@xilinx.com>
2013-03-30 14:30 ` [PATCH v2 1/2] Add MicroBlaze support to elf/elf.h David Holsgrove
2013-04-08 22:15 ` Roland McGrath
2013-04-09 7:41 ` David Holsgrove
2013-04-11 20:25 ` Roland McGrath
2013-04-12 0:33 ` David Holsgrove
2013-03-30 14:31 ` [PATCH v2 2/2] Add MicroBlaze Port David Holsgrove
2013-04-02 23:49 ` Joseph S. Myers [this message]
2013-04-09 8:20 ` David Holsgrove
2013-04-09 8:32 ` Andreas Schwab
2013-04-11 7:58 ` David Holsgrove
2013-04-16 0:51 ` David Holsgrove
2013-04-17 0:18 ` David Holsgrove
2013-04-17 7:47 ` Andreas Schwab
2013-04-18 7:23 ` David Holsgrove
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.64.1304022325380.24069@digraph.polyomino.org.uk \
--to=joseph@codesourcery.com \
--cc=david.holsgrove@xilinx.com \
--cc=edgar.iglesias@xilinx.com \
--cc=john.williams@xilinx.com \
--cc=libc-ports@sourceware.org \
--cc=nagaraju.mekala@xilinx.com \
--cc=tom.shui@xilinx.com \
--cc=vidhumouli.hunsigida@xilinx.com \
--cc=vinod.kathail@xilinx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).