Re: [PATCH 2/4] Nios II: sysdeps/nios2 portions

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

On Fri, 28 Mar 2014, Chung-Lin Tang wrote:

>         * sysdeps/nios2/configure.ac: New file.

And presumably:

	* sysdeps/nios2/configure: New generated file.

> diff --git a/sysdeps/nios2/Makefile b/sysdeps/nios2/Makefile
> new file mode 100644
> index 0000000..34c678a
> --- /dev/null
> +++ b/sysdeps/nios2/Makefile
> @@ -0,0 +1,41 @@
> +# Copyright (C) 1993-2014 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.
> +
> +# The GNU C Library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with the GNU C Library; if not, write to the Free
> +# Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> +# 02111-1307 USA.

Please fix all files using FSF postal addresses, in any patch in this 
series, to use <http://www.gnu.org/licenses/> instead.

> +ifeq ($(subdir),debug)
> +CFLAGS-backtrace.c += -funwind-tables
> +CFLAGS-tst-backtrace2.c += -funwind-tables
> +CFLAGS-tst-backtrace3.c += -funwind-tables
> +CFLAGS-tst-backtrace4.c += -funwind-tables
> +CFLAGS-tst-backtrace5.c += -funwind-tables
> +CFLAGS-tst-backtrace6.c += -funwind-tables

Only the setting of CFLAGS-backtrace.c is needed; the others are in 
debug/Makefile.

> diff --git a/sysdeps/nios2/__longjmp.S b/sysdeps/nios2/__longjmp.S

Make sure you've checked for any need for CFI directives in this code.

> +/* Define bits representing the exception.
> +   Nios II doesn't support FPU exceptions, and these are defined just to
> +   avoid undefined symbols.  */

No, that's not correct.  Follow what Tile / MicroBlaze does, define only 
__FE_UNDEFINED / FE_TONEAREST as rounding modes, only FE_ALL_EXCEPT for 
exceptions, and use the Tile / MicroBlaze math_private.h to avoid any need 
for the other macros to be defined to build glibc.

> +#ifdef __USE_GNU
> +/* Floating-point environment where none of the exceptions are masked.  */
> +# define FE_NOMASK_ENV  ((const fenv_t *) -2)
> +#endif

Don't define this either, given you don't implement it.

> +unsigned int
> +internal_function
> +_dl_nios2_get_gp_value (struct link_map *main_map)
> +{
> +  ElfW(Dyn)* dyn = main_map->l_ld;

Space before rather than after "*" here.

> +  asm("nextpc\t%0\n\t"

asm, like a function call, should have a space before the "(", here and 
elsewhere.

> +      "1: movhi\t%1, %%hiadj(_GLOBAL_OFFSET_TABLE_ - 1b)\n\t"
> +      "addi\t%1, %1, %%lo( _GLOBAL_OFFSET_TABLE_ - 1b)\n\t"

Stray space after "(".

> +#ifndef RTLD_BOOTSTRAP_UDIV
> +static inline unsigned int __attribute__((always_inline))
> +nios2_rtld_bootstrap_udiv (unsigned int num, unsigned int den)

If we end up with an approach where an architecture defines such a macro / 
function, it should at least be conditional on compiler-defined macros 
saying whether hardware division is available - if it is available you can 
just use it, as on other architectures.  If GCC doesn't predefine such a 
macro that can be tested to indicate availability of hardware divide, add 
such a macro (if we end up going this way in glibc), __nios2_hw_divide or 
similar - likewise for hardware multiply if that's only conditionally 
available and we end up with division implemented in terms of multiply in 
glibc.

> +  if (__builtin_expect (r_type == R_NIOS2_RELATIVE, 0))
> +    *reloc_addr = map->l_addr + reloc->r_addend;
> +  else if (__builtin_expect (r_type == R_NIOS2_NONE, 0))

Please use the __glibc_likely / __glibc_unlikely macros in new code 
instead of direct use of __builtin_expect.

> +/* Default to an executable stack.  PF_X can be overridden if PT_GNU_STACK is
> + * present, but it is presumed absent.  */
> +#define DEFAULT_STACK_PERMS (PF_R|PF_W|PF_X)

That seems like a bad default for a new platform.  Since you said 
kernel/userspace interface issues can be fixed, the kernel should be 
changed to default to non-executable stack without needing PT_GNU_STACK, 
and glibc changed to match, unless there is some strong reason to do 
otherwise.

-- 
Joseph S. Myers
joseph@codesourcery.com