| Message ID | 20230108214357.12641-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart via libcamera-devel (2023-01-08 21:43:57) > Not all C libraries include support for locale objects (locale_t) and > the strto*_l() family of functions. A notable example is uClibc that can > be compiled with a hardcoded "C" locale. Compilation then fails as the > newlocale(), freelocale() and strtod_l() functions are not defined. > > Fix the compilation breakage by checking for the availability of > newlocale(), and fall back to strtod() when the function isn't > available. This may not lead to the correct result if support for locale > objects isn't available and the locale isn't hardcoded to "C", but that > is such a corner case that we will likely never encounter it. Agreed. And this is fine by me, as long as it all compiles. I'm not yet aware of anyone actually using libcamera with uClibc (I'd love to hear if anyone is using it ...) - so I think this is just to maintain compatibility for now. Does the same issue happen in musl - or does that have locale support ? Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Fixes: e8ae254970cf ("libcamera: yaml_parser: Use C locale") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > meson.build | 4 ++++ > src/libcamera/base/utils.cpp | 12 ++++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/meson.build b/meson.build > index e86673dd5c0c..7dcd34d3e6b3 100644 > --- a/meson.build > +++ b/meson.build > @@ -72,6 +72,10 @@ if cc.has_header_symbol('unistd.h', 'issetugid') > config_h.set('HAVE_ISSETUGID', 1) > endif > > +if cc.has_header_symbol('locale.h', 'newlocale', prefix : '#define _GNU_SOURCE') > + config_h.set('HAVE_NEWLOCALE', 1) > +endif > + > if cc.has_header_symbol('stdlib.h', 'secure_getenv', prefix : '#define _GNU_SOURCE') > config_h.set('HAVE_SECURE_GETENV', 1) > endif > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > index 4a239427a4d9..ac3e1311995e 100644 > --- a/src/libcamera/base/utils.cpp > +++ b/src/libcamera/base/utils.cpp > @@ -464,6 +464,8 @@ std::string toAscii(const std::string &str) > * \a b > */ > > +#if HAVE_NEWLOCALE > + > namespace { > > /* > @@ -493,6 +495,8 @@ Locale cLocale("C"); > > } /* namespace */ > > +#endif /* HAVE_NEWLOCALE */ > + > /** > * \brief Convert a string to a double independently of the current locale > * \param[in] nptr The string to convert > @@ -506,7 +510,15 @@ Locale cLocale("C"); > */ > double strtod(const char *__restrict nptr, char **__restrict endptr) > { > +#if HAVE_NEWLOCALE > return strtod_l(nptr, endptr, cLocale.locale()); > +#else > + /* > + * If the libc implementation doesn't provide locale object support, > + * assume that strtod() is locale-independent. > + */ > + return strtod(nptr, endptr); > +#endif > } > > } /* namespace utils */ > -- > Regards, > > Laurent Pinchart >
On Sun, Jan 08, 2023 at 11:22:37PM +0000, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2023-01-08 21:43:57) > > Not all C libraries include support for locale objects (locale_t) and > > the strto*_l() family of functions. A notable example is uClibc that can > > be compiled with a hardcoded "C" locale. Compilation then fails as the > > newlocale(), freelocale() and strtod_l() functions are not defined. > > > > Fix the compilation breakage by checking for the availability of > > newlocale(), and fall back to strtod() when the function isn't > > available. This may not lead to the correct result if support for locale > > objects isn't available and the locale isn't hardcoded to "C", but that > > is such a corner case that we will likely never encounter it. > > Agreed. > > And this is fine by me, as long as it all compiles. I'm not yet aware of > anyone actually using libcamera with uClibc (I'd love to hear if anyone > is using it ...) - so I think this is just to maintain compatibility for > now. > > Does the same issue happen in musl - or does that have locale support ? No, musl has locale object support. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Fixes: e8ae254970cf ("libcamera: yaml_parser: Use C locale") > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > meson.build | 4 ++++ > > src/libcamera/base/utils.cpp | 12 ++++++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/meson.build b/meson.build > > index e86673dd5c0c..7dcd34d3e6b3 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -72,6 +72,10 @@ if cc.has_header_symbol('unistd.h', 'issetugid') > > config_h.set('HAVE_ISSETUGID', 1) > > endif > > > > +if cc.has_header_symbol('locale.h', 'newlocale', prefix : '#define _GNU_SOURCE') > > + config_h.set('HAVE_NEWLOCALE', 1) > > +endif > > + > > if cc.has_header_symbol('stdlib.h', 'secure_getenv', prefix : '#define _GNU_SOURCE') > > config_h.set('HAVE_SECURE_GETENV', 1) > > endif > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > > index 4a239427a4d9..ac3e1311995e 100644 > > --- a/src/libcamera/base/utils.cpp > > +++ b/src/libcamera/base/utils.cpp > > @@ -464,6 +464,8 @@ std::string toAscii(const std::string &str) > > * \a b > > */ > > > > +#if HAVE_NEWLOCALE > > + > > namespace { > > > > /* > > @@ -493,6 +495,8 @@ Locale cLocale("C"); > > > > } /* namespace */ > > > > +#endif /* HAVE_NEWLOCALE */ > > + > > /** > > * \brief Convert a string to a double independently of the current locale > > * \param[in] nptr The string to convert > > @@ -506,7 +510,15 @@ Locale cLocale("C"); > > */ > > double strtod(const char *__restrict nptr, char **__restrict endptr) > > { > > +#if HAVE_NEWLOCALE > > return strtod_l(nptr, endptr, cLocale.locale()); > > +#else > > + /* > > + * If the libc implementation doesn't provide locale object support, > > + * assume that strtod() is locale-independent. > > + */ > > + return strtod(nptr, endptr); > > +#endif > > } > > > > } /* namespace utils */
Hi Laurent On Sun, Jan 08, 2023 at 11:43:57PM +0200, Laurent Pinchart via libcamera-devel wrote: > Not all C libraries include support for locale objects (locale_t) and > the strto*_l() family of functions. A notable example is uClibc that can > be compiled with a hardcoded "C" locale. Compilation then fails as the > newlocale(), freelocale() and strtod_l() functions are not defined. > > Fix the compilation breakage by checking for the availability of > newlocale(), and fall back to strtod() when the function isn't > available. This may not lead to the correct result if support for locale > objects isn't available and the locale isn't hardcoded to "C", but that > is such a corner case that we will likely never encounter it. > > Fixes: e8ae254970cf ("libcamera: yaml_parser: Use C locale") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > meson.build | 4 ++++ > src/libcamera/base/utils.cpp | 12 ++++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/meson.build b/meson.build > index e86673dd5c0c..7dcd34d3e6b3 100644 > --- a/meson.build > +++ b/meson.build > @@ -72,6 +72,10 @@ if cc.has_header_symbol('unistd.h', 'issetugid') > config_h.set('HAVE_ISSETUGID', 1) > endif > > +if cc.has_header_symbol('locale.h', 'newlocale', prefix : '#define _GNU_SOURCE') > + config_h.set('HAVE_NEWLOCALE', 1) > +endif > + > if cc.has_header_symbol('stdlib.h', 'secure_getenv', prefix : '#define _GNU_SOURCE') > config_h.set('HAVE_SECURE_GETENV', 1) > endif > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > index 4a239427a4d9..ac3e1311995e 100644 > --- a/src/libcamera/base/utils.cpp > +++ b/src/libcamera/base/utils.cpp > @@ -464,6 +464,8 @@ std::string toAscii(const std::string &str) > * \a b > */ > > +#if HAVE_NEWLOCALE > + Just bikeshedding on the NEWLOCALE symbol. "New" to implies there's an old vs new version, while you're just here checking that locale is supported. Maybe just HAVE_LOCALE ? Apart from this: Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > namespace { > > /* > @@ -493,6 +495,8 @@ Locale cLocale("C"); > > } /* namespace */ > > +#endif /* HAVE_NEWLOCALE */ > + > /** > * \brief Convert a string to a double independently of the current locale > * \param[in] nptr The string to convert > @@ -506,7 +510,15 @@ Locale cLocale("C"); > */ > double strtod(const char *__restrict nptr, char **__restrict endptr) > { > +#if HAVE_NEWLOCALE > return strtod_l(nptr, endptr, cLocale.locale()); > +#else > + /* > + * If the libc implementation doesn't provide locale object support, > + * assume that strtod() is locale-independent. > + */ > + return strtod(nptr, endptr); > +#endif > } > > } /* namespace utils */ > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Tue, Jan 10, 2023 at 09:13:53AM +0100, Jacopo Mondi wrote: > On Sun, Jan 08, 2023 at 11:43:57PM +0200, Laurent Pinchart via libcamera-devel wrote: > > Not all C libraries include support for locale objects (locale_t) and > > the strto*_l() family of functions. A notable example is uClibc that can > > be compiled with a hardcoded "C" locale. Compilation then fails as the > > newlocale(), freelocale() and strtod_l() functions are not defined. > > > > Fix the compilation breakage by checking for the availability of > > newlocale(), and fall back to strtod() when the function isn't > > available. This may not lead to the correct result if support for locale > > objects isn't available and the locale isn't hardcoded to "C", but that > > is such a corner case that we will likely never encounter it. > > > > Fixes: e8ae254970cf ("libcamera: yaml_parser: Use C locale") > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > meson.build | 4 ++++ > > src/libcamera/base/utils.cpp | 12 ++++++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/meson.build b/meson.build > > index e86673dd5c0c..7dcd34d3e6b3 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -72,6 +72,10 @@ if cc.has_header_symbol('unistd.h', 'issetugid') > > config_h.set('HAVE_ISSETUGID', 1) > > endif > > > > +if cc.has_header_symbol('locale.h', 'newlocale', prefix : '#define _GNU_SOURCE') > > + config_h.set('HAVE_NEWLOCALE', 1) > > +endif > > + > > if cc.has_header_symbol('stdlib.h', 'secure_getenv', prefix : '#define _GNU_SOURCE') > > config_h.set('HAVE_SECURE_GETENV', 1) > > endif > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > > index 4a239427a4d9..ac3e1311995e 100644 > > --- a/src/libcamera/base/utils.cpp > > +++ b/src/libcamera/base/utils.cpp > > @@ -464,6 +464,8 @@ std::string toAscii(const std::string &str) > > * \a b > > */ > > > > +#if HAVE_NEWLOCALE > > + > > Just bikeshedding on the NEWLOCALE symbol. "New" to implies there's an > old vs new version, while you're just here checking that locale is > supported. Maybe just HAVE_LOCALE ? It comes directly from the name of the function being checked in meson.build, newlocale(). I could replace that with a check for the locale_t type and use HAVE_LOCALE_T if preferred. > Apart from this: > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > namespace { > > > > /* > > @@ -493,6 +495,8 @@ Locale cLocale("C"); > > > > } /* namespace */ > > > > +#endif /* HAVE_NEWLOCALE */ > > + > > /** > > * \brief Convert a string to a double independently of the current locale > > * \param[in] nptr The string to convert > > @@ -506,7 +510,15 @@ Locale cLocale("C"); > > */ > > double strtod(const char *__restrict nptr, char **__restrict endptr) > > { > > +#if HAVE_NEWLOCALE > > return strtod_l(nptr, endptr, cLocale.locale()); > > +#else > > + /* > > + * If the libc implementation doesn't provide locale object support, > > + * assume that strtod() is locale-independent. > > + */ > > + return strtod(nptr, endptr); > > +#endif > > } > > > > } /* namespace utils */
diff --git a/meson.build b/meson.build index e86673dd5c0c..7dcd34d3e6b3 100644 --- a/meson.build +++ b/meson.build @@ -72,6 +72,10 @@ if cc.has_header_symbol('unistd.h', 'issetugid') config_h.set('HAVE_ISSETUGID', 1) endif +if cc.has_header_symbol('locale.h', 'newlocale', prefix : '#define _GNU_SOURCE') + config_h.set('HAVE_NEWLOCALE', 1) +endif + if cc.has_header_symbol('stdlib.h', 'secure_getenv', prefix : '#define _GNU_SOURCE') config_h.set('HAVE_SECURE_GETENV', 1) endif diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp index 4a239427a4d9..ac3e1311995e 100644 --- a/src/libcamera/base/utils.cpp +++ b/src/libcamera/base/utils.cpp @@ -464,6 +464,8 @@ std::string toAscii(const std::string &str) * \a b */ +#if HAVE_NEWLOCALE + namespace { /* @@ -493,6 +495,8 @@ Locale cLocale("C"); } /* namespace */ +#endif /* HAVE_NEWLOCALE */ + /** * \brief Convert a string to a double independently of the current locale * \param[in] nptr The string to convert @@ -506,7 +510,15 @@ Locale cLocale("C"); */ double strtod(const char *__restrict nptr, char **__restrict endptr) { +#if HAVE_NEWLOCALE return strtod_l(nptr, endptr, cLocale.locale()); +#else + /* + * If the libc implementation doesn't provide locale object support, + * assume that strtod() is locale-independent. + */ + return strtod(nptr, endptr); +#endif } } /* namespace utils */
Not all C libraries include support for locale objects (locale_t) and the strto*_l() family of functions. A notable example is uClibc that can be compiled with a hardcoded "C" locale. Compilation then fails as the newlocale(), freelocale() and strtod_l() functions are not defined. Fix the compilation breakage by checking for the availability of newlocale(), and fall back to strtod() when the function isn't available. This may not lead to the correct result if support for locale objects isn't available and the locale isn't hardcoded to "C", but that is such a corner case that we will likely never encounter it. Fixes: e8ae254970cf ("libcamera: yaml_parser: Use C locale") Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- meson.build | 4 ++++ src/libcamera/base/utils.cpp | 12 ++++++++++++ 2 files changed, 16 insertions(+)