Problem with recent change to getlocalename_l

Problem with recent change to getlocalename_l

[Jeff Law]

We're seeing a few ports fail to build newlib after this change:

> commit 71511d4ac8686c2220093cc01525311d9c88bc4e
> Author: Corinna Vinschen <corinna@vinschen.de>
> Date:   Sun Jan 21 13:23:09 2024 +0100
> 
>     getlocalename_l: implement per SUS Base Specifications Issue 8 draft
>     
>       #include <locale.h>
>       const char *getlocalename_l(int category, locale_t locobj);
>     
>     Most notably, we need a per-thread space to store the string
>     returned if locobj is LC_GLOBAL_LOCALE.  No errors are defined
>     for getlocalename_l.  So we can't use buffer allocation which
>     might lead to an ENOMEM error.  We have to use a "static" buffer
>     in the per-thread state.
>     
>     Note that the feature test macro in locale.h is not quite correct.
>     This needs to be fixed as soon as the

pru-elf shows this failure:

  CC       libc/stdlib/libc_a-btowc.o
In file included from 
/home/jlaw/test/newlib-cygwin/newlib/libc/include/wchar.h:6,
                  from 
/home/jlaw/test/newlib-cygwin/newlib/libc/stdlib/btowc.c:1:
/home/jlaw/test/newlib-cygwin/newlib/libc/stdlib/btowc.c: In function 
'btowc':
/home/jlaw/test/newlib-cygwin/newlib/libc/stdlib/btowc.c:24:3: error: 
'struct _misc_reent' has no member named '_getlocale_l_buf'
    24 |   _REENT_CHECK_MISC(_REENT);
       |   ^~~~~~~~~~~~~~~~~

The tester is also seeing xstormy16-elf and msp430-elf fail in the same 
manner.

Jeff

Re: Problem with recent change to getlocalename_l

[Corinna Vinschen]

Hi Jeff,

