[PATCH 3/3] Remove locale access for _REENT_SMALL

[Corinna Vinschen]

Hi Jaap,


Review inline.

On Feb 16 19:46, Jaap de Wolff wrote:
> diff --git a/newlib/libc/locale/duplocale.c b/newlib/libc/locale/duplocale.c
> index d3e7d782e..432c512f1 100644
> --- a/newlib/libc/locale/duplocale.c
> +++ b/newlib/libc/locale/duplocale.c
> @@ -35,6 +35,7 @@ PORTABILITY
>  */
>  
>  #include <newlib.h>
> +#include <errno.h>
>  #include <reent.h>
>  #include <stdlib.h>
>  #include "setlocale.h"
> @@ -42,6 +43,10 @@ PORTABILITY
>  struct __locale_t *
>  _duplocale_r (struct _reent *p, struct __locale_t *locobj)
>  {
> +#ifdef _REENT_SMALL
> +  p->_errno = ENOMEM;

ENOSYS, please.

> @@ -239,6 +239,7 @@ const struct __locale_t __C_locale =
>  };
>  #endif /* _MB_CAPABLE */
>  
> +#ifndef _REENT_SMALL
>  struct __locale_t __global_locale =

Don't you want to skip __C_locale as well for _REENT_SMALL? 

> @@ -288,6 +291,17 @@ static char *currentlocale (void);
>  
>  #endif /* _MB_CAPABLE */
>  
> +#ifdef _REENT_SMALL
> +const char *default_decimal_point = ".";
> +
> +const char *
> +_locale_decimalpoint (void *locale)
> +{
> +  return default_decimal_point;
> +}
> +
> +#endif
> +

Why not in a header as inline or macro, kind of like

  #define _locale_decimalpoint (_dummy) "."

?

>  const char *
>  __locale_ctype_ptr_l (struct __locale_t *locale)
>  {
> +#ifdef _REENT_SMALL
> +  return DEFAULT_CTYPE_PTR;
> +#else
>    return locale->ctype_ptr;
> +#endif
> +
>  }

Inline?  Macro?

>  const char *
>  __locale_ctype_ptr (void)
>  {
> +#ifdef _REENT_SMALL
> +  return DEFAULT_CTYPE_PTR;
> +#else
>    return __get_current_locale ()->ctype_ptr;
> +#endif
>  }

Same here?

