Page MenuHomeFreeBSD

Implement strerror_l().
ClosedPublic

Authored by kib on Dec 6 2020, 11:35 PM.

Details

Summary

Only for the arches that provide user-mode TLS.

PR: 251651

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Dec 6 2020, 11:35 PM

Man page update is not done yet.
Not tested.
I am not sure what __POSIX_VISIBLE guard is appropriate for the function.

include/string.h
92 ↗(On Diff #80395)

It was added in POSIX 2008 so you can add this to xlocale/_string.h where the other _l functions are declared.

kib marked an inline comment as done.

Move declaration to xlocale/_string.h

Some native English speaker should review the man page changes. Some sentences don't sound right to me but I'm no native speaker either. Everything else looks good to me.

In D27495#615210, @tijl wrote:

Some native English speaker should review the man page changes. Some sentences don't sound right to me but I'm no native speaker either. Everything else looks good to me.

Thanks for review, I will take a look at the man page

lib/libc/string/strerror.3
76 ↗(On Diff #80403)

Maybe:

.Fn strerror
is not thread-safe.
It returns a pointer to an internal buffer that could be overwritten by a
.Fn strerror
call from another thread.

(.Nm or .Fn as appropriate)

83 ↗(On Diff #80403)

Maybe

The
.Fn strerror_l
function accepts error number and locale arguments and returns a pointer to a string corresponding to the specified error in the given locale.
.Fn strerror-l
is thread-safe.
lib/libc/string/strerror.3
159 ↗(On Diff #80403)

Add .Fn strerror_l here as well?

lib/libc/string/strerror.c
132 ↗(On Diff #80403)

This assumes that this function will be used commonly, which seems reasonable.

138–139 ↗(On Diff #80403)

A good encouragement for architectures to implement proper TLS :)

However, do we want to leave this error completely undocumented?

kib marked 2 inline comments as done.

More man page work.

kib marked 3 inline comments as done.Dec 10 2020, 9:53 PM
kib added inline comments.
lib/libc/string/strerror.c
138–139 ↗(On Diff #80403)

I think arch(7) should list arches which do not implement static thread-local storage, then we can reference ENOTSUP from there.

But from what I see, there is no arches left that define NO_TLS. Still I think that maintaining NO_TLS in some form is advantageous because it easier port to the new architectires. We got 2 or 3 in recent times (depends on how to calculate, arm64, riscv, powerc elfv2).

kib marked an inline comment as done.

Even more man page updates.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 16 2020, 9:03 AM
Closed by commit rS368692: Implement strerror_l(). (authored by kib). · Explain Why
This revision was automatically updated to reflect the committed changes.

this change makes my build world (in poudriere) fail

head/lib/libc/nls/msgcat.c
130

when running a build world in a poudriere jail,this change is failing on both amd64 and aarch64 with

--- msgcat.o ---
cc -target x86_64-unknown-freebsd13.0 --sysroot=/usr/obj/poudriere/jails/pkgbase130-current-amd64/usr/src/amd64.amd64/tmp -B/usr/obj/poudriere/jails/pkgbase130-current-amd64/usr/src/amd64.amd64/tmp/usr/bin  -O2 -pipe -fno-common   -DNO__SCCSID -DNO__RCSID -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/include -I/poudriere/jails/pkgbase130-current-amd64/usr/src/include -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/amd64 -DNLS  -D__DBINTERFACE_PRIVATE -I/poudriere/jails/pkgbase130-current-amd64/usr/src/contrib/gdtoa -I/poudriere/jails/pkgbase130-current-amd64/usr/src/contrib/libc-vis -DINET6 -I/usr/obj/poudriere/jails/pkgbase130-current-amd64/usr/src/amd64.amd64/lib/libc -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/resolv -D_ACL_PRIVATE -DPOSIX_MISTAKE -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libmd -I/poudriere/jails/pkgbase130-current-amd64/usr/src/contrib/jemalloc/include -I/poudriere/jails/pkgbase130-current-amd64/usr/src/contrib/tzcode/stdtime -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/stdtime -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/locale -DBROKEN_DES -DPORTMAP -DDES_BUILTIN -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/rpc -DWANT_HYPERV -DYP -DNS_CACHING -DSYMBOL_VERSIONING -g -MD  -MF.depend.msgcat.o -MTmsgcat.o -std=gnu99 -Wno-format-zero-length -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -Wno-uninitialized -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-address-of-packed-member -Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter  -Qunused-arguments    -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libutil -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/msun/amd64 -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/msun/x86 -I/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/msun/src -c /poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/nls/msgcat.c -o msgcat.o
/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/nls/msgcat.c:126:10: error: implicit declaration of function '__catopen_l' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        return (__catopen_l(name, type, __get_locale()));
                ^
/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/nls/msgcat.c:126:9: error: incompatible integer to pointer conversion returning 'int' from a function with result type 'nl_catd' (aka 'struct __nl_cat_d *') [-Werror,-Wint-conversion]
        return (__catopen_l(name, type, __get_locale()));
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/nls/msgcat.c:130:1: error: conflicting types for '__catopen_l'
__catopen_l(const char *name, int type, locale_t locale)
^
/poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc/nls/msgcat.c:126:10: note: previous implicit declaration is here
        return (__catopen_l(name, type, __get_locale()));
                ^
3 errors generated.
*** [msgcat.o] Error code 1

make[4]: stopped in /poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc
1 error

make[4]: stopped in /poudriere/jails/pkgbase130-current-amd64/usr/src/lib/libc
[00:11:46] Error: Failed to 'make buildworld'
[00:11:46] Error while creating jail, cleaning up.
[00:11:46] Removing pkgbase130-current-amd64 jail... done
[00:11:48] Cleaning pkgbase130-current-amd64 data... done
head/lib/libc/nls/msgcat.c
130

As you can see in this review, the prototype for __catopen_l() was added to libc_private.h.

I have no idea what is broken in your env.

head/lib/libc/nls/msgcat.c
130

it seems that the system i was trying to build this is missing the follow up fix https://lists.freebsd.org/pipermail/svn-src-head/2020-December/142563.html

head/lib/libc/nls/msgcat.c
130

Referenced change has nothing to do with __catopen_l(). And we do not restrict visible namespace for libc build.

head/lib/libc/nls/msgcat.c
130

🤷🏻‍♀️ i dunno what to tell you 🤷🏻‍♀️ after manually patching my /usr/include/string.h with https://lists.freebsd.org/pipermail/svn-src-head/2020-December/142563.html it started working

i don't know if poudriere does anything funky here, but this is my observation.

either way i now have a fresh build @ https://alpha.pkgbase.live/FreeBSD:13:amd64/