[please do not cc me, I'm reading the mailing list all the time.  Thanks!]

On Feb  1 09:39, Jeff Law wrote:
> 
> We're seeing a few ports fail to build newlib after this change:
> 
> > commit 71511d4ac8686c2220093cc01525311d9c88bc4e
> > Author: Corinna Vinschen <corinna@vinschen.de>
> > Date:   Sun Jan 21 13:23:09 2024 +0100
> > 
> >     getlocalename_l: implement per SUS Base Specifications Issue 8 draft
> >       #include <locale.h>
> >       const char *getlocalename_l(int category, locale_t locobj);
> >     Most notably, we need a per-thread space to store the string
> >     returned if locobj is LC_GLOBAL_LOCALE.  No errors are defined
> >     for getlocalename_l.  So we can't use buffer allocation which
> >     might lead to an ENOMEM error.  We have to use a "static" buffer
> >     in the per-thread state.
> >     Note that the feature test macro in locale.h is not quite correct.
> >     This needs to be fixed as soon as the
> 
> pru-elf shows this failure:
> 
>  CC       libc/stdlib/libc_a-btowc.o
> In file included from
> /home/jlaw/test/newlib-cygwin/newlib/libc/include/wchar.h:6,
>                  from
> /home/jlaw/test/newlib-cygwin/newlib/libc/stdlib/btowc.c:1:
> /home/jlaw/test/newlib-cygwin/newlib/libc/stdlib/btowc.c: In function
> 'btowc':
> /home/jlaw/test/newlib-cygwin/newlib/libc/stdlib/btowc.c:24:3: error:
> 'struct _misc_reent' has no member named '_getlocale_l_buf'
>    24 |   _REENT_CHECK_MISC(_REENT);
>       |   ^~~~~~~~~~~~~~~~~
> 
> The tester is also seeing xstormy16-elf and msp430-elf fail in the same
> manner.

Given that this new functionality needs a 32 byte buffer, and given that
_REENT_SMALL targets are... well... small, I made the new buffer
optional via `#ifdef _MB_CAPABLE' in struct _misc_reent, which only used
by _REENT_SMALL targets.

Apparently I missed to take the _REENT_CHECK_MISC expression into account.
AFAICS the culprit is the _REENT_INIT_MISC macro now.

Can you please check if this change fixes the problem?

diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
index 4e60c3096ae2..caf554b9c26a 100644
--- a/newlib/libc/include/sys/reent.h
+++ b/newlib/libc/include/sys/reent.h
@@ -514,7 +514,7 @@ struct _reent
 #define _REENT_CHECK_EMERGENCY(var) \
   _REENT_CHECK(var, _emergency, char *, _REENT_EMERGENCY_SIZE, /* nothing */)
 
-#define _REENT_INIT_MISC(var) do { \
+#define __REENT_INIT_MISC(var) do { \
   struct _reent *_r = (var); \
   _r->_misc->_strtok_last = _NULL; \
   _r->_misc->_mblen_state.__count = 0; \
@@ -533,10 +533,18 @@ struct _reent
   _r->_misc->_wcrtomb_state.__value.__wch = 0; \
   _r->_misc->_wcsrtombs_state.__count = 0; \
   _r->_misc->_wcsrtombs_state.__value.__wch = 0; \
-  _r->_misc->_getlocale_l_buf[0] = '\0'; \
   _r->_misc->_l64a_buf[0] = '\0'; \
   _r->_misc->_getdate_err = 0; \
 } while (0)
+#ifdef _MB_CAPABLE
+#define _REENT_INIT_MISC(var) do { \
+  struct _reent *_r = (var); \
+  __REENT_INIT_MISC(_r) \
+  _r->_misc->_getlocale_l_buf[0] = '\0'; \
+} while (0)
+#else
+#define _REENT_INIT_MISC(var) __REENT_INIT_MISC(var)
+#endif
 #define _REENT_CHECK_MISC(var) \
   _REENT_CHECK(var, _misc, struct _misc_reent *, sizeof *((var)->_misc), _REENT_INIT_MISC(var))
 
Thanks,
Corinna

Re: Problem with recent change to getlocalename_l

[Corinna Vinschen]

No, wait.

On Feb  1 20:00, Corinna Vinschen wrote:
> Given that this new functionality needs a 32 byte buffer, and given that
> _REENT_SMALL targets are... well... small, I made the new buffer
> optional via `#ifdef _MB_CAPABLE' in struct _misc_reent, which only used
> by _REENT_SMALL targets.
> 
> Apparently I missed to take the _REENT_CHECK_MISC expression into account.
> AFAICS the culprit is the _REENT_INIT_MISC macro now.
> 
> Can you please check if this change fixes the problem?

Try this one instead, please:

diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
index 4e60c3096ae2..1fd503b9a699 100644
--- a/newlib/libc/include/sys/reent.h
+++ b/newlib/libc/include/sys/reent.h
@@ -514,8 +514,8 @@ struct _reent
 #define _REENT_CHECK_EMERGENCY(var) \
   _REENT_CHECK(var, _emergency, char *, _REENT_EMERGENCY_SIZE, /* nothing */)
 
-#define _REENT_INIT_MISC(var) do { \
-  struct _reent *_r = (var); \
+/* Do not call directly, use _REENT_INIT_MISC(var) instead */
+#define __REENT_INIT_MISC_BODY(_r) \
   _r->_misc->_strtok_last = _NULL; \
   _r->_misc->_mblen_state.__count = 0; \
   _r->_misc->_mblen_state.__value.__wch = 0; \
@@ -533,10 +533,20 @@ struct _reent
   _r->_misc->_wcrtomb_state.__value.__wch = 0; \
   _r->_misc->_wcsrtombs_state.__count = 0; \
   _r->_misc->_wcsrtombs_state.__value.__wch = 0; \
-  _r->_misc->_getlocale_l_buf[0] = '\0'; \
   _r->_misc->_l64a_buf[0] = '\0'; \
-  _r->_misc->_getdate_err = 0; \
+  _r->_misc->_getdate_err = 0;
+#ifdef _MB_CAPABLE
+#define _REENT_INIT_MISC(var) do { \
+  struct _reent *_r = (var); \
+  __REENT_INIT_MISC_BODY(_r) \
+  _r->_misc->_getlocale_l_buf[0] = '\0'; \
+} while (0)
+#else
+#define _REENT_INIT_MISC(var) do { \
+  struct _reent *_r = (var); \
+  __REENT_INIT_MISC_BODY(_r) \
 } while (0)
+#endif
 #define _REENT_CHECK_MISC(var) \
   _REENT_CHECK(var, _misc, struct _misc_reent *, sizeof *((var)->_misc), _REENT_INIT_MISC(var))
 

Re: Problem with recent change to getlocalename_l

[Corinna Vinschen]

