[PATCH/RFC] Keep exported _nl_default_dirname constant when prefix changes

[Jonathan Nieder]

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.

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

Thoughts?

 ChangeLog          | 19 +++++++++++++++++++
 intl/Versions      |  9 ++++++++-
 intl/bindtextdom.c | 25 +++++++++++--------------
 intl/dcigettext.c  | 25 +++++++++++++++++--------
 4 files changed, 55 insertions(+), 23 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4c9b2cac..b5c8bc40 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,22 @@
[phone]  Jonathan Nieder  <jrnieder@...>
+
+ [BZ #14664]
+ 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@...>
 
  [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
@@ -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
@@ -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.  */
 #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);
--
1.8.1.rc0

[Andreas Schwab]

Jonathan Nieder <jrnieder@...> writes:

> 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

Do you need this?  AFAIU if the symbol is not defined (ie. min-ABI is
2.17) it is just ignored.

Andreas.

--
Andreas Schwab, SUSE Labs, schwab@...
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[Jonathan Nieder]

Andreas Schwab wrote:
> Jonathan Nieder <jrnieder@...> writes:
 

>> 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
>
> Do you need this?  AFAIU if the symbol is not defined (ie. min-ABI is
> 2.17) it is just ignored.

I think you're right.  The only Versions files (hurd/Versions and
mach/Versions) that do this kind of thing also have negated blocks
("%if !SHLIB_COMPAT (...)") so presumably they're doing something more
complicated.

Thanks.  The next version will leave intl/Versions alone.

[Carlos O'Donell-2]

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 @@
> [phone]  Jonathan Nieder  <jrnieder@...>
> +
> + [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@...>
>
>   [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.