>  #include <reent.h>
>  #include "setlocale.h"
>  
> +#ifndef _REENT_SMALL
> +
>  struct lconv *
>  __localeconv_l (struct __locale_t *locale)
>  {
> @@ -65,4 +67,8 @@ localeconv (void)
>  {
>    return __localeconv_l (__get_current_locale ());
>  }
> +#endif /* _REENT_SMALL */

Hang on, that doesn't make sense.  In case of newlocale, duplocale, etc.
you keep the functions and just return an error, while in case of of
localeconv you disable the entire function.  Please note that localeconv
existed for _REENT_SMALL targets before introducing per-thread locales,
check the git history.  The difference way back when was that the
localeconv info resided in a global lconv variable instead.  You may want
to retain the localconv info for small targets.

> diff --git a/newlib/libc/locale/newlocale.c b/newlib/libc/locale/newlocale.c
> index c6c2a9ca9..a696cbc0c 100644
> --- a/newlib/libc/locale/newlocale.c
> +++ b/newlib/libc/locale/newlocale.c
> @@ -86,6 +86,10 @@ struct __locale_t *
>  _newlocale_r (struct _reent *p, int category_mask, const char *locale,
>  	      struct __locale_t *base)
>  {
> +#ifdef _REENT_SMALL
> +  p->_errno = ENOMEM;

ENOSYS

> diff --git a/newlib/libc/locale/nl_langinfo.c b/newlib/libc/locale/nl_langinfo.c
> index eb984912f..882dc77b6 100644
> --- a/newlib/libc/locale/nl_langinfo.c
> +++ b/newlib/libc/locale/nl_langinfo.c
> @@ -331,6 +331,7 @@ do_codeset:
>  		break;
>  	case CRNCYSTR:
>  		ret = "";
> +#ifndef _REENT_SMALL
>  		cs = (char*) __get_monetary_locale (locale)->currency_symbol;
>  		if (*cs != '\0') {
>  			char pos = __localeconv_l (locale)->p_cs_precedes;

See above in terms of localconv info.

>  #endif /* _MB_CAPABLE */
>  
> -extern struct lconv *__localeconv_l (struct __locale_t *locale);
> -
>  extern size_t _wcsnrtombs_l (struct _reent *, char *, const wchar_t **,
>  			     size_t, size_t, mbstate_t *, struct __locale_t *);
>  
> +
> +#ifndef _REENT_SMALL
> +
> +extern struct lconv *__localeconv_l (struct __locale_t *locale);
> +

Same here.

>  #ifdef __HAVE_LOCALE_INFO__
>  _ELIDABLE_INLINE const struct lc_ctype_T *
> @@ -276,14 +280,18 @@ __get_current_ctype_locale (void)
>  _ELIDABLE_INLINE int
>  __locale_mb_cur_max_l (struct __locale_t *locale)
>  {
> +#ifdef _REENT_SMALL
> +	return _MB_LEN_MAX;
> +#else

This is incorrect.  MB_CUR_MAX != MB_LEN_MAX.  You have to return the
max value for the current charset here, and you have to store the info
somewhere.  In the pre per-thread locale days this function returned the
value of the global variable __mb_cur_max which got set in loadlocale
(now __loadlocale) from the setting of the local var mbc_max which still
exists.  Just returning MB_LEN_MAX here is an invalid reduction and may
lead to misbehaving user code.

> +#ifdef _REENT_SMALL
> +const char *_locale_decimalpoint(void *locale);
> +#define __CURRENT_LOCALE (locale_t)0
> +#ifdef __CYGWIN__

__CYGWIN__ is *never* _REENT_SMALL.  Just drop this.

> +#define _locale_wctomb(loc,r,buff,wc,ps) __utf8_wctomb(r,buff,wc,ps)
> +#define _locale_mbtowc(loc,r,buff,wc,ps) __utf8_mbtowc(r,buff,wc,ps)
> +#define __WCTOMB __utf8_wctomb
> +#define __MBTOWC __utf8_mbtowc
> +#else
> +#define _locale_wctomb(loc,r,buff,wc,ps) __ascii_wctomb(r,buff,wc,ps)
> +#define _locale_mbtowc(loc,r,buff,wc,ps) __ascii_mbtowc(r,buff,wc,ps)
> +#define __WCTOMB __ascii_wctomb
> +#define __MBTOWC __ascii_mbtowc

But this is wrong.  You're basically switching off *any* multibyte
charset handling on small targets.  But newlib supports UTF-8, EUCJP,
SJIS on small targets as well and did so since 2000 or so.

I don't think everybody will be happy to get this support disabled.

> +#ifdef _REENT_SMALL
> +  p->_errno = ENOMEM;

ENOSYS

I'd like to ask: Do you *really* want to get rid of __global_locale,
or are you just unhappy in terms of its size?  Note that I didn't 
add any extra locale info to targets not defining __HAVE_LOCALE_INFO__.
The only difference is that everything is now in one place and thus
takes memory even if only parts are used.  Let's take a closer look.
Assuming a 32 bit target:

struct __locale_t
{
  char                   categories[...];	// 224 bytes
  int                   (*wctomb)		//   4 bytes
  int                   (*mbtowc)		//   4 bytes
  int                    cjk_lang;		//   4 bytes
  char                  *ctype_ptr;		//   4 bytes
  struct lconv           lconv;			//  96 bytes
  char                   mb_cur_max[2];		//   2 bytes
  char                   ctype_codeset[];	//  32 bytes
  char                   message_codeset[];	//  32 bytes
};

Again, there's nothing in that struct which wasn't there before,
and especially codeset support was required for all targets.

The difference is that the categories and lconv only existed if
the application called setlocale or localeconv.

So, what about just moving the biggest offenders, categories and lconv,
out of the struct and make sure they only exist on a REENT_SMALL target
if it calls setlocale or localeconv?


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/newlib/attachments/20180219/d7178148/attachment.sig>