[<prev] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20261024215214.GE23438@brightrain.aerifal.cx>
Date: Sat, 24 Oct 2026 17:52:14 -0400
From: Rich Felker <dalias@...c.org>
To: Luca Kellermann <mailto.luca.kellermann@...il.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH v2] musl locale overhaul series, take 2
On Sat, Oct 24, 2026 at 09:45:35PM +0200, Luca Kellermann wrote:
> On Fri, Oct 02, 2026 at 01:38:00AM -0400, Rich Felker wrote:
> > The attached patch series is the current contents of my integration
> > branch for the locale support overhaul project, funded by NLnet and
> > the NGI Zero Core Fund. Compared to the v1 draft, this is now largely
> > in the form it will be merged to the master branch after the 1.2.7
> > bugfix release. Significant additional work has been done to eliminate
> > most of the increased size of global data for locale, and to reduce
> > code churn at the same time.
> >
> > As before these are factored into small commits to admit individual
> > assessment both now and in the historical record. The high-level
> > changes are:
>
> I reviewed most changes in detail and found a few bugs, see below. I
> also suggest some other changes.
Thank you for the detailed review! Replies inline below:
> I didn't review the logic for collation and strfmon().
>
> > - Updates of built-in character data to Unicode 17. This is entirely
> > mechanical, generated by musl-chartable-tools[1], probably not
> > intersting to read. The bugs present in the v1 patch series due to
> > parse errors during table generation are now fixed.
>
> Unicode 18 is available since september.
It wsa in draft when I applied the 17 tables. I can update to 18 now.
> > [...]
> >
> > - Linear-search-free versions of all of the error message lookup
> > interfaces (strerror, hstrerror, gai_strerror, regerror), signal
> > name lookup (strsignal), nl_langinfo (this is the backend for
> > localized data/time formatting), and gettext error messages. These
>
> getopt, not gettext.
Thanks. This is just a mistake in the email, yes?
> > now all use the new string table lookup infrastructure for both
> > built-in and locale string tables.
> >
> > [...]
> >
> > From 0f52112210365e9cf9ee139c1bdcfa1ce4dc887c Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Sat, 12 Sep 2026 18:02:56 -0400
> > Subject: [PATCH 03/23] add integer-keyed multi-level table lookup for new
> > locale support
> >
> > [...]
> >
> > diff --git a/src/internal/ikmlt.c b/src/internal/ikmlt.c
> > [...]
> > +/* ikmlt_lookup_path takes a path of keys (first element of path is
> > + * the number of components) starting from the tabel ld and traversing
>
> Typo: tabel
Thanks, will fix.
>
> > [...]
> > diff --git a/src/internal/ikmlt.h b/src/internal/ikmlt.h
> > [...]
> > +#define ikmlt_lookup __ikmlt_lookup
> > +#define ikmlt_lookup_path __ikmlt_lookup_path
> > +
> > +hidden const void *ikmlt_lookup(const unsigned char *, int);
> > +hidden const void *ikmlt_lookup_path(const unsigned char *, const int *);
>
> Are these defines really worthwhile just to safe a few characters?
It's not to save a few characters but so that the file ikmlt.c is
usable outside of libc. If it used the __-prefixed names directly,
that would be UB, so any reuse of the file would have to carry a
patch. Not really a big deal just, but I thought it was nicer to
avoid.
> > From 42c57b8d8505d79c4cba540a1d137999a95f5b53 Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Sat, 12 Sep 2026 20:41:12 -0400
> > Subject: [PATCH 04/23] add common __loc_lookup backend for new locale
> >
> > [...]
> >
> > diff --git a/src/locale/__loc_lookup.c b/src/locale/__loc_lookup.c
> > [...]
> > +const char *__loc_lookup(const unsigned char *ptable, const unsigned char *ltable, const int *path, const int *fallback)
> > +{
>
> Changing the type of ptable to const void * would remove the need for
> a cast at every callsite.
Hm, that may be worthwhile.
>
> > + if (!path) return 0;
> > + const char *pstr = ikmlt_lookup_path(ptable, path);
> > + if (!pstr) return __loc_lookup(ptable, ltable, fallback, 0);
> > + if (!ltable) return (char *)pstr;
> > + const char *lstr = ikmlt_lookup_path(ltable, path);
> > + return lstr ? (char *)lstr : (char *)pstr;
>
> The casts to char * seem unnecessary.
Indeed. I think that carried over from a draft that had it returning
char * so that strerror and strsignal would not need casts. It's
probably better to keep const-correctness until the last moment when
strerror and strsignal return.
> > From deba1b9ed3762e79c232ebf9df6e72cbe79b8fc4 Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Sat, 12 Sep 2026 22:24:28 -0400
> > Subject: [PATCH 05/23] add include frameworks for building built-in C locale
> > string tables
> >
> > [...]
> >
> > diff --git a/src/internal/mdata.h b/src/internal/mdata.h
> > [...]
> > +#define M_3(a,b,c,...) c
> > +#define M_S(...) M_3(,,__VA_ARGS__)
> > +#define M_N(...) M_3(,__VA_ARGS__)
> > +#define M_B(...) M_3(__VA_ARGS__)
>
> M_B() should be defined as M_3(__VA_ARGS__,). Otherwise you get the
> following with -pedantic: warning: ISO C99 requires at least one
> argument for the "..." in a variadic macro
OK.
> > +.header = {
> > + 0xff & (M_B(M_TABLE)>>24),
> > + 0xff & (M_B(M_TABLE)>>16),
> > + 0xff & (M_B(M_TABLE)>>8),
> > + 0xff & (M_B(M_TABLE)),
>
> M_B(M_TABLE) will sometimes be a negative int. Shifting negative
> values is implementation-defined. Maybe convert to unsigned before?
That's probably the right fix.
> > + 0, 1,
> > + (sizeof (unsigned char[]){
>
> char would suffice. The array is only used to count.
Indeed.
> > +#define M(n, s) [(n)-(M_B(M_TABLE))] = 0,
> > +#include M_S(M_TABLE)
> > +#undef M
> > + } - 1) >> 8,
> > + (sizeof (unsigned char[]){
> > +#define M(n, s) [(n)-(M_B(M_TABLE))] = 0,
> > +#include M_S(M_TABLE)
> > +#undef M
>
> M() is identically defined twice here, maybe just define it once?
Yes I think that works.
> > + } - 1) & 0xff,
> > +},
> > +.offsets = {
> > +#define M(n, s) \
> > + [2*((n)-(M_B(M_TABLE)))] = \
> > + (offsetof(struct M_N(M_TABLE), m_##n)+1)/256,\
> > + (offsetof(struct M_N(M_TABLE), m_##n)+1)%256,
>
> Otherwise this file uses >> and & instead of / and %.
>
> > [...]
> > diff --git a/src/internal/mdecl.h b/src/internal/mdecl.h
> > [...]
> > +#define M_3(a,b,c,...) c
> > +#define M_S(...) M_3(,,__VA_ARGS__)
> > +#define M_N(...) M_3(,__VA_ARGS__)
> > +#define M_B(...) M_3(__VA_ARGS__)
>
> Same here with M_B().
OK.
>
> > [...]
> >
> > From 0fdc2d64e40c52726fe216a48d5496448b41e935 Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Sat, 12 Sep 2026 23:40:38 -0400
> > Subject: [PATCH 06/23] switch langinfo and error strings over to new common
> > table lookup
> >
> > [...]
> >
> > errno numbers can vary by arch (only mips at present), but the keys
>
> powerpc (like mips) also has one additional error: EDEADLOCK (not the
> same as EDEADLK like on other archs), but see my comments below.
Yes, that's fine. We don't remap it because there is a slot number
reserved for it in the standard numbering, allowing it to have its own
message if desired.
> > used in the table need to be arch-agnostic, since locale files may be
> > shared across archs. the common arch/generic numbering is adopted,
> > with a translation table built conditionally for archs that don't
> > match. for now, EDEADLK != L_EDEADLK is used as a proxy condition; it
> > can be expanded or explicitly declared by the arch if needed for
> > future archs.
>
> If my other comments about EDEADLK and EDEADLOCK are followed, it
> might be worth considering to change the proxy condition (e.g. to
> ENOMSG != L_ENOMSG) to avoid confusion. There are two concerns here:
> completely different numbering (only mips) and whether EDEADLK and
> EDEADLOCK are distinct errors (mips and powerpc).
Yes, that's probably a good idea.
> > [...]
> >
> > diff --git a/src/errno/__strerror.h b/src/errno/__strerror.h
> > [...]
> > * with these messages, and then to define a lookup table translating
> > * error codes to offsets of corresponding fields in the structure. */
>
> This comment seems a bit out of date now, the lookup table and
> structure containing the messages are defined together at the same
> time.
What do you mean? The comment was written for the previously
strerror-only version of the same concept, and I think it still
applies just as much as it did before.
> > [...]
> > diff --git a/src/errno/l_errno.h b/src/errno/l_errno.h
> > [...]
> > +#define L_EAGAIN 11
> > [...]
> > +#define L_EWOULDBLOCK 41
>
> Why L_EAGAIN != L_EWOULDBLOCK? All supported archs have EAGAIN ==
> EWOULDBLOCK. Is L_EWOULDBLOCK needed at all?
I believe there are some as-yet-unsupported ones where they are not
equal, and regardless it should be possible in the locale data
representation to define distinct messages for them in case we later
support targets where they're distinct.
> This even leads to the problem that archs using remap_errno.h will
> return "Unknown error" from strerror(EAGAIN). The reason is that
> remap_errno looks like this:
>
> static unsigned char remap_errno[] = {
> // ...
> [EAGAIN] = L_EAGAIN, // [11] = 11,
> // ...
> [EWOULDBLOCK] = L_EWOULDBLOCK, // [11] = 41,
> // ...
> };
There should be a #if EWOULDBLOCK != EAGAIN around the second one.
> So remap_errno[11] will give 41 for which __strerror.h has no entry
> (no E(EWOULDBLOCK, "...")). Removing L_EWOULDBLOCK and its use in
> remap_errno.h would solve this. Comments in place of L_EWOULDBLOCK for
> l_errno.h and remap_errno.h could clarify why it's missing:
>
> /* L_EWOULDBLOCK not needed, EWOULDBLOCK == EAGAIN */
> /* L_EWOULDBLOCK does not exist */
>
> A similar problem could arise for a future arch where remap_errno.h is
> used but where EDEADLK == EDEADLOCK. This could be solved by only
> defining and using L_EDEADLOCK conditionally (similar to how
> strsignal() handles SIGSTKFLT and SIGEMT):
>
> l_errno.h:
>
> #if EDEADLK != EDEADLOCK
> #define L_EDEADLOCK 58
> #endif
L_DEADLOCK should always be defined. It is defining the keys for the
locale data format not a property of the arch. In principle l_errno.h
does not even have access to the errno.h macros.
> remap_errno.h:
>
> #ifdef L_EDEADLOCK
> [EDEADLOCK] = L_EDEADLOCK,
> #endif
#if EDEADLOCK != EDEADLK
[EDEADLOCK] = L_EDEADLOCK,
#endif
> __strerror.h (this message does not exist yet but could be added):
>
> #ifdef L_EDEADLOCK
> // see errno(3)
> E(EDEADLOCK, "File locking deadlock error")
> #endif
I think it would be:
#if EDEADLOCK != EDEADLK
E(EDEADLOCK, "File locking deadlock error")
#endif
> If a message for EDEADLOCK would instead be added unconditionally, all
> archs other than mips would return it for strerror(58), even though
> only powerpc has EDEADLOCK defined as 58. Other archs should return
> "Unknown error".
Yes, the intent is not to have errors which don't exist present on the
arch present in the linked-in table so that the values from a locale
file don't get used unless the arch you're running on actually has
those errors.
> > [...]
> > diff --git a/src/errno/remap_errno.h b/src/errno/remap_errno.h
> > [...]
> > +[EOPNOTSUPP] = L_EOPNOTSUPP,
>
> This could be [ENOTSUP] = L_ENOTSUP. Then L_EOPNOTSUPP could be
> removed from l_errno.h since __strerror.h uses L_ENOTSUP via
> E(ENOTSUP, "Not supported"). Defining both L_ENOTSUP and L_EOPNOTSUPP
> is a bit redundant, they have the same value. Comments like those I
> suggested above for L_EWOULDBLOCK would be nice.
Yes, it's probably a mistake to have both here since we don't have a
framework to support them being distinct.
> > [...]
> > diff --git a/src/errno/strerror.c b/src/errno/strerror.c
> > [...]
> > -#include <string.h>
>
> strerror() and strerror_r() are declared in string.h, so it should
> probably stay included.
Yes, thanks for catching that.
> > [...]
> > +enum { L_E0 = 0, L_E_ = -1 };
>
> Could L_E0 and L_E_ be defined in l_errno.h instead?
Yes, I think so.
> > +#define M_TABLE "../errno/__strerror.h", strerror_data, -1
>
> Shouldn't M_TABLE definitions use L_E_, EAI_OVERFLOW, H_, REG__, NEG1
> and GETOPT_INVAL instead of magic numbers for M_B()?
Yes, probably so.
> > +#define ERRDOMAIN_ERRNO 0
> > +
> > +static const struct {
> > + unsigned char header[8];
> > + unsigned char offsets[4];
> > + struct {
> > + #include "mdecl.h"
> > + } errors;
> > +} strerrors = {
> > + .header = { 0,0,0,0,0,0,0,4-1, },
> > + .offsets = { [ERRDOMAIN_ERRNO] = 1, },
> > + .errors = {
> > + #include "mdata.h"
> > + },
> > +};
>
> To save a few more bytes, these definitions could be written like
> this (use ERRDOMAIN_x as start to only have a single offset):
>
> static const struct {
> unsigned char header[8];
> unsigned char offsets[1];
> struct {
> #include "mdecl.h"
> } errors;
> } strerrors = {
> .header = { 0,0,0,ERRDOMAIN_ERRNO },
> .offsets = { 1 },
> .errors = {
> #include "mdata.h"
> },
> };
>
> Alignment shouldn't matter. The values are strings only and ikmlt
> reads the headers and offsets in an alignment-agnostic way.
I think my concern was whether we can safely assume a psABI does not
define an alignment for a structure >1 when the struct contains only
char[] data. I wrote it not to assume that. Maybe it doesn't matter.
> Repeat for all other ERRDOMAIN_s.
>
> > [...]
> > +static int common_errno(int e)
> > {
> > - const char *s;
> > +#if EDEADLK != L_EDEADLK
> > + static unsigned char remap_errno[] = {
> > + #include "remap_errno.h"
> > + };
> > [...]
> > + if (e >= sizeof remap_errno / sizeof *remap_errno) return -1;
>
> / sizeof *remap_errno can be omitted, it's always 1. This line would
That's true but it's not obviously correct to someone reading, and
depends on the remapping table never being enlarged past 256. This is
why I wrote it not to assume the type of the array.
> also be clearer if it returned L_E_: conceptionally common_errno()
> maps Ex to L_Ex and should return L_E_ for unknown values.
I'm fine with either. It does not have to be numerically equal to
L_E_, just a value that's invalid as an errno (which any negative
number is).
> > + return remap_errno[e];
>
> common_errno() should return L_E0 if !e and L_E_ if !remap_errno[e].
> Right now strerror(47) returns "No error information" instead of
> "Unknown error" on mips.
Oh, yes. Thanks!
> static int common_errno(int e)
> {
> #if ...
> static unsigned char remap_errno[] = {
> #include "remap_errno.h"
> };
> if (!e) return L_E0;
> #ifdef EDQUOT_ORIG
> if (e==EDQUOT) e=-1;
> else if (e==EDQUOT_ORIG) e=EDQUOT;
> #endif
> if (e >= sizeof remap_errno) return L_E_;
> e = remap_errno[e];
> return e ? e: L_E_;
> #else
> return e;
> #endif
> }
>
> > +#else
> > + return e;
> > +#endif
> > +}
> > +
> > +char *__strerror_l(int e, locale_t loc)
> > +{
> > + e = common_errno(e);
> > + static const int unknown[] = LPATH(ERRDOMAIN_ERRNO, -1);
>
> The unknown path should probably use L_E_ instead of -1.
As before I'm fairly indifferent to which is used.
> > [...]
> > diff --git a/src/locale/langinfo.c b/src/locale/langinfo.c
> > [...]
> > +#define M_TABLE_TIME "../locale/lc_time.h", lc_time_data, LC_TIME<<16
> > +#define M_TABLE_MESSAGES "../locale/lc_messages.h", lc_messages_data, LC_MESSAGES<<16
> > +#define M_TABLE_MONETARY "../locale/lc_monetary.h", lc_monetary_data, LC_MONETARY<<16
> > +#define M_TABLE_NUMERIC "../locale/lc_numeric.h", lc_numeric_data, LC_NUMERIC<<16
>
> LC_MONETARY (4) could start at CRNCYSTR (0x4000F).
I guess it would save a few bytes, yes.
> > +static const struct {
> > + unsigned char header[8];
> > + unsigned char offsets[12];
> > + struct langinfo_data {
> > + int :0;
>
> What's the purpose of these zero-width bit-fields? Alignment? This
> might be unnecessary, see my other comment about alignment.
For alignment in case misaligned accesses are significantly slower.
> > [...]
> > + } data;
> > +} c_langinfo = {
> > + .header = { 0,0,0,0,0,1,0,6-1, },
> > + .offsets = {
> > + O(LC_TIME, lc_time),
> > + O(LC_MESSAGES, lc_messages),
> > + O(LC_MONETARY, lc_monetary),
> > + O(LC_NUMERIC, lc_numeric),
> > + },
> > + .data = {
> > [...]
> > + },
> > +};
>
> If LC_NUMERIC (1) would be used as start, 2 bytes could be saved (or 4
> if LC_NUMERIC is removed and LC_TIME (2) is used as start, see below):
>
> static const struct {
> unsigned char header[8];
> unsigned char offsets[5*2]; // or 4*2
> struct langinfo_data {
> // ...
> } data;
> } c_langinfo = {
> .header = { 0,0,0,1,0,1,0,5-1 }, // or 4-1
> .offsets = {
> // or -2 each
> O(LC_TIME-1, lc_time),
> O(LC_MESSAGES-1, lc_messages),
> O(LC_MONETARY-1, lc_monetary),
> O(LC_NUMERIC-1, lc_numeric),
> },
> .data = {
> // ...
> },
> };
>
> > char *__nl_langinfo_l(nl_item item, locale_t loc)
> > {
> > int cat = item >> 16;
> > int idx = item & 65535;
> > - const char *str;
> > + int path[] = { 2, cat, item };
>
> Why isn't LPATH() used here?
Oversight or written before the macro existed. It should be changed to
use LPATH().
> > + static const int unknown[] = { 2, -1, -1 };
>
> This path doesn't lead to any value, so a null pointer could be passed
> to __loc_lookup() instead.
I think that works but there was a reason it did not work when first
written.
> > if (item == CODESET) return loc->cat[LC_CTYPE] ? "UTF-8" : "ASCII";
> > + //if (item == RADIXCHAR && loc->cat[LC_NUMERIC])
> > + // return loc->cat[LC_NUMERIC]->lconv.decimal_point;
>
> RADIXCHAR and THOUSEP could be implemented like this to avoid
> redundancy in the file format:
They actually can't because lconv is not present in the __locale_map
structure. It is derived from both LC_NUMERIC and LC_MONETARY combined
and thus is only present in the __locale_struct, and only if
localeconv is linked/used.
It would be possible to just dynamically lookup the lconv strings here
rather than using the langinfo table, but that's more code (linked in
every static binary, and cognitive burden to understand) which seems
like a bad tradeoff for a few bytes of automatically generate locale
data.
> if (item == RADIXCHAR) {
> static const int r_path[] = LPATH(K_LCONV, 0);
> const char *str = __ikmlt_lookup_path(
> LMEMB(loc, LC_NUMERIC, table_root), r_path);
> if (!str || str[0] != ',' || str[1]) str = ".";
> return (char *)str;
> }
> if (item == THOUSEP) {
> static const int t_path[] = LPATH(K_LCONV, 1);
> const char *str = __ikmlt_lookup_path(
> LMEMB(loc, LC_NUMERIC, table_root), t_path);
> return str ? (char *)str : "";
> }
>
> lc_numeric.h could then be removed and LC_TIME (2) could be used as
> start in c_langinfo.
Saving 4 bytes of table by spending ~100 bytes of code?
> Is it possible to synthesize CRNCYSTR from currency_symbol,
> p_cs_precedes and n_cs_precedes (or the other way around)? If so, I
> have some ideas how to get rid of it from the file format.
Yes but then you need writable runtime storage for it. Read-only
mappings are free. Trading a few bytes of read-only mappings for
read-write buffers per-process and significantly more code to generate
the data at runtime is a very bad tradeoff in the opposite direction
musl generally takes.
> > /* _NL_LOCALE_NAME extension */
> > - if (idx == 65535 && cat < LC_ALL)
> > + if (idx == 65535 && (unsigned)cat < LC_ALL)
>
> Instead of casting to unsigned here, maybe cast item to unsigned
> before shifting (shifting negative values is implementation-defined).
Yes, that's code that was already present tho and not really a big
deal. Either definition is fine here.
> > [...]
> > + const char *str = __loc_lookup((const void *)&c_langinfo,
> > + 0 /* LMEMB(CURRENT_LOCALE,cat,langinfo) */,
> > + path, unknown);
>
> This should use loc instead of CURRENT_LOCALE. There will also be an
> out of bounds access to loc->cat if cat < 0 or cat >= LC_ALL.
Thanks! That's a real bug.
>
> > [...]
> > diff --git a/src/locale/lc_messages.h b/src/locale/lc_messages.h
> > [...]
> > +M(YESEXPR, "^[yY]\0")
> > +M(NOEXPR, "^[nN]\0")
>
> \0 can be omitted now.
Yes, thanks for catching it.
> > [...]
> > diff --git a/src/locale/lc_time.h b/src/locale/lc_time.h
> > [...]
> > +M(D_T_FMT, "%a %b %e %T %Y")
>
> I think this should be M(D_T_FMT, "%a %b %e %H:%M:%S %Y"), see
> POSIX.1-2024 XBD 7.3.5.3 LC_TIME Category in the POSIX Locale [1].
They're equivalent but we probably should be using the officially
specified one.
> > [...]
> > diff --git a/src/regex/regerror.c b/src/regex/regerror.c
> > [...]
> > +enum { REG__ = -1 };
> > +#define M_TABLE "../regex/regerror.h", regerror_data, -1
>
> REG_ENOSYS is defined as -1 too, so adding a message for it will be
> more work than just adding M(REG_ENOSYS, "...") to regerror.h. Using
> -2 for REG__ would make it easier.
That was an oversight, but I don't think it matters. REG_ENOSYS was
removed in POSIX 2008. I don't think it needs a message since it can
never be returned.
> > From 0cfd41aab32b1c7e7eb93c28f8582dfa1586106d Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Mon, 14 Sep 2026 17:44:45 -0400
> > Subject: [PATCH 08/23] locale: remove synthesis of nonexistent locale names
> >
> > this change does not make setlocale fail for locale names lacking a
> > definition, but it eliminates the behavior of preserving the requested
> > name, instead handling them as aliases for C.UTF-8 (LC_CTYLE) or C
> > (all other categories).
>
> Typo: LC_CTYLE
Thanks.
> > this breaks gettext search for translations to languages where no libc
> > locale exists, but adding support for the new LANGUAGE environment
> > variable will mitigate that.
>
> Will this be added as part of the locale merge?
I'd like to get it done in the same release cycle even if we don't get
to other POSIX 2024 additions.
> > From 27359091d7126f0edaa30796eb102596446eacdb Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Mon, 14 Sep 2026 18:09:27 -0400
> > Subject: [PATCH 09/23] locale: make explicit request of unavailable locales
> > fail
> >
> > have __get_locale track whether the request is implicit (obtained via
> > environment for an argument of "") or explicit. if explicit, names
> > that fail to resove to a locale file will produce a return value of
> > LOC_MAP_FAILED, which setlocale and newlocale already interpret as an
> > error condition. if implicit, return C.UTF-8 for LC_CTYPE, and
> > otherwise a null pointer (interpreted as C locale).
>
> setlocale(LC_ALL, "some-very-very-very-very-very-very-long-name") will
> still not fail but behave as setlocale(LC_ALL, "C.UTF-8"). setlocale()
> could be changed like this:
Why do you think that will happen? If the loop that checks the name
length and characters present ends early, val[n] is nonzero and we
take the goto fail. If !implicit, this returns LOC_MAP_FAILED. Only
for setlocale(LC_ALL /* or LC_CTYPE */, "") with the LC_* vars set to
something invalid will you get C.UTF_8.
> char *setlocale(int cat, const char *name)
> {
> // ...
> if (cat == LC_ALL) {
> int i;
> if (name) {
> struct __locale_struct tmp_locale;
> const char *p = name;
> for (i=0; i<LC_ALL; i++) {
> const char *z = __strchrnul(p, ';');
> if (z-p > LOCALE_NAME_MAX) {
> UNLOCK(__locale_lock);
> return 0;
> }
> char part[LOCALE_NAME_MAX+1];
> memcpy(part, p, z-p);
> part[z-p] = 0;
> if (*z) p = z+1;
> lm = __get_locale(i, part);
> // ...
> }
> // ...
> }
> // ...
> }
> // ...
> }
We already have an error-out path for LOC_MAP_FAILED.
> > diff --git a/src/locale/locale_map.c b/src/locale/locale_map.c
> > [...]
> > +fail:
> > + if (!implicit) return LOC_MAP_FAILED;
>
> As __get_locale() can now return LOC_MAP_FAILED, it must ensure that
> errno is set to an appropriate value so newlocale() can pass it on.
> This is currently not the case if called with an invalid locale name
> (leading dot, slash, too long) or if the path search doesn't actually
> try to map any file. Could be solved like this:
Oh, I looked at adding error codes but saw that setlocale does not
define errors, and dropped it. Indeed if newlocale needs them I should
add them.
> const struct __locale_map *__get_locale(int cat, const char *val)
> {
> // ...
> int err = EINVAL; // or ENOENT, I'm not sure
EINVAL from newlocale is for invalid mask bit. ENOENT is needed here.
> > From 9b7e95d577305f6142e3c28273c973cfd586680a Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Tue, 15 Sep 2026 19:01:33 -0400
> > Subject: [PATCH 10/23] locale: add loader for new locale format & adjust
> > related logic
> >
> > [...]
> >
> > diff --git a/src/internal/locale_impl.h b/src/internal/locale_impl.h
> > [...]
> > + K_ERRORS,
> > [...]
> > + const unsigned char *errors;
>
> This also includes strings for strsignal() and getopt(). These are not
> really errors. Would K_STRINGS/strings/STRDOMAIN_x be better names?
Signals are at least something error-like. This table is not for
arbitrary strings (like messages, langinfo strings, etc.) but things
that are particularly error-code-like. But this is not really a public
facing interface surface so it doesn't matter so much; it's more of a
bikeshed.
> > [...]
> > diff --git a/src/locale/c_locale.c b/src/locale/c_locale.c
> > [...]
> > #include <stdint.h>
> >
> > -static const uint32_t empty_mo[] = { 0x950412de, 0, -1, -1, -1 };
>
> This no longer needs to include stdint.h.
Thanks.
> > [...]
> > diff --git a/src/locale/locale_map.c b/src/locale/locale_map.c
> > [...]
> > +#include <stddef.h>
>
> Why include stddef.h?
It can be removed. The lconv expansion code was originally here, but
moved to localeconv.c when I realized it's not per __locale_map but
associated with the __locale_struct.
> > [...]
> > +static int load_locale(struct __locale_map *l, const unsigned char *lm)
> > +{
> > + const unsigned char *root;
> > +
> > + if (memcmp(lm, "ikmlt\xff\xc0\xc1loc2\0\0\0\0", 16)) {
>
> Does it make sense to ensure that map_size > (16 + 8) (magic + header
> of root table) before?
>
> I'm also curious. Does this magic value have a meaning?
ikmlt is the table data format. \xff\xc0\xc1 is bytes to ensure the
file can't be interpreted as text (invalid in UTF-8). loc2 is "locale,
take 2" and the zeros are just to pad it up to a nice multiple for the
base table alignment in case we want to preserve a high alignment for
anything (can't align anything more than the base).
The zeros could be used for versioning information but I don't think
that makes sense. It's better to use the table structure itself for
that kind of information.
> > + errno = EINVAL;
> > + return -1;
> > + }
>
> Is EINVAL the right error for newlocale() in this situation? Or would
> ENOENT (or some other error) be more appropriate?
ENOENT or some other error. Overloading EINVAL to mean something
different than it's specified for is not conforming.
> > + l->table_root = root = lm+16;
> > +
>
> This empty line has a trailing tab.
I don't think we strongly avoid that as long as the spurious
indentation matches, but it'd be better to remove it. Thanks.
>
> > [...]
> > const struct __locale_map *__get_locale(int cat, const char *val)
> > {
> > [...]
> > + struct __locale_map tmp;
> > + if (load_locale(&tmp, map) ||
> > + !(new = malloc(sizeof *new))) {
> > __munmap((void *)map, map_size);
> > break;
> > }
>
> Couldn't the path search continue when seeing an invalid file
> (load_locale() returns -1)?
I think it would be safe to continue, but I'm not sure it's
well-motivated. Why would you put invalid files in the locale path
except if you wanted to suppress that locale from getting loaded from
a later path component?
> > - new->map = map;
> > - new->map_size = map_size;
> > + *new = tmp;
> > memcpy(new->name, val, n);
> > new->name[n] = 0;
> > new->next = loc_head;
>
> Writing it this way would avoid copying a partially uninitialized tmp
> (even when dummy_collation_init() is used later):
>
> struct __locale_map tmp = { 0 };
> if (load_locale(&tmp, map) ||
> !(new = malloc(sizeof *new))) {
> // ...
> break;
> }
> memcpy(tmp.name, val, n);
> tmp.name[n] = 0;
> tmp.next = loc_head;
> *new = tmp;
> loc_head = new;
I guess something like this could be done. Is there a motivation for
avoiding the uninitialized contents that won't be used?
> > From 5ddebacde40d9f0150cd22ce8066a1ae7d6f90fd Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Mon, 21 Sep 2026 18:27:14 -0400
> > Subject: [PATCH 13/23] convert strsignal to use new locale string table
> > lookups
> >
> > [...]
> >
> > diff --git a/src/string/strsignal.c b/src/string/strsignal.c
> > [...]
> > #include <string.h>
> > #include "locale_impl.h"
>
> This file doesn't compile at this point in the patch series because it
> isn't including stddef.h. It will compile with commit
> aba708364983906ee36b1842fb1dc62dd157eefd because then locale_impl.h
> includes collate.h which includes nfd.h which includes stddef.h.
Thanks. Will fix.
> > +enum { NEG1 = -1 };
>
> This could either have a better name (e.g. L_SIGEMT) or no name at all
> like the rest of the values.
It can't be unnamed beause expanding -1 in contents where an
identifier is being built will not parse right.
> > [...]
> > char *strsignal(int signum)
> > {
> > - const char *s = strings;
> > -
> > - signum = sigmap(signum);
> > - if (signum - 1U >= _NSIG-1) signum = 0;
> > -
> > - for (; signum--; s++) for (; *s; s++);
> > + static const int unknown[] = LPATH(ERRDOMAIN_SIGNAL, 0);
> > + const int path[] = LPATH(ERRDOMAIN_SIGNAL, signum);
>
> This should still use sigmap(). Right now strsignal() is broken on
> mips.
Thanks, will fix.
>
> > [...]
> > diff --git a/src/string/strsignal.h b/src/string/strsignal.h
> > [...]
> > +#ifdef SIGSTKFLT
> > +M(16, "Stack fault")
> > +#endif
> > [...]
> > +#if _NSIG > 65
> > [...]
> > +M(127, "RT127")
> > +#endif
>
> RT128 was previously included in the multistring. But it was never
> used because 128 - 1U >= _NSIG-1 (_NSIG is 65 or 128). So you
> intentionally left it out, right?
Yes. The idea that a signal number 128 could exist was a historical
bug in the mips kernel. The bit encoding space for wait status does
not allow it to exist.
> > +#ifdef SIGEMT
> > +M(NEG1, "Emulator trap")
> > +#endif
>
> I would expect to find the message for SIGEMT next to SIGSTKFLT to
> mimick strsignal.c.
I can see potential reasons to expect either, but I'm not sure either
really is better.
> > From 4df51353e0739882b28c55c1fe4a451425539841 Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Mon, 21 Sep 2026 20:05:18 -0400
> > Subject: [PATCH 14/23] convert getopt to use new locale string table lookups
> >
> > [...]
> >
> > diff --git a/src/misc/getopt_long.c b/src/misc/getopt_long.c
> > [...]
> > #include <stdio.h>
> > #include <string.h>
> > #include "stdio_impl.h"
> > +#include "locale_impl.h"
>
> This new include is not needed.
Thanks. It was from before I split out getopt_msg.c.
>
> > [...]
> > diff --git a/src/misc/getopt_msg.c b/src/misc/getopt_msg.c
> > [...]
> > +#include <string.h>
> > +#include "stdio_impl.h"
> > +#include "locale_impl.h"
>
> Same problem like strsignal.c with stddef.h here.
Thanks for catching it.
> > [...]
> >
> > From 8ef4199d01c0df6c8622051ba4e4662de6c423d7 Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Fri, 18 Sep 2026 14:04:12 -0400
> > Subject: [PATCH 15/23] locale: remove now-unused gettext-style translation
> > code
> >
> > [...]
> >
> > diff --git a/src/internal/locale_impl.h b/src/internal/locale_impl.h
> > [...]
> > -hidden const char *__lctrans_impl(const char *, const struct __locale_map *);
> > [...]
> > diff --git a/src/locale/__lctrans.c b/src/locale/__lctrans.c
> > [...]
> > -static const char *dummy(const char *msg, const struct __locale_map *lm)
> > -{
> > - return msg;
> > -}
> > -
> > -weak_alias(dummy, __lctrans_impl);
>
> The (previously) non-dummy __lctrans_impl() in locale_map.c still
> exists (and is no longer hidden).
I think I remember seeing that too but forgot to fix it. I'll remove
it too.
> > From fe70f88faf43c60b7f94a3c919eead772a7e5ca9 Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Fri, 18 Sep 2026 11:37:06 -0400
> > Subject: [PATCH 16/23] add an iterator for unicode normalization form NFD for
> > internal use
> >
> > [...]
> >
> > diff --git a/src/internal/nfd.h b/src/internal/nfd.h
> > [...]
> > +#define decomp_iterator_start __decomp_iterator_start
> > +#define decomp_iterate __decomp_iterate
> > +#define nfd_iterator_start __nfd_iterator_start
> > +#define nfd_iterate __nfd_iterate
> > +#define nfd_iterate_ccc __nfd_iterate_ccc
>
> Same question as for ikmlt: are these defines really worthwhile?
Same motivation: being able to use the code outside of libc.
> > +void decomp_iterator_start(struct decomp_iterator *, const void *, int);
> > +uint32_t decomp_iterate(struct decomp_iterator *);
> > +
> > +void nfd_iterator_start(struct nfd_iterator *, const void *, int);
> > +uint32_t nfd_iterate_ccc(struct nfd_iterator *);
> > +wchar_t nfd_iterate(struct nfd_iterator *);
>
> Shouldn't these functions be hidden?
Yes. I just omitted adding the hidden to declaration because this code
was copied from outside libc with just the #defines added. I'll add
hidden here.
> > From aba708364983906ee36b1842fb1dc62dd157eefd Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Fri, 18 Sep 2026 12:08:29 -0400
> > Subject: [PATCH 17/23] add collation implementation
> >
> > [...]
> >
> > diff --git a/src/internal/collate.h b/src/internal/collate.h
> > [...]
> > +#define ce_iterate __ce_iterate
> > +#define ce_iterator_start __ce_iterator_start
> > +#define collation_init __collation_init
> > +
> > +hidden const unsigned char *ce_iterate(struct ce_iterator *);
> > +hidden void ce_iterator_start(struct ce_iterator *, const void *, int, const struct collation *);
> > +hidden int collation_init(struct collation *, const unsigned char *);
>
> Same question as for ikmlt: are these defines really worthwhile?
Same motivation.
> > +size_t __strxfrm_core(char *restrict, const void *restrict, size_t, int, const struct collation *);
> > +int __strcoll_core(const void *, const void *, int, const struct collation *);
>
> Shouldn't these functions be hidden?
>
> > [...]
> > diff --git a/src/locale/locale_map.c b/src/locale/locale_map.c
> > [...]
> > + collation_init(l->collate, ikmlt_lookup(root, K_COLLATION));
>
> Why does collation_init() return a value if it's unused?
We might end up using it, or not. At present libc just doesn't need
the return value because it's expected to setup a valid
no-collation-data state if it fails, and that's what we want on
failure.
> > [...]
> > diff --git a/src/locale/strxfrm.c b/src/locale/strxfrm.c
> > [...]
> > +size_t __strxfrm_core(char *restrict dest, const void *restrict src, size_t n, int wide, const struct collation *col)
> > +{
> > [...]
> > + size_t total_len = nlevels-1;
> > +
>
> This empty line has a trailing tab.
OK, will fix.
> > [...]
> > diff --git a/src/locale/wcsxfrm.c b/src/locale/wcsxfrm.c
> > [...]
> > + /* In absence of collation rules, strxfrm behaves as identity. */
>
> This is wcsxfrm(), not strxfrm().
Thanks for catching this.
> > From 304767d2576e1c285c76afb71522bd5bbdad85ed Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Thu, 17 Sep 2026 14:11:48 -0400
> > Subject: [PATCH 18/23] move global_locale object out of libc struct
> >
> > [...]
> >
> > diff --git a/src/internal/libc.h b/src/internal/libc.h
> > [...]
> > +extern hidden struct __locale_struct __global_locale;
>
> This could be declared in locale_impl.h instead. locale_impl.h would
> then no longer need to include libc.h.
Maybe a good idea, but I think it would require other files to include
locale_impl.h. Files where it would be somewhat more invasive and
unexpected. So keeping it in libc.h is probably better.
> > From c45dbe1d43d5a5aab85f529902f3f57d8149fe7a Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Fri, 18 Sep 2026 11:32:47 -0400
> > Subject: [PATCH 19/23] implement non-stub localeconv
> >
> > [...]
> >
> > setlocale and newlocale are modified to expand the actual locale data
> > into the target locale object. the mutable global locale structure is
> > pre-initialized with the C locale values, so that no code is needed to
> > prep it, and the static __c_locale and __c_dot_utf8_locale objects
> > which exist to make newlocale failure-free for these locales also hold
> > copies of this data.
>
> This is no longer the case in v2. Now the lconv pointer is (initially)
> a null pointer for these locales and localeconv() returns a pointer to
> posix_lconv for them.
OK, that commit message should be updated. Thanks!
> > diff --git a/src/internal/locale_impl.h b/src/internal/locale_impl.h
> > [...]
> > struct __locale_struct {
> > const struct __locale_map *cat[6];
> > + struct lconv *lconv;
> > };
>
> duplocale() doesn't handle the lconv pointer correctly. It just copies
> it, so the two locale objects will share the same lconv structure. If
> one of these locale objects is subsequently passed to newlocale(), the
> lconv structure will be overwritten for both.
Nice catch.
>
> duplocale() could be changed like this:
>
> static void dummy_lconv_expand(locale_t loc)
> {
> }
>
> weak_alias(dummy_lconv_expand, __lconv_expand);
>
> locale_t __duplocale(locale_t old)
> {
> locale_t new = malloc(sizeof *new + sizeof *new->lconv);
> if (!new) return 0;
> if (old == LC_GLOBAL_LOCALE) old = &__global_locale;
> *new = *old;
> new->lconv = (void *)(new + 1);
> __lconv_expand(new);
> return new;
> }
That works, but I think we can avoid calling __lconv_expand here and
avoid the need for a weak dummy for it. Just copying the contents of
the lconv struct works. Yes it contains pointers but they are all
pointers to immutable string objects.
So just: *new->lconv = *old->lconv;
>
> > [...]
> > diff --git a/src/locale/localeconv.c b/src/locale/localeconv.c
> > [...]
> > +void __lconv_expand(locale_t loc)
> > +{
> > [...]
> > + *lc = posix_lconv;
> > +
>
> This empty line has a trailing tab.
OK.
>
> > [...]
> > + if ((char)-1 > 0) {
> > + loc->lconv->grouping += strlen(loc->lconv->grouping) + 1;
> > + loc->lconv->mon_grouping += strlen(loc->lconv->mon_grouping) + 1;
> > + }
>
> This doesn't work if the value for (mon_)grouping from posix_lconv is
> used (e.g. LC_NUMERIC/LC_MONETARY is missing in loc). posix_lconv
> could be changed like this:
>
> static const struct lconv posix_lconv = {
> // ...
> .grouping = "\0",
> // ...
> .mon_grouping = "\0",
> // ...
> }
>
> To fix it for all empty strings instead:
>
> if ((char)-1 > 0) {
> size_t l = strlen(loc->lconv->grouping);
> loc->lconv->grouping += l + !!l;
> l = strlen(loc->lconv->mon_grouping);
> loc->lconv->mon_grouping += l + !!l;
> }
Hmm, I'm not sure which is better. Probably sacrificing 1 byte of
rodata is smarter than adding more code logic.
>
> > [...]
> > + return;
> > +}
>
> The return can be omitted.
Yes. I think originally it had a success/failure return and that was
removed.
> > struct lconv *localeconv(void)
> > {
> > - return (void *)&posix_lconv;
> > + /* Prior to first setlocale, or for uselocale(&default_ctype_locale),
> > + * the locale's lconv pointer may be null. In either case,
> > + * default_lconv accurately reflects the locale. */
>
> Shouldn't this comment say posix_lconv instead of default_lconv?
Yes, it was renamed and renamed back and the comment wasn't changed
back. I'll fix it.
> > [...]
> > diff --git a/src/locale/newlocale.c b/src/locale/newlocale.c
> > [...]
> > + if ((loc = malloc(sizeof *loc + sizeof *loc->lconv))) {
> > + *loc = tmp;
> > + loc->lconv = (void *)(loc + 1);
>
> Writing it this way would avoid copying a partially uninitialized tmp:
>
> tmp.lconv = (void *)(loc + 1);
> *loc = tmp;
I don't think it matters. If your concern is static analysis tooling
that fusses, it should really just be fixed not to fuss when the
uninitialized fields are unconditionally overwritten after the
assignment.
> > From 331f8c00a8dba3da34367983fb05569c7e0bc570 Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Sun, 20 Sep 2026 21:16:22 -0400
> > Subject: [PATCH 20/23] implement a working, non-stub strfmon
> >
> > [...]
> >
> > diff --git a/src/locale/strfmon.c b/src/locale/strfmon.c
> > [...]
> > static ssize_t vstrfmon_l(char *s, size_t n, locale_t loc, const char *fmt, va_list ap)
> > {
> > + locale_t old = __uselocale(loc);
> > + const struct lconv *lc = localeconv();
> > + __uselocale(old);
>
> Other functions that temporarily change the current locale (like
> iconv() or fputwc()) just use CURRENT_LOCALE instead of uselocale().
I'm fairly indifferent to consistency on things like this. Often
functions which are already written in terms of musl internals use the
internal method, while functions like strfmon that are "mostly or
entirely portable library code" use the public interfaces or
__-prefixed versions thereof.
>
> > [...]
> >
> > From 81ab5823ae00390d6a85dee0fb0a2492403451bb Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Wed, 23 Sep 2026 19:18:57 -0400
> > Subject: [PATCH 21/23] add a default locale path and switch to new environment
> > variable
> >
> > [...]
> >
> > diff --git a/src/locale/locale_map.c b/src/locale/locale_map.c
> > [...]
> > + if (!path) path = "/etc/musl/locale:/usr/share/musl/locale:/share/musl/locale";
> >
> > if (path) for (; *path; path=z+!!*z) {
>
> path will now never be a null pointer at this point.
Indeed.
>
> > z = __strchrnul(path, ':');
>
> Two things I noticed for the path search:
>
> A zero-length prefix in the sequence of path prefixes (":", ":/a",
> "/a:", "/a::/b") will lead to a lookup in the root directory. execvp()
> would search in the current working directory instead.
I think that bug was already there, so I should just fix it as a
separate change.
> The condition for detecting prefixes that are too long could be
> relaxed:
>
> if (l > sizeof buf - n - 2) continue;
>
> instead of
>
> if (l >= sizeof buf - n - 2) continue;
>
> > [...]
> >
> > From ef8494a49da47bf0a80c4de27316548a77a7dbeb Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Wed, 23 Sep 2026 19:34:03 -0400
> > Subject: [PATCH 22/23] stop locale path search on any error except
> > deterministic unavailability
> >
> > [...]
> >
> > unlike with execvp, we don't bother tracking if EACCES was seen
> > because setlocale is not specified to report a meaningful error.
>
> But newlocale() is specified to report a meaningful error.
Yes, that means I need to go back and add the eaccess_seen logic.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
