Re: [PATCH 2/2] MIPS16: MIPS16 support proper

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

On Wed, 23 Jan 2013, Maciej W. Rozycki wrote:

> Index: ports/sysdeps/mips/__longjmp.c
> ===================================================================
> --- ports/sysdeps/mips/__longjmp.c	2013-01-17 00:55:17.000000000 +0000
> +++ ports/sysdeps/mips/__longjmp.c	2013-01-20 19:40:57.097753716 +0000
> @@ -23,8 +23,8 @@
>    #error This file uses GNU C extensions; you must compile with GCC.
>  #endif
>  
> -void
> -__longjmp (env_arg, val_arg)
> +static void __attribute__ ((nomips16))
> +____longjmp (env_arg, val_arg)
>       __jmp_buf env_arg;
>       int val_arg;
>  {
> @@ -86,3 +86,5 @@ __longjmp (env_arg, val_arg)
>    /* Avoid `volatile function does return' warnings.  */
>    for (;;);
>  }
> +
> +strong_alias (____longjmp, __longjmp);

Why is the renaming / alias needed?

> Index: ports/sysdeps/mips/abort-instr.h
> ===================================================================
> --- ports/sysdeps/mips/abort-instr.h	2013-01-17 00:55:17.000000000 +0000
> +++ ports/sysdeps/mips/abort-instr.h	2013-01-20 19:40:57.097753716 +0000
> @@ -1,2 +1,6 @@
>  /* An instruction which should crash any program is a breakpoint.  */
> +#ifdef __mips16
> +#define ABORT_INSTRUCTION asm ("break 63")
> +#else
>  #define ABORT_INSTRUCTION asm ("break 255")
> +#endif

Indentation inside #if, "# define".

> +/* MIPS16 uses GCC __sync_* builtins to implement the required atomic
> +   operations, to abstract out the unsupported assembly instructions.
> +   ??? Maybe eventually use them for 32-bit MIPS too?  */
> +#ifdef __mips16
> +# if __GNUC_PREREQ (4, 1)

glibc requires at least 4.3 to build, so no such condition is needed in an 
internal header.

However, __sync_* are obsolete; the __atomic_* built-in functions 
(supported in 4.7 and later, well-optimized for MIPS in 4.8 and later) are 
preferred because they allow more precise specification of the barrier 
semantics in particular cases.  So if building with 4.8 or later, you 
should use the existing definitions in terms of __atomic_* that are 
already used in those circumstances for MIPS - and probably use them for 
the (4.7 and MIPS16) combination, since they won't be any worse than 
__sync_*.  Fallbacks using __sync_* should only be for the (older GCC, 
MIPS16) combination.  So the logic in the file should be:

#if (4.8 or later, or 4.7 and MIPS16)
existing implementation using __atomic_*
#elif (MIPS16)
implementation using __sync_*, for older GCC and MIPS16
#else
existing implementation using inline asm
#endif

> +#else
> +
> +/* MIPS16 version.  We currently only support O32 under MIPS16; the proper
> +   assembly preprocessor abstractions will need to be added if other ABIs
> +   are to be supported.  */
> +
> +#define RTLD_START asm (\

Indentation, "# define".  Many further cases in this patch as well, not 
individually pointed out.

>  /* Macros for accessing the hardware control word.  */
> +extern fpu_control_t __fpu_getcw (void) __THROW;
> +extern void __fpu_setcw (fpu_control_t) __THROW;
> +#ifdef __mips16
> +#define _FPU_GETCW(cw) do { (cw) = __fpu_getcw (); } while (0)
> +#define _FPU_SETCW(cw) __fpu_setcw (cw)
> +#else
>  #define _FPU_GETCW(cw) __asm__ volatile ("cfc1 %0,$31" : "=r" (cw))
>  #define _FPU_SETCW(cw) __asm__ volatile ("ctc1 %0,$31" : : "r" (cw))
> +#endif

Names __mips_fpu_getcw and __mips_fpu_setcw might reduce any risk of 
conflicts with any future architecture-independent internal function?

> +  _FPU_GETCW(cw);

> +  _FPU_SETCW(cw);

Missing space before '('.

> Index: ports/sysdeps/mips/mips32/mips16/fpu/Versions
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ ports/sysdeps/mips/mips32/mips16/fpu/Versions	2013-01-20 22:11:01.057025227 +0000
> @@ -0,0 +1,5 @@
> +libc {
> +  GLIBC_2.18 {
> +    __fpu_getcw; __fpu_getcw;
> +  }
> +}

The public ABI exported by glibc should not depend on whether glibc itself 
was built as MIPS16; remember that these functions will be needed by 
MIPS16 code linked with a MIPS32 glibc.  So this Versions file should not 
be in a mips16 sysdeps directory.

> Index: ports/sysdeps/mips/sys/tas.h
> ===================================================================
> --- ports/sysdeps/mips/sys/tas.h	2013-01-17 00:55:17.000000000 +0000
> +++ ports/sysdeps/mips/sys/tas.h	2013-01-20 19:40:57.376549265 +0000
> @@ -24,7 +24,7 @@
>  
>  __BEGIN_DECLS
>  
> -extern int _test_and_set (int *__p, int __v) __THROW;
> +extern int __attribute__((nomips16)) test_and_set (int *__p, int __v) __THROW;
>  
>  #ifdef __USE_EXTERN_INLINES
>  
> @@ -32,7 +32,7 @@ extern int _test_and_set (int *__p, int 
>  #  define _EXTERN_INLINE __extern_inline
>  # endif
>  
> -_EXTERN_INLINE int
> +_EXTERN_INLINE int __attribute__((nomips16))
>  __NTH (_test_and_set (int *__p, int __v))
>  {
>    int __r, __t;

This is an installed header, so you need to use __nomips16__ rather than 
just plain nomips16.

> Index: ports/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ ports/sysdeps/unix/sysv/linux/mips/mips32/mips16/mips16-syscall.h	2013-01-20 19:40:57.546859991 +0000
> @@ -0,0 +1,91 @@
> +/* MIPS16 syscall wrappers.
> +   Copyright (C) 2013 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Maciej W. Rozycki <macro@codesourcery.com>.

We don't put "Contributed by" lines in new files in glibc.

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

All files in glibc should now use a URL instead of an FSF postal address.

These points apply similarly to other new files.

Please send a revised patch for review.

-- 
Joseph S. Myers
joseph@codesourcery.com