Re: libc-2.25.90.pot

Re: libc-2.25.90.pot

[Benno Schulenberg]

>> The GNU C Library is about to release version 2.26.
>>
>> * Complete snapshot of 2.16 release containing `po/libc.pot':
>> ftp://alpha.gnu.org/gnu/glibc/glibc-2.25-737-g66cd050.tar.bz2
> 
> I see these changes when compared to the previous POT file:
> 
> -msgid "\"%s\", line %d: "
> +msgid "\"%s\", line %"
> 
> -msgid " (rule from \"%s\", line %d)"
> +msgid " (rule from \"%s\", line %"
> 
> Are you sure these changes are intentional?  If yes, then maybe
> they could use a translator hint, to say what is coming after the
> last "%"?  Also, since those strings are not marked as c-format,
> a malicious translator might be able to crash the relevant program
> by putting in %tralala or something instead of %s?

Ping?

Benno

Re: libc-2.25.90.pot

[Adhemerval Zanella]

On 18/07/2017 14:08, Benno Schulenberg wrote:
> 
>>> The GNU C Library is about to release version 2.26.
>>>
>>> * Complete snapshot of 2.16 release containing `po/libc.pot':
>>> ftp://alpha.gnu.org/gnu/glibc/glibc-2.25-737-g66cd050.tar.bz2
>>
>> I see these changes when compared to the previous POT file:
>>
>> -msgid "\"%s\", line %d: "
>> +msgid "\"%s\", line %"
>>
>> -msgid " (rule from \"%s\", line %d)"
>> +msgid " (rule from \"%s\", line %"
>>
>> Are you sure these changes are intentional?  If yes, then maybe
>> they could use a translator hint, to say what is coming after the
>> last "%"?  Also, since those strings are not marked as c-format,
>> a malicious translator might be able to crash the relevant program
>> by putting in %tralala or something instead of %s?
> 
> Ping?
> 
> Benno

I think these a limitation of xgettext used to generate the pot files.
Both are similar defined:

timezone/zic.c

# msgid "\"%s\", line %"
487           fprintf(stderr, _("\"%s\", line %"PRIdLINENO": "), filename, linenum);

