[PATCH] locale: XFAIL newlocale usage in static binary (Bug 23164)

[PATCH] locale: XFAIL newlocale usage in static binary (Bug 23164)

[Carlos O'Donell]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

There is a glibc optimization which allows for locale categories
to be removed during static compilation. There have been various
bugs for this support over the years, with bug 16915 being the
most recent. The solution there was to emit a reference to all the
categories to avoid any being removed. This fix, although it's in
the generic __nl_langinfo_l function, doesn't appear to be enough
to fix the case for a statically linked program that uses newlocale
and nl_langinfo_l. This commit doesn't fix the problem, but it does
add a XFAIL'd test case such that a fix can be applied against this
and the XFAIL removed. It's not entirely clear that the problem is
the same as that which was seen in bug 16915.

The commit makes tst-langinfo.c into a test driver for use by two
new tests which use setenv/nl_langinfo or newlocale/nl_langinfo_l,
both dynamic (which pass) and static (for which tst-langinfo-newlocale-static
fails and is XFAIL'd). In addition we add CURRENCY_SYMBOL test coverage
which was the original problem reported in the related gcc/C++ PR.

OK to commit?

-- 
Cheers,
Carlos.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-locale-XFAIL-newlocale-usage-in-static-binary-Bug-23.patch --]
[-- Type: text/x-patch; name="0001-locale-XFAIL-newlocale-usage-in-static-binary-Bug-23.patch", Size: 13587 bytes --]

From 01e74bd1aa78263669ce66f0568d6573fb78a15a Mon Sep 17 00:00:00 2001
From: Carlos O'Donell <carlos@redhat.com>
Date: Mon, 14 May 2018 08:25:46 -0400
Subject: [PATCH] locale: XFAIL newlocale usage in static binary (Bug 23164)

There is a glibc optimization which allows for locale categories
to be removed during static compilation. There have been various
bugs for this support over the years, with bug 16915 being the
most recent. The solution there was to emit a reference to all the
categories to avoid any being removed. This fix, although it's in
the generic __nl_langinfo_l function, doesn't appear to be enough
to fix the case for a statically linked program that uses newlocale
and nl_langinfo_l. This commit doesn't fix the problem, but it does
add a XFAIL'd test case such that a fix can be applied against this
and the XFAIL removed. It's not entirely clear that the problem is
the same as that which was seen in bug 16915.
---
 ChangeLog                                  | 29 ++++++++++++++++
 localedata/Makefile                        | 35 ++++++++++++++++---
 localedata/tst-langinfo-newlocale-static.c |  1 +
 localedata/tst-langinfo-newlocale.c        | 55 ++++++++++++++++++++++++++++++
 localedata/tst-langinfo-setlocale-static.c |  1 +
 localedata/tst-langinfo-setlocale.c        | 54 +++++++++++++++++++++++++++++
 localedata/tst-langinfo.c                  | 24 ++-----------
 localedata/tst-langinfo.sh                 |  4 +++
 8 files changed, 176 insertions(+), 27 deletions(-)
 create mode 100644 localedata/tst-langinfo-newlocale-static.c
 create mode 100644 localedata/tst-langinfo-newlocale.c
 create mode 100644 localedata/tst-langinfo-setlocale-static.c
 create mode 100644 localedata/tst-langinfo-setlocale.c

diff --git a/ChangeLog b/ChangeLog
index 6e9b14cffe..9d5d232f79 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,32 @@
+2018-05-14  Carlos O'Donell  <carlos@redhat.com>
+
+	[BZ #23164]
+	* localedata/tst-langinfo-setlocale.c: New file.
+	* localedata/tst-langinfo-setlocale-static.c: New file.
+	* localedata/tst-langinfo-newlocale.c: New file.
+	* localedata/tst-langinfo-newlocale-static.c: New file.
+	* localedata/Makefile (test-srcs): Remove tst-langinfo. Add
+	tst-langinfo-setlocale, tst-langinfo-setlocale-static,
+	tst-langinfo-newlocale, tst-langinfo-newlocale-static.
+	(tests-static): Remove tst-langinfo-static. Add
+	tst-langinfo-newlocale-static, tst-langinfo-setlocale-static.
+	(tests-special): Remove $(objpfx)tst-langinfo.out,
+	$(objpfx)tst-langinfo-static.out. Add
+	$(objpfx)tst-langinfo-setlocale.out,
+	$(objpfx)tst-langinfo-newlocale.out,
+	$(objpfx)tst-langinfo-setlocale-static.out,
+	$(objpfx)tst-langinfo-newlocale-static.out.
+	($(objpfx)tst-langinfo.out): Remove.
+	($(objpfx)tst-langinfo-static.out): Remove.
+	($(objpfx)tst-langinfo-newlocale.out): New target.
+	($(objpfx)tst-langinfo-newlocale-static.out): New target.
+	(test-xfail-tst-langinfo-newlocale-static): Add.
+	($(objpfx)tst-langinfo-setlocale.out): New target.
+	($(objpfx)tst-langinfo-setlocale-static.out): New target.
+	* localedata/tst-langinfo.c: Call test_locale.
+	* localedata/tst-langinfo.sh: Add LC_MONETARY CURRENCY_SYMBOL test
+	data.
+
 2018-05-18  Joseph Myers  <joseph@codesourcery.com>
 
 	* math/gen-tgmath-tests.py: Import sys.
diff --git a/localedata/Makefile b/localedata/Makefile
index d51064adec..2e6e0dcb2a 100644
--- a/localedata/Makefile
+++ b/localedata/Makefile
@@ -34,7 +34,9 @@ vpath %.h tests-mbwc
 
 
 test-srcs := collate-test xfrm-test tst-fmon tst-rpmatch tst-trans \
-	     tst-ctype tst-langinfo tst-langinfo-static tst-numeric
+	     tst-ctype tst-langinfo-newlocale tst-langinfo-setlocale \
+	     tst-langinfo-newlocale-static tst-langinfo-setlocale-static \
+	     tst-numeric
 # List of test input files (list sorted alphabetically):
 test-input := \
 	am_ET.UTF-8 \
@@ -168,13 +170,16 @@ install-others := $(addprefix $(inst_i18ndir)/, \
 
 tests: $(objdir)/iconvdata/gconv-modules
 
-tests-static += tst-langinfo-static
+tests-static += tst-langinfo-newlocale-static tst-langinfo-setlocale-static
 
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)sort-test.out $(objpfx)tst-fmon.out \
 		 $(objpfx)tst-locale.out $(objpfx)tst-rpmatch.out \
 		 $(objpfx)tst-trans.out $(objpfx)tst-ctype.out \
-		 $(objpfx)tst-langinfo.out $(objpfx)tst-langinfo-static.out \
+		 $(objpfx)tst-langinfo-newlocale.out \
+		 $(objpfx)tst-langinfo-setlocale.out \
+		 $(objpfx)tst-langinfo-newlocale-static.out \
+		 $(objpfx)tst-langinfo-setlocale-static.out \
 		 $(objpfx)tst-numeric.out
 # We have to generate locales (list sorted alphabetically)
 LOCALES := \
@@ -332,18 +337,38 @@ $(objpfx)tst-ctype.out: tst-ctype.sh $(objpfx)tst-ctype \
 	$(SHELL) $< $(common-objpfx) '$(test-program-cmd-before-env)' \
 		 '$(run-program-env)' '$(test-program-cmd-after-env)' > $@; \
 	$(evaluate-test)
-$(objpfx)tst-langinfo.out: tst-langinfo.sh $(objpfx)tst-langinfo \
+$(objpfx)tst-langinfo-newlocale.out: tst-langinfo.sh \
+			$(objpfx)tst-langinfo-newlocale \
 			$(objpfx)sort-test.out \
 			$(addprefix $(objpfx),$(CTYPE_FILES))
 	$(SHELL) $< $(common-objpfx) '$(test-program-cmd-before-env)' \
 		 '$(run-program-env)' '$(test-program-cmd-after-env)' > $@; \
 	$(evaluate-test)
-$(objpfx)tst-langinfo-static.out: tst-langinfo.sh $(objpfx)tst-langinfo-static \
+$(objpfx)tst-langinfo-newlocale-static.out: tst-langinfo.sh \
+			$(objpfx)tst-langinfo-newlocale-static \
 			$(objpfx)sort-test.out \
 			$(addprefix $(objpfx),$(CTYPE_FILES))
 	$(SHELL) $< $(common-objpfx) '$(test-program-cmd-before-env)' \
 		 '$(run-program-env)' '$(test-program-cmd-after-env)' > $@; \
 	$(evaluate-test)
+# Static use of newlocale is known not to work. See Bug 23164.
+test-xfail-tst-langinfo-newlocale-static = yes
+
+$(objpfx)tst-langinfo-setlocale.out: tst-langinfo.sh \
+			$(objpfx)tst-langinfo-setlocale \
+			$(objpfx)sort-test.out \
+			$(addprefix $(objpfx),$(CTYPE_FILES))
+	$(SHELL) $< $(common-objpfx) '$(test-program-cmd-before-env)' \
+		 '$(run-program-env)' '$(test-program-cmd-after-env)' > $@; \
+	$(evaluate-test)
+$(objpfx)tst-langinfo-setlocale-static.out: tst-langinfo.sh \
+			$(objpfx)tst-langinfo-setlocale-static \
+			$(objpfx)sort-test.out \
+			$(addprefix $(objpfx),$(CTYPE_FILES))
+	$(SHELL) $< $(common-objpfx) '$(test-program-cmd-before-env)' \
+		 '$(run-program-env)' '$(test-program-cmd-after-env)' > $@; \
+	$(evaluate-test)
+
 $(objpfx)tst-digits.out: $(objpfx)tst-locale.out
 $(objpfx)tst-mbswcs6.out: $(addprefix $(objpfx),$(CTYPE_FILES))
 endif
diff --git a/localedata/tst-langinfo-newlocale-static.c b/localedata/tst-langinfo-newlocale-static.c
new file mode 100644
index 0000000000..8097ecd96f
--- /dev/null
+++ b/localedata/tst-langinfo-newlocale-static.c
@@ -0,0 +1 @@
+#include <tst-langinfo-newlocale.c>
diff --git a/localedata/tst-langinfo-newlocale.c b/localedata/tst-langinfo-newlocale.c
new file mode 100644
index 0000000000..d29a5101e1
--- /dev/null
+++ b/localedata/tst-langinfo-newlocale.c
@@ -0,0 +1,55 @@
+/* Test program for newlocale() + nl_langinfo_l() functions.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <langinfo.h>
+#include <locale.h>
+#include <stdio.h>
+#include <string.h>
+
+/* Return 0 if the test passed, 1 for failed.  */
+static int
+test_locale (char *locale, char *paramstr, int param, char *expected)
+{
+  char *actual;
+  locale_t loc;
+  int result = 0;
+
+  loc = newlocale (LC_ALL_MASK, locale, 0);
+  if (loc == NULL)
+    {
+      puts (": failed to create new locale");
+      return 1;
+    }
+
+  printf ("nl_langinfo_l(%s, %s [%p])", paramstr, locale, loc);
+  actual = nl_langinfo_l(param, loc);
+  printf (" = \"%s\", ", actual);
+
+  if (strcmp (actual, expected) == 0)
+    puts ("OK");
+  else
+    {
+      printf ("FAILED (expected: %s)\n", expected);
+      result = 1;
+    }
+
+  freelocale (loc);
+  return result;
+}
+
+#include <tst-langinfo.c>
diff --git a/localedata/tst-langinfo-setlocale-static.c b/localedata/tst-langinfo-setlocale-static.c
new file mode 100644
index 0000000000..055d1325c4
--- /dev/null
+++ b/localedata/tst-langinfo-setlocale-static.c
@@ -0,0 +1 @@
+#include <tst-langinfo-setlocale.c>
diff --git a/localedata/tst-langinfo-setlocale.c b/localedata/tst-langinfo-setlocale.c
new file mode 100644
index 0000000000..3c41f109f4
--- /dev/null
+++ b/localedata/tst-langinfo-setlocale.c
@@ -0,0 +1,54 @@
+/* Test program for setlocale() + nl_langinfo() functions.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <langinfo.h>
+#include <locale.h>
+#include <stdio.h>
+#include <string.h>
+
+/* Return 0 if the test passed, 1 for failed.  */
+static int
+test_locale (char *locale, char *paramstr, int param, char *expected)
+{
+  char *actual;
+
+  printf ("LC_ALL=%s nl_langinfo(%s)", locale, paramstr);
+
+  /* Set the locale and check whether it worked.  */
+  setlocale (LC_ALL, locale);
+  if (strcmp (locale, setlocale (LC_ALL, NULL)) != 0)
+    {
+      puts (": failed to set locale");
+      return 1;
+    }
+
+  actual = nl_langinfo (param);
+  printf (" = \"%s\", ", actual);
+
+  if (strcmp (actual, expected) == 0)
+    puts ("OK");
+  else
+    {
+      printf ("FAILED (expected: %s)\n", expected);
+      return 1;
+    }
+
+  return 0;
+}
+
+#include <tst-langinfo.c>
diff --git a/localedata/tst-langinfo.c b/localedata/tst-langinfo.c
index 0d33e75215..5b2c117292 100644
--- a/localedata/tst-langinfo.c
+++ b/localedata/tst-langinfo.c
@@ -1,4 +1,4 @@
-/* Test program for nl_langinfo() function.
+/* Test driver for nl_langinfo[_l] functions.
    Copyright (C) 2000-2018 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@cygnus.com>.
@@ -162,7 +162,6 @@ do_test (void)
       char *locale;
       char *paramstr;
       char *expected;
-      char *actual;
       int param;
 
       if (fgets (buf, sizeof (buf), stdin) == NULL)
@@ -269,26 +268,7 @@ do_test (void)
 	  continue;
 	}
 
-      /* Set the locale and check whether it worked.  */
-      printf ("LC_ALL=%s nl_langinfo(%s)", locale, paramstr);
-      setlocale (LC_ALL, locale);
-      if (strcmp (locale, setlocale (LC_ALL, NULL)) != 0)
-	{
-	  puts (": failed to set locale");
-	  result = 1;
-	  continue;
-	}
-
-      actual = nl_langinfo (param);
-      printf (" = \"%s\", ", actual);
-
-      if (strcmp (actual, expected) == 0)
-	puts ("OK");
-      else
-	{
-	  printf ("FAILED (expected: %s)\n", expected);
-	  result = 1;
-	}
+      result = test_locale (locale, paramstr, param, expected);
     }
 
   return result;
diff --git a/localedata/tst-langinfo.sh b/localedata/tst-langinfo.sh
index d6787ca369..400ea6d36c 100755
--- a/localedata/tst-langinfo.sh
+++ b/localedata/tst-langinfo.sh
@@ -157,6 +157,7 @@ en_US.ISO-8859-1     RADIXCHAR   .
 en_US.ISO-8859-1     THOUSEP     ,
 en_US.ISO-8859-1     YESEXPR     ^[+1yY]
 en_US.ISO-8859-1     NOEXPR      ^[-0nN]
+en_US.UTF-8	     CURRENCY_SYMBOL	$
 de_DE.ISO-8859-1     ABDAY_1     So
 de_DE.ISO-8859-1     ABDAY_2     Mo
 de_DE.ISO-8859-1     ABDAY_3     Di
@@ -247,6 +248,7 @@ de_DE.UTF-8          RADIXCHAR   ,
 de_DE.UTF-8          THOUSEP     .
 de_DE.UTF-8          YESEXPR     ^[+1jJyY]
 de_DE.UTF-8          NOEXPR      ^[-0nN]
+de_DE.UTF-8          CURRENCY_SYMBOL    €
 fr_FR.ISO-8859-1     ABDAY_1     dim.
 fr_FR.ISO-8859-1     ABDAY_2     lun.
 fr_FR.ISO-8859-1     ABDAY_3     mar.
@@ -292,6 +294,7 @@ fr_FR.ISO-8859-1     RADIXCHAR   ,
 fr_FR.ISO-8859-1     THOUSEP     " "
 fr_FR.ISO-8859-1     YESEXPR     ^[+1oOyY]
 fr_FR.ISO-8859-1     NOEXPR      ^[-0nN]
+fr_FR.UTF-8          CURRENCY_SYMBOL    €
 ja_JP.EUC-JP         ABDAY_1     Æü
 ja_JP.EUC-JP         ABDAY_2     ·î
 ja_JP.EUC-JP         ABDAY_3     ²Ð
@@ -340,6 +343,7 @@ ja_JP.EUC-JP         NOEXPR      ^([-0nN
 # Is CRNCYSTR supposed to be the national or international sign?
 # ja_JP.EUC-JP         CRNCYSTR    JPY
 ja_JP.EUC-JP         CODESET     EUC-JP
+ja_JP.UTF-8          CURRENCY_SYMBOL    ï¿¥
 EOF
 ${tst_langinfo_before_env} \
 ${run_program_env} \
-- 
2.14.3

Re: [PATCH] locale: XFAIL newlocale usage in static binary (Bug 23164)

[Carlos O'Donell]

On 05/21/2018 02:14 PM, Carlos O'Donell wrote:
> There is a glibc optimization which allows for locale categories
> to be removed during static compilation. There have been various
> bugs for this support over the years, with bug 16915 being the
> most recent. The solution there was to emit a reference to all the
> categories to avoid any being removed. This fix, although it's in
> the generic __nl_langinfo_l function, doesn't appear to be enough
> to fix the case for a statically linked program that uses newlocale
> and nl_langinfo_l. This commit doesn't fix the problem, but it does
> add a XFAIL'd test case such that a fix can be applied against this
> and the XFAIL removed. It's not entirely clear that the problem is
> the same as that which was seen in bug 16915.
> 
> The commit makes tst-langinfo.c into a test driver for use by two
> new tests which use setenv/nl_langinfo or newlocale/nl_langinfo_l,
> both dynamic (which pass) and static (for which tst-langinfo-newlocale-static
> fails and is XFAIL'd). In addition we add CURRENCY_SYMBOL test coverage
> which was the original problem reported in the related gcc/C++ PR.
> 
> OK to commit?
 
If nobody objects I'll commit the new XFAIL'd test case next Monday.

-- 
Cheers,
Carlos.

Re: [PATCH] locale: XFAIL newlocale usage in static binary (Bug 23164)

[Rafal Luzynski]

Carlos,

This is a very incomplete review.  So far I can only confirm that your
patch compiles and adds one more XFAIL to the test results.

21.05.2018 20:14 Carlos O'Donell <carlos@redhat.com> wrote:
>
> [...] In addition we add CURRENCY_SYMBOL test coverage
> which was the original problem reported in the related gcc/C++ PR.

Would you please mention it in the commit message?

> There is a glibc optimization which allows for locale categories
> to be removed during static compilation. There have been various
> [...]

I think this is just a nitpick but as far as I can see most commit
messages contain a double space between the sentences.  The same
applies to your whole commit message.

> diff --git a/localedata/Makefile b/localedata/Makefile
> index d51064adec..2e6e0dcb2a 100644
> --- a/localedata/Makefile
> +++ b/localedata/Makefile
> @@ -34,7 +34,9 @@ vpath %.h tests-mbwc
>  
>  
>  test-srcs := collate-test xfrm-test tst-fmon tst-rpmatch tst-trans \
> -	     tst-ctype tst-langinfo tst-langinfo-static tst-numeric
> +	     tst-ctype tst-langinfo-newlocale tst-langinfo-setlocale \
> +	     tst-langinfo-newlocale-static tst-langinfo-setlocale-static \
> +	     tst-numeric

OK, you remove tst-langinfo and tst-langinfo-static and add
tst-langinfo-newlocale, tst-langinfo-setlocale, tst-langinfo-newlocale-static,
and tst-langinfo-setlocale-static instead.

> -tests-static += tst-langinfo-static
> +tests-static += tst-langinfo-newlocale-static tst-langinfo-setlocale-static

OK, consequently tst-langinfo-static replaced with
tst-langinfo-newlocale-static and tst-langinfo-setlocale-static.
Where is tst-langinfo replaced with tst-langinfo-newlocale and
tst-langinfo-setlocale?  I guess it happens somewhere automagically.

> -		 $(objpfx)tst-langinfo.out $(objpfx)tst-langinfo-static.out \
> +		 $(objpfx)tst-langinfo-newlocale.out \
> +		 $(objpfx)tst-langinfo-setlocale.out \
> +		 $(objpfx)tst-langinfo-newlocale-static.out \
> +		 $(objpfx)tst-langinfo-setlocale-static.out \

OK, consequently the test results file names changed and the same in more
places in Makefile, so I will just skip it.

> diff --git a/localedata/tst-langinfo-newlocale-static.c
> b/localedata/tst-langinfo-newlocale-static.c
> new file mode 100644
> index 0000000000..8097ecd96f
> --- /dev/null
> +++ b/localedata/tst-langinfo-newlocale-static.c
> @@ -0,0 +1 @@
> +#include <tst-langinfo-newlocale.c>

Maybe I'm wrong but instead of creating a new file whose only line is
"#include <another-file.c>" wouldn't it be better to generate the static
binaries from the same source file(s) but just with different build
options?

> diff --git a/localedata/tst-langinfo-setlocale-static.c
> b/localedata/tst-langinfo-setlocale-static.c
> new file mode 100644
> index 0000000000..055d1325c4
> --- /dev/null
> +++ b/localedata/tst-langinfo-setlocale-static.c

Same here.

diff --git a/localedata/tst-langinfo.sh b/localedata/tst-langinfo.sh
index d6787ca369..400ea6d36c 100755
--- a/localedata/tst-langinfo.sh
+++ b/localedata/tst-langinfo.sh
@@ -157,6 +157,7 @@ en_US.ISO-8859-1     RADIXCHAR   .
 en_US.ISO-8859-1     THOUSEP     ,
 en_US.ISO-8859-1     YESEXPR     ^[+1yY]
 en_US.ISO-8859-1     NOEXPR      ^[-0nN]
+en_US.UTF-8	     CURRENCY_SYMBOL	$

OK, correct currency symbol for USA, and also correct for Germany, France,
and Japan below (skipped here).


> OK to commit?

Again, my review is very incomplete so I cannot guarantee and I cannot
give my "Reviewed-by" tag.

That said, I don't object so unless anybody else objects it is OK.
Feel free to take my nitpicks into account or reject them, they
are not strong objections.

Regards,

Rafal

Re: [PATCH] locale: XFAIL newlocale usage in static binary (Bug 23164)

[Carlos O'Donell]

On 05/23/2018 07:55 PM, Rafal Luzynski wrote:
> This is a very incomplete review.  So far I can only confirm that your
> patch compiles and adds one more XFAIL to the test results.

Thanks for that review.

I've committed what I posted.

Comments follow.

> 21.05.2018 20:14 Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> [...] In addition we add CURRENCY_SYMBOL test coverage
>> which was the original problem reported in the related gcc/C++ PR.
> 
> Would you please mention it in the commit message?

I tried to keep the commit message simple.

To meet your requirement I have added the gcc/C++ PR linked into
bug 23164 e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85732

This way the bugzilla can be kept up to date with all the linked-in
bugs from gcc/C++ PR that relate to this issue.

>> There is a glibc optimization which allows for locale categories
>> to be removed during static compilation. There have been various
>> [...]
> 
> I think this is just a nitpick but as far as I can see most commit
> messages contain a double space between the sentences.  The same
> applies to your whole commit message.

Thanks. I thought I had fixed this, but apparently I missed this.

Yes, we should follow GNU formatting and use 2 spaces after a period.


>> diff --git a/localedata/tst-langinfo-newlocale-static.c
>> b/localedata/tst-langinfo-newlocale-static.c
>> new file mode 100644
>> index 0000000000..8097ecd96f
>> --- /dev/null
>> +++ b/localedata/tst-langinfo-newlocale-static.c
>> @@ -0,0 +1 @@
>> +#include <tst-langinfo-newlocale.c>
> 
> Maybe I'm wrong but instead of creating a new file whose only line is
> "#include <another-file.c>" wouldn't it be better to generate the static
> binaries from the same source file(s) but just with different build
> options?

The problem is that the glibc build infrastructure ties the build options
to the file name.

e.g.
test-foo-CFLAGS = -static

Therefore you can only have either shared or static on the same source
file, which is why you see a common pattern in glibc which is to include
the shared source file in a "static" variant.

If we had reliable support for symlinks in VCS then we could use that too,
but we don't.

>> diff --git a/localedata/tst-langinfo-setlocale-static.c
>> b/localedata/tst-langinfo-setlocale-static.c
>> new file mode 100644
>> index 0000000000..055d1325c4
>> --- /dev/null
>> +++ b/localedata/tst-langinfo-setlocale-static.c
> 
> Same here.

Same answer.

> diff --git a/localedata/tst-langinfo.sh b/localedata/tst-langinfo.sh
> index d6787ca369..400ea6d36c 100755
> --- a/localedata/tst-langinfo.sh
> +++ b/localedata/tst-langinfo.sh
> @@ -157,6 +157,7 @@ en_US.ISO-8859-1     RADIXCHAR   .
>  en_US.ISO-8859-1     THOUSEP     ,
>  en_US.ISO-8859-1     YESEXPR     ^[+1yY]
>  en_US.ISO-8859-1     NOEXPR      ^[-0nN]
> +en_US.UTF-8	     CURRENCY_SYMBOL	$
> 
> OK, correct currency symbol for USA, and also correct for Germany, France,
> and Japan below (skipped here).

Thanks for checking! 

>> OK to commit?
> 
> Again, my review is very incomplete so I cannot guarantee and I cannot
> give my "Reviewed-by" tag.

Thanks, that's OK.

> That said, I don't object so unless anybody else objects it is OK.
> Feel free to take my nitpicks into account or reject them, they
> are not strong objections.

Thank you again for the review!

In the future may I ask that you TO me directly on a review.
It's hard for me to filter reviews of my patches that only respond to
the list?

-- 
Cheers,
Carlos.

Re: [PATCH] locale: XFAIL newlocale usage in static binary (Bug 23164)

[Rafal Luzynski]

4.07.2018 21:35 Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 05/23/2018 07:55 PM, Rafal Luzynski wrote:
> > This is a very incomplete review. So far I can only confirm that your
> > patch compiles and adds one more XFAIL to the test results.
>
> Thanks for that review.
>
> I've committed what I posted.

Thank you, good to see this commit pushed.  Shouldn't you close the
bug report as well, and mark the Target Milestone 2.28?

> Comments follow.
>
> > 21.05.2018 20:14 Carlos O'Donell <carlos@redhat.com> wrote:
> >>
> >> [...] In addition we add CURRENCY_SYMBOL test coverage
> >> which was the original problem reported in the related gcc/C++ PR.
> >
> > Would you please mention it in the commit message?
>
> I tried to keep the commit message simple.

I like what you have put in the ChangeLog and I thought that every
commit message should contain the ChangeLog entry copied.  Of course
it was correct to remove ChangeLog diff in libc-alpha post.  Never mind,
maybe I'm wrong and even if not it is probably impossible to fix the
commit comment now.

> To meet your requirement I have added the gcc/C++ PR linked into
> bug 23164 e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85732
>
> This way the bugzilla can be kept up to date with all the linked-in
> bugs from gcc/C++ PR that relate to this issue.

That's more than I expected but great, thank you.

[ cut the part where I just totally agree ]

> [...]
> In the future may I ask that you TO me directly on a review.
> It's hard for me to filter reviews of my patches that only respond to
> the list?

That's what I usually do.  Didn't I this time?  I'm sorry about that.
I don't understand why I did not, most probably my mistake.  If I had
any good reason then I don't remember it now, it's been a while ago.

Regards,

Rafal

Re: [PATCH] locale: XFAIL newlocale usage in static binary (Bug 23164)

[Carlos O'Donell]

On 07/04/2018 04:28 PM, Rafal Luzynski wrote:
> 4.07.2018 21:35 Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> On 05/23/2018 07:55 PM, Rafal Luzynski wrote:
>>> This is a very incomplete review. So far I can only confirm that your
>>> patch compiles and adds one more XFAIL to the test results.
>>
>> Thanks for that review.
>>
>> I've committed what I posted.
> 
> Thank you, good to see this commit pushed.  Shouldn't you close the
> bug report as well, and mark the Target Milestone 2.28?

No, the bug is not fixed. I just added the regression tests that prove
the bug is not fixed. Someone still has to do the work to fix the bug.

So sadly the bug is still there :-( ... but having a reproducible test
case is half the battle.

>> [...]
>> In the future may I ask that you TO me directly on a review.
>> It's hard for me to filter reviews of my patches that only respond to
>> the list?
> 
> That's what I usually do.  Didn't I this time?  I'm sorry about that.
> I don't understand why I did not, most probably my mistake.  If I had
> any good reason then I don't remember it now, it's been a while ago.

Thanks. No worries. I found the review by going back to the list and
cross checking for any replies.

-- 
Cheers,
Carlos.