On Feb  1 20:11, Corinna Vinschen wrote:
> No, wait.
> 
> On Feb  1 20:00, Corinna Vinschen wrote:
> > Given that this new functionality needs a 32 byte buffer, and given that
> > _REENT_SMALL targets are... well... small, I made the new buffer
> > optional via `#ifdef _MB_CAPABLE' in struct _misc_reent, which only used
> > by _REENT_SMALL targets.
> > 
> > Apparently I missed to take the _REENT_CHECK_MISC expression into account.
> > AFAICS the culprit is the _REENT_INIT_MISC macro now.
> > 
> > Can you please check if this change fixes the problem?
> 
> Try this one instead, please:

Sorry, I suddenly realized that I screwed up the name of the new member
as well.  Sigh.

Please try this one, it's also much simpler:

diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
index 4e60c3096ae2..0cba1667cc66 100644
--- a/newlib/libc/include/sys/reent.h
+++ b/newlib/libc/include/sys/reent.h
@@ -514,6 +514,11 @@ struct _reent
 #define _REENT_CHECK_EMERGENCY(var) \
   _REENT_CHECK(var, _emergency, char *, _REENT_EMERGENCY_SIZE, /* nothing */)
 
+#ifdef _MB_CAPABLE
+#define __REENT_INIT_MISC_GETLOCALENAME_L _r->_misc->_getlocalename_l_buf[0] = '\0'
+#else
+#define __REENT_INIT_MISC_GETLOCALENAME_L
+#endif
 #define _REENT_INIT_MISC(var) do { \
   struct _reent *_r = (var); \
   _r->_misc->_strtok_last = _NULL; \
@@ -533,7 +538,7 @@ struct _reent
   _r->_misc->_wcrtomb_state.__value.__wch = 0; \
   _r->_misc->_wcsrtombs_state.__count = 0; \
   _r->_misc->_wcsrtombs_state.__value.__wch = 0; \
-  _r->_misc->_getlocale_l_buf[0] = '\0'; \
+  __REENT_INIT_MISC_GETLOCALENAME_L; \
   _r->_misc->_l64a_buf[0] = '\0'; \
   _r->_misc->_getdate_err = 0; \
 } while (0)


Sorry,
Corinna

Re: Problem with recent change to getlocalename_l

[Dimitar Dimitrov]

On Thu, Feb 01, 2024 at 08:44:32PM +0100, Corinna Vinschen wrote:
> On Feb  1 20:11, Corinna Vinschen wrote:
> > No, wait.
> > 
> > On Feb  1 20:00, Corinna Vinschen wrote:
> > > Given that this new functionality needs a 32 byte buffer, and given that
> > > _REENT_SMALL targets are... well... small, I made the new buffer
> > > optional via `#ifdef _MB_CAPABLE' in struct _misc_reent, which only used
> > > by _REENT_SMALL targets.
> > > 
> > > Apparently I missed to take the _REENT_CHECK_MISC expression into account.
> > > AFAICS the culprit is the _REENT_INIT_MISC macro now.
> > > 
> > > Can you please check if this change fixes the problem?
> > 
> > Try this one instead, please:
> 
> Sorry, I suddenly realized that I screwed up the name of the new member
> as well.  Sigh.
> 
> Please try this one, it's also much simpler:
> 
> diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
> index 4e60c3096ae2..0cba1667cc66 100644
> --- a/newlib/libc/include/sys/reent.h
> +++ b/newlib/libc/include/sys/reent.h
> @@ -514,6 +514,11 @@ struct _reent
>  #define _REENT_CHECK_EMERGENCY(var) \
>    _REENT_CHECK(var, _emergency, char *, _REENT_EMERGENCY_SIZE, /* nothing */)
>  
> +#ifdef _MB_CAPABLE
> +#define __REENT_INIT_MISC_GETLOCALENAME_L _r->_misc->_getlocalename_l_buf[0] = '\0'
> +#else
> +#define __REENT_INIT_MISC_GETLOCALENAME_L
> +#endif
>  #define _REENT_INIT_MISC(var) do { \
>    struct _reent *_r = (var); \
>    _r->_misc->_strtok_last = _NULL; \
> @@ -533,7 +538,7 @@ struct _reent
>    _r->_misc->_wcrtomb_state.__value.__wch = 0; \
>    _r->_misc->_wcsrtombs_state.__count = 0; \
>    _r->_misc->_wcsrtombs_state.__value.__wch = 0; \
> -  _r->_misc->_getlocale_l_buf[0] = '\0'; \
> +  __REENT_INIT_MISC_GETLOCALENAME_L; \
>    _r->_misc->_l64a_buf[0] = '\0'; \
>    _r->_misc->_getdate_err = 0; \
>  } while (0)
> 