# msgid " (rule from \"%s\", line %"
490                 fprintf(stderr, _(" (rule from \"%s\", line %"PRIdLINENO")")

And 'PRIdLINENO' is defined as 

50 typedef intmax_t lineno;
51 #define PRIdLINENO PRIdMAX

Which means we will get different string encodings depending of how
__PRI64_PREFIX is defined by the ABI.

Both changes came from '92bd70fb' (Update timezone code from tzcode 2017b.) and
from original tz project it seems to get from cc8aec6ecb1ecc (zic: some
integer-width fixups).  TZ commit log seems to indicate this is just a integer
cleanup without original from an actual bug or limitation.

Paul I think we can use default %d for ints for these two specific printf to
make translation work as intended. What do you think?

Re: libc-2.25.90.pot

[Siddhesh Poyarekar]

Looping libc-alpha into the response as well; I just noticed that you
pinged here too.

Siddhesh

On Wednesday 19 July 2017 12:38 PM, Siddhesh Poyarekar wrote:
> On Monday 17 July 2017 01:31 AM, Benno Schulenberg wrote:
>> I see these changes when compared to the previous POT file:
>>
>> -msgid "\"%s\", line %d: "
>> +msgid "\"%s\", line %"
>>
>> -msgid " (rule from \"%s\", line %d)"
>> +msgid " (rule from \"%s\", line %"
>>
>> Are you sure these changes are intentional?  If yes, then maybe
>> they could use a translator hint, to say what is coming after the
>> last "%"?  Also, since those strings are not marked as c-format,
>> a malicious translator might be able to crash the relevant program
>> by putting in %tralala or something instead of %s?
> 
> It is a safe and intentional change; the full string is:
> 
> "\"%s\", line %"PRIdLINENO": "
> 
> where PRIdLINENO is PRIdMAX, which is a signed integer of maximum width,
> which could change for different architectures.  It is however a numeric
> type.
> 
> Would you like me to add a comment on top of that line to indicate that
> it is a numeric type?  I reckon the string itself is self-explanatory.
> 
> Siddhesh
> 

Re: libc-2.25.90.pot

[Siddhesh Poyarekar]

On Wednesday 19 July 2017 12:32 AM, Adhemerval Zanella wrote:
> I think these a limitation of xgettext used to generate the pot files.
> Both are similar defined:
> 
> timezone/zic.c
> 
> # msgid "\"%s\", line %"
> 487           fprintf(stderr, _("\"%s\", line %"PRIdLINENO": "), filename, linenum);
> 
> # msgid " (rule from \"%s\", line %"
> 490                 fprintf(stderr, _(" (rule from \"%s\", line %"PRIdLINENO")")
> 
> And 'PRIdLINENO' is defined as 
> 
> 50 typedef intmax_t lineno;
> 51 #define PRIdLINENO PRIdMAX
> 
> Which means we will get different string encodings depending of how
> __PRI64_PREFIX is defined by the ABI.
> 
> Both changes came from '92bd70fb' (Update timezone code from tzcode 2017b.) and
> from original tz project it seems to get from cc8aec6ecb1ecc (zic: some
> integer-width fixups).  TZ commit log seems to indicate this is just a integer
> cleanup without original from an actual bug or limitation.
> 
> Paul I think we can use default %d for ints for these two specific printf to
> make translation work as intended. What do you think?

There are three of those instances and they refer to line numbers in the
timezone data file.  If we can assume that the tzdata files are sane
then I suppose we could assume %d for these.

Siddhesh

Re: libc-2.25.90.pot

[Benno Schulenberg]

Op 19-07-2017 om 09:15 schreef Siddhesh Poyarekar:
> On Wednesday 19 July 2017 12:32 AM, Adhemerval Zanella wrote:
>>
>> And 'PRIdLINENO' is defined as
>>
>> 50 typedef intmax_t lineno;
>> 51 #define PRIdLINENO PRIdMAX
>>
>> Which means we will get different string encodings depending of how
>> __PRI64_PREFIX is defined by the ABI.
>>
>> Both changes came from '92bd70fb' (Update timezone code from tzcode 2017b.) and
>> from original tz project it seems to get from cc8aec6ecb1ecc (zic: some
>> integer-width fixups).  TZ commit log seems to indicate this is just a integer
>> cleanup without original from an actual bug or limitation.
>>
>> Paul I think we can use default %d for ints for these two specific printf to
>> make translation work as intended. What do you think?
> 
> There are three of those instances and they refer to line numbers in the
> timezone data file.  If we can assume that the tzdata files are sane
> then I suppose we could assume %d for these.

Or at least change them to something that xgettext recognizes.  There are
several instances of %<PRIuMAX> and %<PRIu64> in the POT file, so xgettext
is able to recognize some of those "PRI..." strings, but seemingly not
custom-defined ones?

Benno

Re: libc-2.25.90.pot

[Adhemerval Zanella]

On 19/07/2017 04:33, Benno Schulenberg wrote:
> Op 19-07-2017 om 09:15 schreef Siddhesh Poyarekar:
>> On Wednesday 19 July 2017 12:32 AM, Adhemerval Zanella wrote:
>>>
>>> And 'PRIdLINENO' is defined as
>>>
>>> 50 typedef intmax_t lineno;
>>> 51 #define PRIdLINENO PRIdMAX
>>>
>>> Which means we will get different string encodings depending of how
>>> __PRI64_PREFIX is defined by the ABI.
>>>
>>> Both changes came from '92bd70fb' (Update timezone code from tzcode 2017b.) and
>>> from original tz project it seems to get from cc8aec6ecb1ecc (zic: some
>>> integer-width fixups).  TZ commit log seems to indicate this is just a integer
>>> cleanup without original from an actual bug or limitation.
>>>
>>> Paul I think we can use default %d for ints for these two specific printf to
>>> make translation work as intended. What do you think?
>>
>> There are three of those instances and they refer to line numbers in the
>> timezone data file.  If we can assume that the tzdata files are sane
>> then I suppose we could assume %d for these.
> 
> Or at least change them to something that xgettext recognizes.  There are
> several instances of %<PRIuMAX> and %<PRIu64> in the POT file, so xgettext
> is able to recognize some of those "PRI..." strings, but seemingly not
> custom-defined ones?

I think for glibc, since we support C99 intmax_t printf format, we can
just use %jd instead:

	* timezone/zic.c (PRIdLINENO): Remove macro.
	(verror): Use %jd instead of PRIdLINENO.
---
 ChangeLog      | 5 +++++
 timezone/zic.c | 7 +++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/timezone/zic.c b/timezone/zic.c
index 068fb43..aaef06b 100644
--- a/timezone/zic.c
+++ b/timezone/zic.c
@@ -48,7 +48,6 @@ static ptrdiff_t const PTRDIFF_MAX = MAXVAL(ptrdiff_t, TYPE_BIT(ptrdiff_t));
 
 /* The type and printf format for line numbers.  */
 typedef intmax_t lineno;
-#define PRIdLINENO PRIdMAX
 
 struct rule {
 	const char *	r_filename;
@@ -484,10 +483,10 @@ verror(const char *const string, va_list args)
 	** on BSD systems.
 	*/
 	if (filename)
-	  fprintf(stderr, _("\"%s\", line %"PRIdLINENO": "), filename, linenum);
+	  fprintf(stderr, _("\"%s\", line %jd: "), filename, linenum);
 	vfprintf(stderr, string, args);
 	if (rfilename != NULL)
-		fprintf(stderr, _(" (rule from \"%s\", line %"PRIdLINENO")"),
+		fprintf(stderr, _(" (rule from \"%s\", line %jd)"),
 			rfilename, rlinenum);
 	fprintf(stderr, "\n");
 }
@@ -1250,7 +1249,7 @@ _("\"Zone %s\" line and -p option are mutually exclusive"),
 		if (zones[i].z_name != NULL &&
 			strcmp(zones[i].z_name, fields[ZF_NAME]) == 0) {
 				error(_("duplicate zone name %s"
-					" (file \"%s\", line %"PRIdLINENO")"),
+					" (file \"%s\", line %jd)"),
 					fields[ZF_NAME],
 					zones[i].z_filename,
 					zones[i].z_linenum);
-- 

Re: libc-2.25.90.pot

[Siddhesh Poyarekar]

On Wednesday 19 July 2017 06:58 PM, Adhemerval Zanella wrote:
> I think for glibc, since we support C99 intmax_t printf format, we can
> just use %jd instead:
> 
> 	* timezone/zic.c (PRIdLINENO): Remove macro.
> 	(verror): Use %jd instead of PRIdLINENO.

Fine with me, but I'd like to hear from Joseph/Paul too if they have any
context from the tz project that may make this a bad idea.

Siddhesh

> ---
>  ChangeLog      | 5 +++++
>  timezone/zic.c | 7 +++----
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/timezone/zic.c b/timezone/zic.c
> index 068fb43..aaef06b 100644
> --- a/timezone/zic.c
> +++ b/timezone/zic.c
> @@ -48,7 +48,6 @@ static ptrdiff_t const PTRDIFF_MAX = MAXVAL(ptrdiff_t, TYPE_BIT(ptrdiff_t));
>  
>  /* The type and printf format for line numbers.  */
>  typedef intmax_t lineno;
> -#define PRIdLINENO PRIdMAX
>  
>  struct rule {
>  	const char *	r_filename;
> @@ -484,10 +483,10 @@ verror(const char *const string, va_list args)
>  	** on BSD systems.
>  	*/
>  	if (filename)
> -	  fprintf(stderr, _("\"%s\", line %"PRIdLINENO": "), filename, linenum);
> +	  fprintf(stderr, _("\"%s\", line %jd: "), filename, linenum);
>  	vfprintf(stderr, string, args);
>  	if (rfilename != NULL)
> -		fprintf(stderr, _(" (rule from \"%s\", line %"PRIdLINENO")"),
> +		fprintf(stderr, _(" (rule from \"%s\", line %jd)"),
>  			rfilename, rlinenum);
>  	fprintf(stderr, "\n");
>  }
> @@ -1250,7 +1249,7 @@ _("\"Zone %s\" line and -p option are mutually exclusive"),
>  		if (zones[i].z_name != NULL &&
>  			strcmp(zones[i].z_name, fields[ZF_NAME]) == 0) {
>  				error(_("duplicate zone name %s"
> -					" (file \"%s\", line %"PRIdLINENO")"),
> +					" (file \"%s\", line %jd)"),
>  					fields[ZF_NAME],
>  					zones[i].z_filename,
>  					zones[i].z_linenum);
> 

Re: libc-2.25.90.pot

[Siddhesh Poyarekar]

On Wednesday 19 July 2017 07:19 PM, Siddhesh Poyarekar wrote:
> On Wednesday 19 July 2017 06:58 PM, Adhemerval Zanella wrote:
>> I think for glibc, since we support C99 intmax_t printf format, we can
>> just use %jd instead:
>>
>> 	* timezone/zic.c (PRIdLINENO): Remove macro.
>> 	(verror): Use %jd instead of PRIdLINENO.
> 
> Fine with me, but I'd like to hear from Joseph/Paul too if they have any
> context from the tz project that may make this a bad idea.

In the interest of unblocking the translation team, lets wait till
tomorrow and then commit if there are no objections.  The more I look at
the change the more it looks like future-proofing and not relevant to a
current problem.  If the future-proofing is necessary, we can get it
back in 2.26.90 in a way that does not break translation strings.

Siddhesh

Re: libc-2.25.90.pot

[Adhemerval Zanella]

On 20/07/2017 08:17, Siddhesh Poyarekar wrote:
> On Wednesday 19 July 2017 07:19 PM, Siddhesh Poyarekar wrote:
>> On Wednesday 19 July 2017 06:58 PM, Adhemerval Zanella wrote:
>>> I think for glibc, since we support C99 intmax_t printf format, we can
>>> just use %jd instead:
>>>
>>> 	* timezone/zic.c (PRIdLINENO): Remove macro.
>>> 	(verror): Use %jd instead of PRIdLINENO.
>>
>> Fine with me, but I'd like to hear from Joseph/Paul too if they have any
>> context from the tz project that may make this a bad idea.
> 
> In the interest of unblocking the translation team, lets wait till
> tomorrow and then commit if there are no objections.  The more I look at
> the change the more it looks like future-proofing and not relevant to a
> current problem.  If the future-proofing is necessary, we can get it
> back in 2.26.90 in a way that does not break translation strings.

I would not say future-proof, but rather the macro is to allow more
compatibility for different libc implementation (for the case where the
libc is c99 compatible but not support %jd).

Re: libc-2.25.90.pot

[Siddhesh Poyarekar]

On Thursday 20 July 2017 06:22 PM, Adhemerval Zanella wrote:
> I would not say future-proof, but rather the macro is to allow more
> compatibility for different libc implementation (for the case where the
> libc is c99 compatible but not support %jd).

Oh I meant PRIdLINENO, not PRIdMAX.  I suppose using PRIdMAX directly
might work since gettext seems to recognize the other PRI* macros.

Siddhesh

Re: libc-2.25.90.pot

[Paul Eggert]

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

On 07/20/2017 07:57 AM, Siddhesh Poyarekar wrote:
> Oh I meant PRIdLINENO, not PRIdMAX.  I suppose using PRIdMAX directly
> might work since gettext seems to recognize the other PRI* macros.

Yes, PRIdMAX should work. The zic.c code uses the macro (as opposed to 
just %jd) to be portable to pre-C99 libraries. I installed the attached 
patch into the upstream tzdb sources; the zic.c part of it should work 
for glibc too. Thanks for reporting the problem.


[-- Attachment #2: 0001-Fix-zic.c-incompatibility-with-xgettext.patch --]
[-- Type: text/x-patch, Size: 2478 bytes --]

From fd13571c626bac8f5bb90ad79543cefae0fef134 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 20 Jul 2017 12:34:12 -0700
Subject: [PATCH] Fix zic.c incompatibility with xgettext

Problem reported by Benno Schulenberg in:
https://sourceware.org/ml/libc-alpha/2017-07/msg00653.html
* NEWS: Document this.
* zic.c (PRIdLINENO): Remove.  All uses replaced with definiens.
---
 NEWS  |  3 +++
 zic.c | 11 ++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/NEWS b/NEWS
index 5f79225..64d5c91 100644
--- a/NEWS
+++ b/NEWS
@@ -102,6 +102,9 @@ Unreleased, experimental changes
     Calculation of time_t extrema works around a bug in GCC 4.8.4
     (Reported by Stan Shebs and Joseph Myers.)
 
+    zic.c no longer mistranslates formats of line numbers in non-English
+    locales.  (Problem reported by Benno Schulenberg.)
+
     Several minor changes have been made to the code to make it a
     bit easier to port to MS-Windows and Solaris.  (Thanks to Kees
     Dekker for reporting the problems.)
diff --git a/zic.c b/zic.c
index 300d563..765fe7d 100644
--- a/zic.c
+++ b/zic.c
@@ -58,9 +58,10 @@ typedef int_fast64_t	zic_t;
 static ptrdiff_t const PTRDIFF_MAX = MAXVAL(ptrdiff_t, TYPE_BIT(ptrdiff_t));
 #endif
 
-/* The type and printf format for line numbers.  */
+/* The type for line numbers.  Use PRIdMAX to format them; formerly
+   there was also "#define PRIdLINENO PRIdMAX" and formats used
+   PRIdLINENO, but xgettext cannot grok that.  */
 typedef intmax_t lineno;
-#define PRIdLINENO PRIdMAX
 
 struct rule {
 	const char *	r_filename;
@@ -499,10 +500,10 @@ verror(const char *const string, va_list args)
 	** on BSD systems.
 	*/
 	if (filename)
-	  fprintf(stderr, _("\"%s\", line %"PRIdLINENO": "), filename, linenum);
+	  fprintf(stderr, _("\"%s\", line %"PRIdMAX": "), filename, linenum);
 	vfprintf(stderr, string, args);
 	if (rfilename != NULL)
-		fprintf(stderr, _(" (rule from \"%s\", line %"PRIdLINENO")"),
+		fprintf(stderr, _(" (rule from \"%s\", line %"PRIdMAX")"),
 			rfilename, rlinenum);
 	fprintf(stderr, "\n");
 }
@@ -1264,7 +1265,7 @@ _("\"Zone %s\" line and -p option are mutually exclusive"),
 		if (zones[i].z_name != NULL &&
 			strcmp(zones[i].z_name, fields[ZF_NAME]) == 0) {
 				error(_("duplicate zone name %s"
-					" (file \"%s\", line %"PRIdLINENO")"),
+					" (file \"%s\", line %"PRIdMAX")"),
 					fields[ZF_NAME],
 					zones[i].z_filename,
 					zones[i].z_linenum);
-- 
2.13.3

Re: libc-2.25.90.pot

[Siddhesh Poyarekar]

I've applied this to glibc.

Siddhesh

On Friday 21 July 2017 01:11 AM, Paul Eggert wrote:
> On 07/20/2017 07:57 AM, Siddhesh Poyarekar wrote:
>> Oh I meant PRIdLINENO, not PRIdMAX.  I suppose using PRIdMAX directly
>> might work since gettext seems to recognize the other PRI* macros.
> 
> Yes, PRIdMAX should work. The zic.c code uses the macro (as opposed to
> just %jd) to be portable to pre-C99 libraries. I installed the attached
> patch into the upstream tzdb sources; the zic.c part of it should work
> for glibc too. Thanks for reporting the problem.
> 
> 
> 0001-Fix-zic.c-incompatibility-with-xgettext.patch
> 
> 
> From fd13571c626bac8f5bb90ad79543cefae0fef134 Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 20 Jul 2017 12:34:12 -0700
> Subject: [PATCH] Fix zic.c incompatibility with xgettext
> 
> Problem reported by Benno Schulenberg in:
> https://sourceware.org/ml/libc-alpha/2017-07/msg00653.html
> * NEWS: Document this.
> * zic.c (PRIdLINENO): Remove.  All uses replaced with definiens.
> ---
>  NEWS  |  3 +++
>  zic.c | 11 ++++++-----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 5f79225..64d5c91 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -102,6 +102,9 @@ Unreleased, experimental changes
>      Calculation of time_t extrema works around a bug in GCC 4.8.4
>      (Reported by Stan Shebs and Joseph Myers.)
>  
> +    zic.c no longer mistranslates formats of line numbers in non-English
> +    locales.  (Problem reported by Benno Schulenberg.)
> +
>      Several minor changes have been made to the code to make it a
>      bit easier to port to MS-Windows and Solaris.  (Thanks to Kees
>      Dekker for reporting the problems.)
> diff --git a/zic.c b/zic.c
> index 300d563..765fe7d 100644
> --- a/zic.c
> +++ b/zic.c
> @@ -58,9 +58,10 @@ typedef int_fast64_t	zic_t;
>  static ptrdiff_t const PTRDIFF_MAX = MAXVAL(ptrdiff_t, TYPE_BIT(ptrdiff_t));
>  #endif
>  
> -/* The type and printf format for line numbers.  */
> +/* The type for line numbers.  Use PRIdMAX to format them; formerly
> +   there was also "#define PRIdLINENO PRIdMAX" and formats used
> +   PRIdLINENO, but xgettext cannot grok that.  */
>  typedef intmax_t lineno;
> -#define PRIdLINENO PRIdMAX
>  
>  struct rule {
>  	const char *	r_filename;
> @@ -499,10 +500,10 @@ verror(const char *const string, va_list args)
>  	** on BSD systems.
>  	*/
>  	if (filename)
> -	  fprintf(stderr, _("\"%s\", line %"PRIdLINENO": "), filename, linenum);
> +	  fprintf(stderr, _("\"%s\", line %"PRIdMAX": "), filename, linenum);
>  	vfprintf(stderr, string, args);
>  	if (rfilename != NULL)
> -		fprintf(stderr, _(" (rule from \"%s\", line %"PRIdLINENO")"),
> +		fprintf(stderr, _(" (rule from \"%s\", line %"PRIdMAX")"),
>  			rfilename, rlinenum);
>  	fprintf(stderr, "\n");
>  }
> @@ -1264,7 +1265,7 @@ _("\"Zone %s\" line and -p option are mutually exclusive"),
>  		if (zones[i].z_name != NULL &&
>  			strcmp(zones[i].z_name, fields[ZF_NAME]) == 0) {
>  				error(_("duplicate zone name %s"
> -					" (file \"%s\", line %"PRIdLINENO")"),
> +					" (file \"%s\", line %"PRIdMAX")"),
>  					fields[ZF_NAME],
>  					zones[i].z_filename,
>  					zones[i].z_linenum);
>