[PATCH/RFC] Keep exported _nl_default_dirname constant when prefix changes
Carlos O'Donell
carlos@systemhalted.org
Thu Dec 20 20:01:00 GMT 2012
On 12/11/2012 06:12 AM, Jonathan Nieder wrote:
> Passing --prefix with a value of length != 4 breaks
> "make check-abi-libc":
>
> | --- sysdeps/unix/sysv/linux/x86_64/64/nptl/libc.abilist 2012-12-09 17:22:03.917834726 -0800
> | +++ /home/jrn/src/glibc/BUILD/libc.symlist 2012-12-11 02:11:08.487665939 -0800
> | @@ -516 +516 @@ GLIBC_2.2.5
> | - _nl_default_dirname D 0x12
> | + _nl_default_dirname D 0x21
>
> That 0x12 is sizeof("/usr/share/locale"), including terminating NUL.
> Changing the size is is a real ABI break: the size is recorded in a
> COPY relocation in applications unfortunate enough to use the
> _nl_default_dirname symbol. The dynamic linker will print a warning
> and truncate the value when trying to run such an application if the
> length of the installation prefix is shorter in the version of libc
> used at build time than at run time.
>
> Luckily this symbol is only pedantically part of the ABI and is not
> actually useful. It's not advertised in headers and its inclusion in
> the public ABI was a historical mistake. ABI compatibility can be
> maintained by exporting the value _nl_default_dirname =
> "/usr/share/locale" regardless of the configured prefix.
>
> Meanwhile the internal variable representing the default localedir
> should continue to vary with $prefix, so rename it for clarity.
>
> While at it, this patch moves _nl_default_dirname to the .text.compat
> section, guards its definition with SHLIB_COMPAT (GLIBC_2_0,
> GLIBC_2_17), and sets the "hidden" bit (bit 15) on the symbol version.
>
> Addresses bug 14664.
Excellent description.
> Improved by David Miller, Andreas Schwab, Roland McGrath, and Andi
> Kleen.
> ---
> compat_symbol works very nicely for this --- thanks again for your
> help. Result of testing:
>
> LIBRARY_PATH=$HOME/opt/glibc/lib gcc -o test test.c
> /tmp/ccDrZppl.o: In function `main':
> test.c:(.text+0x5): undefined reference to `_nl_default_dirname'
> collect2: error: ld returned 1 exit status
Just to be pedantic could you also test an *old* application run with
the new glibc?
Technically we should have a regression test that tries to use
_nl_default_dirname whose failure indicates a PASS. How hard would it be
to add something like that? Such a test would be useful for distros
backporting patches and would catch any snafus.
> Thoughts?
>
> ChangeLog | 19 +++++++++++++++++++
> intl/Versions | 9 ++++++++-
> intl/bindtextdom.c | 25 +++++++++++--------------
> intl/dcigettext.c | 25 +++++++++++++++++--------
> 4 files changed, 55 insertions(+), 23 deletions(-)
Looks good modulo Andreas' comments about intl/Versions.
> diff --git a/ChangeLog b/ChangeLog
> index 4c9b2cac..b5c8bc40 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,22 @@
> +2012-12-11 Jonathan Nieder <jrnieder@gmail.com>
> +
> + [BZ #14664]
This goes in the git commit. Please remove.
~~~
> + Rename the internal variable representing the default location of
> + message catalogs and stop exporting it. To maintain ABI compatibility,
> + define _nl_default_dirname to its fixed-size canonical value
> + "/usr/share/locale" for use at runtime.
~~~
> +
> + * intl/Versions: Include <shlib-compat.h> and define SHARED.
> + (libc: GLIBC_2.0): Conditionalize _nl_default_dirname on
> + [SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_17)].
> + * intl/bindtextdom.c (_nl_default_dirname): Rename to
> + __nl_default_dirname. Declare with attribute_hidden instead of
> + libc_hidden_proto.
> + * intl/dcigettext.c (_nl_default_dirname): Likewise.
> + [SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_17)] (_nl_old_default_dirname):
> + New array in .data.compat section with value "/usr/share/locale". Add
> + compatibility symbol _nl_default_dirname for GLIBC_2.0.
> +
> 2012-12-11 Siddhesh Poyarekar <siddhesh@redhat.com>
>
> [BZ #14246]
> diff --git a/intl/Versions b/intl/Versions
> index d76982db..4491e595 100644
> --- a/intl/Versions
> +++ b/intl/Versions
> @@ -1,7 +1,14 @@
> +%define SHARED
> +%include <shlib-compat.h>
> +
> libc {
> GLIBC_2.0 {
> # global variables
> - _nl_msg_cat_cntr; _nl_default_dirname; _nl_domain_bindings;
> + _nl_msg_cat_cntr;
> +%if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_17)
> + _nl_default_dirname;
> +%endif
> + _nl_domain_bindings;
>
> # functions used in inline functions or macros
> __dcgettext;
> diff --git a/intl/bindtextdom.c b/intl/bindtextdom.c
> index 98a3606d..0ae6124b 100644
> --- a/intl/bindtextdom.c
> +++ b/intl/bindtextdom.c
Update and merge copyright years.
> @@ -45,7 +45,7 @@
> names than the internal variables in GNU libc, otherwise programs
> using libintl.a cannot be linked statically. */
> #if !defined _LIBC
> -# define _nl_default_dirname libintl_nl_default_dirname
> +# define __nl_default_dirname libintl_nl_default_dirname
> # define _nl_domain_bindings libintl_nl_domain_bindings
> #endif
>
> @@ -57,10 +57,7 @@
> /* @@ end of prolog @@ */
>
> /* Contains the default location of the message catalogs. */
> -extern const char _nl_default_dirname[];
> -#ifdef _LIBC
> -libc_hidden_proto (_nl_default_dirname)
> -#endif
> +extern const char __nl_default_dirname[] attribute_hidden;
>
> /* List with bindings of specific domains. */
> extern struct binding *_nl_domain_bindings;
> @@ -149,8 +146,8 @@ set_binding_values (domainname, dirnamep, codesetp)
> char *result = binding->dirname;
> if (strcmp (dirname, result) != 0)
> {
> - if (strcmp (dirname, _nl_default_dirname) == 0)
> - result = (char *) _nl_default_dirname;
> + if (strcmp (dirname, __nl_default_dirname) == 0)
> + result = (char *) __nl_default_dirname;
> else
> {
> #if defined _LIBC || defined HAVE_STRDUP
> @@ -165,7 +162,7 @@ set_binding_values (domainname, dirnamep, codesetp)
>
> if (__builtin_expect (result != NULL, 1))
> {
> - if (binding->dirname != _nl_default_dirname)
> + if (binding->dirname != __nl_default_dirname)
> free (binding->dirname);
>
> binding->dirname = result;
> @@ -217,7 +214,7 @@ set_binding_values (domainname, dirnamep, codesetp)
> {
> /* Simply return the default values. */
> if (dirnamep)
> - *dirnamep = _nl_default_dirname;
> + *dirnamep = __nl_default_dirname;
> if (codesetp)
> *codesetp = NULL;
> }
> @@ -239,11 +236,11 @@ set_binding_values (domainname, dirnamep, codesetp)
>
> if (dirname == NULL)
> /* The default value. */
> - dirname = _nl_default_dirname;
> + dirname = __nl_default_dirname;
> else
> {
> - if (strcmp (dirname, _nl_default_dirname) == 0)
> - dirname = _nl_default_dirname;
> + if (strcmp (dirname, __nl_default_dirname) == 0)
> + dirname = __nl_default_dirname;
> else
> {
> char *result;
> @@ -266,7 +263,7 @@ set_binding_values (domainname, dirnamep, codesetp)
> }
> else
> /* The default value. */
> - new_binding->dirname = (char *) _nl_default_dirname;
> + new_binding->dirname = (char *) __nl_default_dirname;
>
> if (codesetp)
> {
> @@ -319,7 +316,7 @@ set_binding_values (domainname, dirnamep, codesetp)
> if (0)
> {
> failed_codeset:
> - if (new_binding->dirname != _nl_default_dirname)
> + if (new_binding->dirname != __nl_default_dirname)
> free (new_binding->dirname);
> failed_dirname:
> free (new_binding);
> diff --git a/intl/dcigettext.c b/intl/dcigettext.c
> index 088fdcbd..02cac215 100644
> --- a/intl/dcigettext.c
> +++ b/intl/dcigettext.c
Update and merge copyright years.
> @@ -105,7 +105,7 @@ extern int errno;
> #if !defined _LIBC
> # define _nl_default_default_domain libintl_nl_default_default_domain
> # define _nl_current_default_domain libintl_nl_current_default_domain
> -# define _nl_default_dirname libintl_nl_default_dirname
> +# define __nl_default_dirname libintl_nl_default_dirname
> # define _nl_domain_bindings libintl_nl_domain_bindings
> #endif
>
> @@ -267,14 +267,23 @@ const char *_nl_current_default_domain attribute_hidden
> = _nl_default_default_domain;
>
> /* Contains the default location of the message catalogs. */
> +const char __nl_default_dirname[] attribute_hidden = LOCALEDIR;
>
> +/* Historically, _nl_default_dirname was exported (though not advertised
> + in any headers). Hopefully no one uses it. Unfortunately anyone who
> + did use it has a COPY relocation that hard-codes the size.
> +
> + The real _nl_default_dirname is now named __nl_default_dirname and is
> + private. We export a separate _nl_default_dirname for compatibility
> + with a value that makes sense for prefix=/usr, which is the best one
> + could hope for given the ABI constraint. */
Good comment.
> #ifdef _LIBC
> -extern const char _nl_default_dirname[];
> -libc_hidden_proto (_nl_default_dirname)
> +#include <shlib-compat.h>
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_17)
> +const char attribute_compat_data_section _nl_old_default_dirname[]
> + = "/usr/share/locale";
> +compat_symbol (libc, _nl_old_default_dirname, _nl_default_dirname, GLIBC_2_0);
> #endif
> -const char _nl_default_dirname[] = LOCALEDIR;
> -#ifdef _LIBC
> -libc_hidden_data_def (_nl_default_dirname)
> #endif
>
> /* List with bindings of specific domains created by bindtextdomain()
> @@ -524,7 +533,7 @@ DCIGETTEXT (domainname, msgid1, msgid2, plural, n, category)
> }
>
> if (binding == NULL)
> - dirname = (char *) _nl_default_dirname;
> + dirname = (char *) __nl_default_dirname;
> else if (binding->dirname[0] == '/')
> dirname = binding->dirname;
> else
> @@ -1452,7 +1461,7 @@ libc_freeres_fn (free_mem)
> {
> struct binding *oldp = _nl_domain_bindings;
> _nl_domain_bindings = _nl_domain_bindings->next;
> - if (oldp->dirname != _nl_default_dirname)
> + if (oldp->dirname != __nl_default_dirname)
> /* Yes, this is a pointer comparison. */
> free (oldp->dirname);
> free (oldp->codeset);
>
Please repost with ChangeLog, copyright years, and Version issues updated.
Please comment on feasibility of regression test case.
Cheers,
Carlos.
More information about the Libc-alpha
mailing list