Hi,

This fixes the build for pru-unknown-elf, and there are no regressions
when running the GCC and newlib testsuites.

Regards,
Dimitar

> 
> Sorry,
> Corinna
> 

Re: Problem with recent change to getlocalename_l

[Jeff Law]

On 2/4/24 12:34, Dimitar Dimitrov wrote:
> On Thu, Feb 01, 2024 at 08:44:32PM +0100, Corinna Vinschen wrote:
>> On Feb  1 20:11, Corinna Vinschen wrote:
>>> No, wait.
>>>
>>> On Feb  1 20:00, Corinna Vinschen wrote:
>>>> Given that this new functionality needs a 32 byte buffer, and given that
>>>> _REENT_SMALL targets are... well... small, I made the new buffer
>>>> optional via `#ifdef _MB_CAPABLE' in struct _misc_reent, which only used
>>>> by _REENT_SMALL targets.
>>>>
>>>> Apparently I missed to take the _REENT_CHECK_MISC expression into account.
>>>> AFAICS the culprit is the _REENT_INIT_MISC macro now.
>>>>
>>>> Can you please check if this change fixes the problem?
>>>
>>> Try this one instead, please:
>>
>> Sorry, I suddenly realized that I screwed up the name of the new member
>> as well.  Sigh.
>>
>> Please try this one, it's also much simpler:
[ ... ]


>>
> 
> Hi,
> 
> This fixes the build for pru-unknown-elf, and there are no regressions
> when running the GCC and newlib testsuites.
Yea, that worked for me as well.  All the usual crosses built and passed 
with the exception of rl78, but I don't think that's a newlib problem, 
it's more likely a compiler issue.

Thanks for the testing Corinna's patch too!

jeff

Re: Problem with recent change to getlocalename_l

[Corinna Vinschen]

On Feb  4 19:39, Jeff Law wrote:
> On 2/4/24 12:34, Dimitar Dimitrov wrote:
> > On Thu, Feb 01, 2024 at 08:44:32PM +0100, Corinna Vinschen wrote:
> > > On Feb  1 20:11, Corinna Vinschen wrote:
> > > > No, wait.
> > > > 
> > > > On Feb  1 20:00, Corinna Vinschen wrote:
> > > > > Given that this new functionality needs a 32 byte buffer, and given that
> > > > > _REENT_SMALL targets are... well... small, I made the new buffer
> > > > > optional via `#ifdef _MB_CAPABLE' in struct _misc_reent, which only used
> > > > > by _REENT_SMALL targets.
> > > > > 
> > > > > Apparently I missed to take the _REENT_CHECK_MISC expression into account.
> > > > > AFAICS the culprit is the _REENT_INIT_MISC macro now.
> > > > > 
> > > > > Can you please check if this change fixes the problem?
> > > > 
> > > > Try this one instead, please:
> > > 
> > > Sorry, I suddenly realized that I screwed up the name of the new member
> > > as well.  Sigh.
> > > 
> > > Please try this one, it's also much simpler:
> [ ... ]
> 
> 
> > > 
> > 
> > Hi,
> > 
> > This fixes the build for pru-unknown-elf, and there are no regressions
> > when running the GCC and newlib testsuites.
> Yea, that worked for me as well.  All the usual crosses built and passed
> with the exception of rl78, but I don't think that's a newlib problem, it's
> more likely a compiler issue.
> 
> Thanks for the testing Corinna's patch too!
> 
> jeff

Thanks for testing guys, I pushed the patch.


Corinna

Re: Problem with recent change to getlocalename_l

[Torbjorn SVENSSON]

