[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <at-CWLzJQMThF1sh@mail.gmail.com>
Date: Mon, 26 Oct 2026 17:39:52 +0100
From: Luca Kellermann <mailto.luca.kellermann@...il.com>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH v2] musl locale overhaul series, take 2
On Sat, Oct 24, 2026 at 05:52:14PM -0400, Rich Felker wrote:
> 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:
> [...]
> > > - 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?
Yes, only in the email.
> [...]
> > > 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.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.
Ah, I didn't think about that. Makes sense then.
> [...]
> > > 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.
The point I wanted to get accross is that the numbers also vary for
powerpc (in the sense that it has one additional error). So from that
point of view, the commit message is not 100% accurate. I agree that
no remapping is needed though.
> [...]
> > > 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.
Just slight wording and it's ok if you think no change is needed. The
part of the comment I'm referring to is this:
"This file is included multiple times to declare and define a
structure with these messages, and then to define a lookup table"
Sounds like it's included 3 times to (1) declare the message
structure, (2) define the message structure and (3) define the lookup
table. This was accurate before.
With mdecl.h/mdata.h it's included 6 times to (1) declare the size of
the lookup table, (2) declare the message structure, (3,4) initialize
the table header with the size of the lookup table, (5) initialize the
lookup table and (6) initialize the message structure.
So it's no longer first message structure and then lookup table but
both intertwined.
> > > [...]
> > > 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.
Makes sense. However, errno(3) says: "On Linux, these two have the
same value on all architectures."
But idk if that's accurate.
> > 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.
Yes, seems like the right fix if you want to keep L_EWOULDBLOCK.
> [...]
> > 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.
Ok.
> > 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
Yes, that's the right condition if L_EDEADLOCK is always defined.
> > 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.
To avoid forgetting this in the future, should __strerror.h be
prepared?
#if EWOULDBLOCK != EAGAIN
// E(EWOULDBLOCK, "")
#endif
#if EDEADLOCK != EDEADLK
// E(EDEADLOCK, "") // if you don't want to add it now
#endif
> [...]
> > > +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.
Ok.
> > 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).
Sure, I didn't think about it that way.
Keeping other negative L_ values unused might be useful in the future
though. Think of supporting things like ENOTSUP != EOPNOTSUPP if
that's ever needed.
> [...]
> > > 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.
With "implemented like this" I meant the following code, not what was
commented out.
> 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.
That's exactly what the code below does.
> > 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?
I suggested this because it would make it impossible to have
mismatched values for RADIXCHAR/decimal_point or THOUSEP/thousands_sep
(e.g. due to translator mistake).
But the tooling could also ensure this. Only allow one of each
RADIXCHAR/decimal_point and THOUSEP/thousands_sep in the source format
and use it for both in the generated file.
> > 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.
Better do this in the tooling then if deemed useful.
> > > /* _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.
A fix for both the implementation-defined shift behavior and the out
of bounds access:
char *__nl_langinfo_l(nl_item item, locale_t loc)
{
int cat = ((unsigned)item) >> 16;
if (cat >= LC_ALL) return "";
// ...
if (idx == 65535)
return loc->cat[cat] ? (char *)loc->cat[cat]->name : "C";
// ...
}
> [...]
> > > 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.
The problem is in setlocale.c, not locale_map.c:
char part[LOCALE_NAME_MAX+1] = "C.UTF-8";
const char *p = name;
for (i=0; i<LC_ALL; i++) {
const char *z = __strchrnul(p, ';');
if (z-p <= LOCALE_NAME_MAX) {
memcpy(part, p, z-p);
part[z-p] = 0;
if (*z) p = z+1;
}
// for long names, part is still "C.UTF-8" at this point
lm = __get_locale(i, part);
// ...
}
> > 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.
This is the fix for the problem described above.
> [...]
> > > 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.
On second thought, the messages for getopt() are also errors. So the
name is fine.
> [...]
> > > +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?
What do you think about this? As in:
const void *map = __map_file(buf, &map_size);
// ...
if (map_size <= 24) goto unmap; // skip load_locale()
Or is it always valid to access these first few bytes?
> [...]
> > > + 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.
git am made me aware of this, it printed some warnings about
whitespace.
> > > [...]
> > > 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?
Hm, better don't continue then.
> > > - 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?
I just wasn't sure if copying uninitialized memory like this is some
sort of UB, especially if pointers are involved.
> > > 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
> [...]
> > > +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.
Ah, yes. The name is fine then.
> [...]
> > > 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/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.
Also makes sense when using collate.c outside of musl.
> [...]
> > > 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.
Yes, just __init_tls.c and pthread_create.c though. These already
change with this commit.
> > > 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
> [...]
> > 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;
old->lconv can be a null pointer if old is &__global_locale or
&default_ctype_locale.
lconv would also be uninitialized if localeconv() isn't linked in, if
that's a problem.
> [...]
> > > 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.
Same concern about potential UB.
> > > 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.
Using __uselocale() is probably better then. Should
src/include/locale.h be added to declare __uselocale()?
> [...]
> > > 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
> [...]
> > > 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.
Yes, it's not new.
> [...]
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.
