This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH/RFC] Keep exported _nl_default_dirname constant when prefixchanges


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]