On 2024-02-01 20:11, Corinna Vinschen wrote:
> No, wait.
> 
> On Feb  1 20:00, Corinna Vinschen wrote:
>> Given that this new functionality needs a 32 byte buffer, and given that
>> _REENT_SMALL targets are... well... small, I made the new buffer
>> optional via `#ifdef _MB_CAPABLE' in struct _misc_reent, which only used
>> by _REENT_SMALL targets.
>>
>> Apparently I missed to take the _REENT_CHECK_MISC expression into account.
>> AFAICS the culprit is the _REENT_INIT_MISC macro now.
>>
>> Can you please check if this change fixes the problem?
> 
> Try this one instead, please:
> 
> diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
> index 4e60c3096ae2..1fd503b9a699 100644
> --- a/newlib/libc/include/sys/reent.h
> +++ b/newlib/libc/include/sys/reent.h
> @@ -514,8 +514,8 @@ struct _reent
>   #define _REENT_CHECK_EMERGENCY(var) \
>     _REENT_CHECK(var, _emergency, char *, _REENT_EMERGENCY_SIZE, /* nothing */)
>   
> -#define _REENT_INIT_MISC(var) do { \
> -  struct _reent *_r = (var); \
> +/* Do not call directly, use _REENT_INIT_MISC(var) instead */

Maybe do an #undef after the _REENT_INIT_MISC has been define to prevent 
the __REENT_INIT_MISC_BODY symbol to be visible outside this block would 
work here?

> +#define __REENT_INIT_MISC_BODY(_r) \
>     _r->_misc->_strtok_last = _NULL; \
>     _r->_misc->_mblen_state.__count = 0; \
>     _r->_misc->_mblen_state.__value.__wch = 0; \
> @@ -533,10 +533,20 @@ struct _reent
>     _r->_misc->_wcrtomb_state.__value.__wch = 0; \
>     _r->_misc->_wcsrtombs_state.__count = 0; \
>     _r->_misc->_wcsrtombs_state.__value.__wch = 0; \
> -  _r->_misc->_getlocale_l_buf[0] = '\0'; \
>     _r->_misc->_l64a_buf[0] = '\0'; \
> -  _r->_misc->_getdate_err = 0; \
> +  _r->_misc->_getdate_err = 0;
> +#ifdef _MB_CAPABLE
> +#define _REENT_INIT_MISC(var) do { \
> +  struct _reent *_r = (var); \
> +  __REENT_INIT_MISC_BODY(_r) \
> +  _r->_misc->_getlocale_l_buf[0] = '\0'; \
> +} while (0)
> +#else
> +#define _REENT_INIT_MISC(var) do { \
> +  struct _reent *_r = (var); \
> +  __REENT_INIT_MISC_BODY(_r) \
>   } while (0)
> +#endif

Something like this here:

#undef __REENT_INIT_MISC_BODY

>   #define _REENT_CHECK_MISC(var) \
>     _REENT_CHECK(var, _misc, struct _misc_reent *, sizeof *((var)->_misc), _REENT_INIT_MISC(var))
>   
> 

Re: Problem with recent change to getlocalename_l

[Corinna Vinschen]

On Feb  1 21:55, Torbjorn SVENSSON wrote:
> On 2024-02-01 20:11, Corinna Vinschen wrote:
> > No, wait.
> > 
> > On Feb  1 20:00, Corinna Vinschen wrote:
> > > Given that this new functionality needs a 32 byte buffer, and given that
> > > _REENT_SMALL targets are... well... small, I made the new buffer
> > > optional via `#ifdef _MB_CAPABLE' in struct _misc_reent, which only used
> > > by _REENT_SMALL targets.
> > > 
> > > Apparently I missed to take the _REENT_CHECK_MISC expression into account.
> > > AFAICS the culprit is the _REENT_INIT_MISC macro now.
> > > 
> > > Can you please check if this change fixes the problem?
> > 
> > Try this one instead, please:
> > 
> > diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
> > index 4e60c3096ae2..1fd503b9a699 100644
> > --- a/newlib/libc/include/sys/reent.h
> > +++ b/newlib/libc/include/sys/reent.h
> > @@ -514,8 +514,8 @@ struct _reent
> >   #define _REENT_CHECK_EMERGENCY(var) \
> >     _REENT_CHECK(var, _emergency, char *, _REENT_EMERGENCY_SIZE, /* nothing */)
> > -#define _REENT_INIT_MISC(var) do { \
> > -  struct _reent *_r = (var); \
> > +/* Do not call directly, use _REENT_INIT_MISC(var) instead */
> 
> Maybe do an #undef after the _REENT_INIT_MISC has been define to prevent the
> __REENT_INIT_MISC_BODY symbol to be visible outside this block would work
> here?

No, this patch was dumb.  Better give the one from
https://sourceware.org/pipermail/newlib/2024/021006.html
a try.


Thanks,
Corinna