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

[David Holsgrove <david dot holsgrove at xilinx dot com>]

> -----Original Message-----
> From: Joseph Myers [mailto:joseph@codesourcery.com]
> Sent: Wednesday, 3 April 2013 9:50 am
> To: David Holsgrove
> Cc: libc-ports@sourceware.org; John Williams; Edgar Iglesias; Vinod Kathail;
> Vidhumouli Hunsigida; Nagaraju Mekala; Tom Shui
> Subject: Re: [PATCH v2 2/2] Add MicroBlaze Port
> 
> 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).
> 

Hi Joseph, and thanks again for the review.
Please see my replies inline below, and I've attached an updated patch.

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

Fixed.

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

Discussion ongoing, if possible I would like to separate this architecturally
independent issue from the initial port submission, and come back to address this
when we have a solution for all ports.

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

backtrace / backtrace_linux.c reformatted.

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

I've grep'ed / sed'ed my way to updating all relevant comments in the patch.
Some exceptions are where we're commenting about the end of an #ifdef with just
the definition as the comment, but this seems in line with other ports practice.

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

Updated fenv.h to drop defining the FE_* macros - following Tile's math_private.h
allowed me to resolve the build errors I originally had with math code expecting
these macros to be defined.

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

Removed ports/sysdeps/microblaze/configure{,.in} - they were carry over from
initial copy of existing port.

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

Updated licenses to include exception wording for crti.S and others in line with
existing ports usage.

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

Corrected spacing between parentheses in the use of function like macros but I've
matched the usage in existing ports where relevant to assist in future comparisons.

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

Removed unnecessary MICROBLAZE_HIDDEN_DEF redefinition of 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....
> 

Reverted to generic lowlevellock.c - differences between previous MicroBlaze
version and generic due to divergence since original MicroBlaze port was
created, and are not relevant today.

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

Inserted spaces where appropriate in #ifdefs to highlight nesting.

The instances where we had been pointing to an i386 file which comments
"Consider moving to syscalls.list" have been added to the syscalls.list or fall on
the generic version.

With these changes, I believe I've addressed all comments from your first review.
Please let me know if you have any other recommendations, and thanks again for
the help and reviews thus far.

thanks again,
David

> --
> Joseph S. Myers
> joseph@codesourcery.com

Attachment: 0002-MicroBlaze-Port.patch
Description: 0002-MicroBlaze-Port.patch