GNU bug report logs - #24311
[PATCH] maint: accommodate LP64 systems

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 24311 in the body.
You can then email your comments to 24311 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Peter Rosin <peda <at> lysator.liu.se>
To: bug-diffutils <at> gnu.org
Cc: Peter Rosin <peda <at> lysator.liu.se>
Subject: [PATCH] maint: accommodate LP64 systems
Date: Fri, 26 Aug 2016 10:41:24 +0200

It is wrong to assume that pointers fit in long int.

* src/context.c (print_context_number_range): Use intptr_t and the
%PRIdPTR format when printing the lin type.
(print_unidiff_number_rande): Likewise.
* src/diff.h (translate_range): Likewise.
* src/diff3.c (output_diff3): Likewise.
(undotlines): Likewise.
(output_diff3_edscript): Likewise.
* src/ed.c (print_rcs_hunk): Likewise.
* src/side.c (print_sdiff_common_lines): Likewise.
(print_sdiff_hunk): Likewise.
* src/system.h: Likewise.
* src/ifdef.c (do_printf_spec): Likewise, but also make use of the
formats PRIoPTR, PRIxPTR and PRIXPTR.
---
 src/context.c | 12 ++++++------
 src/diff.h    |  2 +-
 src/diff3.c   | 37 +++++++++++++++++++------------------
 src/ed.c      |  8 +++++---
 src/ifdef.c   | 30 +++++++++++++++++++++++-------
 src/sdiff.c   | 16 ++++++++--------
 src/side.c    | 12 ++++++------
 src/system.h  |  2 +-
 src/util.c    | 23 ++++++++++++-----------
 9 files changed, 81 insertions(+), 61 deletions(-)

diff --git a/src/context.c b/src/context.c
index 1a92a60..4fcf0b8 100644
--- a/src/context.c
+++ b/src/context.c
@@ -126,7 +126,7 @@ print_context_script (struct change *script, bool unidiff)
 static void
 print_context_number_range (struct file_data const *file, lin a, lin b)
 {
-  long int trans_a, trans_b;
+  intptr_t trans_a, trans_b;
   translate_range (file, a, b, &trans_a, &trans_b);
 
   /* We can have B <= A in the case of a range of no lines.
@@ -139,9 +139,9 @@ print_context_number_range (struct file_data const *file, lin a, lin b)
      specification.  */
 
   if (trans_b <= trans_a)
-    fprintf (outfile, "%ld", trans_b);
+    fprintf (outfile, "%"PRIdPTR, trans_b);
   else
-    fprintf (outfile, "%ld,%ld", trans_a, trans_b);
+    fprintf (outfile, "%"PRIdPTR",%"PRIdPTR, trans_a, trans_b);
 }
 
 /* Print FUNCTION in a context header.  */
@@ -299,7 +299,7 @@ pr_context_hunk (struct change *hunk)
 static void
 print_unidiff_number_range (struct file_data const *file, lin a, lin b)
 {
-  long int trans_a, trans_b;
+  intptr_t trans_a, trans_b;
   translate_range (file, a, b, &trans_a, &trans_b);
 
   /* We can have B < A in the case of a range of no lines.
@@ -307,9 +307,9 @@ print_unidiff_number_range (struct file_data const *file, lin a, lin b)
      which is B.  It would be more logical to print A, but
      'patch' expects B in order to detect diffs against empty files.  */
   if (trans_b <= trans_a)
-    fprintf (outfile, trans_b < trans_a ? "%ld,0" : "%ld", trans_b);
+    fprintf (outfile, trans_b < trans_a ? "%"PRIdPTR",0" : "%"PRIdPTR, trans_b);
   else
-    fprintf (outfile, "%ld,%ld", trans_a, trans_b - trans_a + 1);
+    fprintf (outfile, "%"PRIdPTR",%"PRIdPTR, trans_a, trans_b - trans_a + 1);
 }
 
 /* Print a portion of an edit script in unidiff format.
diff --git a/src/diff.h b/src/diff.h
index 0983e7c..c4b236d 100644
--- a/src/diff.h
+++ b/src/diff.h
@@ -406,7 +406,7 @@ extern void print_script (struct change *, struct change * (*) (struct change *)
                           void (*) (struct change *));
 extern void setup_output (char const *, char const *, bool);
 extern void translate_range (struct file_data const *, lin, lin,
-                             long int *, long int *);
+                             intptr_t *, intptr_t *);
 
 enum color_context
 {
diff --git a/src/diff3.c b/src/diff3.c
index b80aeb3..eb4f729 100644
--- a/src/diff3.c
+++ b/src/diff3.c
@@ -1428,20 +1428,20 @@ output_diff3 (FILE *outputfile, struct diff3_block *diff,
 	  int realfile = mapping[i];
 	  lin lowt = D_LOWLINE (ptr, realfile);
 	  lin hight = D_HIGHLINE (ptr, realfile);
-	  long int llowt = lowt;
-	  long int lhight = hight;
+	  intptr_t llowt = lowt;
+	  intptr_t lhight = hight;
 
 	  fprintf (outputfile, "%d:", i + 1);
 	  switch (lowt - hight)
 	    {
 	    case 1:
-	      fprintf (outputfile, "%lda\n", llowt - 1);
+	      fprintf (outputfile, "%"PRIdPTR"a\n", llowt - 1);
 	      break;
 	    case 0:
-	      fprintf (outputfile, "%ldc\n", llowt);
+	      fprintf (outputfile, "%"PRIdPTR"c\n", llowt);
 	      break;
 	    default:
-	      fprintf (outputfile, "%ld,%ldc\n", llowt, lhight);
+	      fprintf (outputfile, "%"PRIdPTR",%"PRIdPTR"c\n", llowt, lhight);
 	      break;
 	    }
 
@@ -1495,19 +1495,20 @@ dotlines (FILE *outputfile, struct diff3_block *b, int filenum)
 
 /* Output to OUTPUTFILE a '.' line.  If LEADING_DOT is true, also
    output a command that removes initial '.'s starting with line START
-   and continuing for NUM lines.  (START is long int, not lin, for
-   convenience with printf %ld formats.)  */
+   and continuing for NUM lines.  (START is intptr_t, not lin, for
+   convenience with printf formats.)  */
 
 static void
-undotlines (FILE *outputfile, bool leading_dot, long int start, lin num)
+undotlines (FILE *outputfile, bool leading_dot, intptr_t start, lin num)
 {
   fputs (".\n", outputfile);
   if (leading_dot)
     {
       if (num == 1)
-	fprintf (outputfile, "%lds/^\\.//\n", start);
+        fprintf (outputfile, "%"PRIdPTR"s/^\\.//\n", start);
       else
-	fprintf (outputfile, "%ld,%lds/^\\.//\n", start, start + num - 1);
+        fprintf (outputfile, "%"PRIdPTR",%"PRIdPTR"s/^\\.//\n",
+                 start, start + num - 1);
     }
 }
 
@@ -1548,7 +1549,7 @@ output_diff3_edscript (FILE *outputfile, struct diff3_block *diff,
 	   ? DIFF_ALL
 	   : DIFF_1ST + rev_mapping[b->correspond - DIFF_1ST]);
 
-      long int low0, high0;
+      intptr_t low0, high0;
 
       /* If we aren't supposed to do this output block, skip it.  */
       switch (type)
@@ -1569,7 +1570,7 @@ output_diff3_edscript (FILE *outputfile, struct diff3_block *diff,
 
 	  /* Mark end of conflict.  */
 
-	  fprintf (outputfile, "%lda\n", high0);
+	  fprintf (outputfile, "%"PRIdPTR"a\n", high0);
 	  leading_dot = false;
 	  if (type == DIFF_ALL)
 	    {
@@ -1591,7 +1592,7 @@ output_diff3_edscript (FILE *outputfile, struct diff3_block *diff,
 
 	  /* Mark start of conflict.  */
 
-	  fprintf (outputfile, "%lda\n<<<<<<< %s\n", low0 - 1,
+	  fprintf (outputfile, "%"PRIdPTR"a\n<<<<<<< %s\n", low0 - 1,
 		   type == DIFF_ALL ? file0 : file1);
 	  leading_dot = false;
 	  if (type == DIFF_2ND)
@@ -1607,9 +1608,9 @@ output_diff3_edscript (FILE *outputfile, struct diff3_block *diff,
 	/* Write out a delete */
 	{
 	  if (low0 == high0)
-	    fprintf (outputfile, "%ldd\n", low0);
+	    fprintf (outputfile, "%"PRIdPTR"d\n", low0);
 	  else
-	    fprintf (outputfile, "%ld,%ldd\n", low0, high0);
+	    fprintf (outputfile, "%"PRIdPTR",%"PRIdPTR"d\n", low0, high0);
 	}
       else
 	/* Write out an add or change */
@@ -1617,13 +1618,13 @@ output_diff3_edscript (FILE *outputfile, struct diff3_block *diff,
 	  switch (high0 - low0)
 	    {
 	    case -1:
-	      fprintf (outputfile, "%lda\n", high0);
+	      fprintf (outputfile, "%"PRIdPTR"a\n", high0);
 	      break;
 	    case 0:
-	      fprintf (outputfile, "%ldc\n", high0);
+	      fprintf (outputfile, "%"PRIdPTR"c\n", high0);
 	      break;
 	    default:
-	      fprintf (outputfile, "%ld,%ldc\n", low0, high0);
+	      fprintf (outputfile, "%"PRIdPTR",%"PRIdPTR"c\n", low0, high0);
 	      break;
 	    }
 
diff --git a/src/ed.c b/src/ed.c
index 1fae2b8..14f9a43 100644
--- a/src/ed.c
+++ b/src/ed.c
@@ -144,7 +144,7 @@ static void
 print_rcs_hunk (struct change *hunk)
 {
   lin i, f0, l0, f1, l1;
-  long int tf0, tl0, tf1, tl1;
+  intptr_t tf0, tl0, tf1, tl1;
 
   /* Determine range of line numbers involved in each file.  */
   enum changes changes = analyze_hunk (hunk, &f0, &l0, &f1, &l1);
@@ -159,14 +159,16 @@ print_rcs_hunk (struct change *hunk)
     {
       /* For deletion, print just the starting line number from file 0
 	 and the number of lines deleted.  */
-      fprintf (outfile, "d%ld %ld\n", tf0, tf0 <= tl0 ? tl0 - tf0 + 1 : 1);
+      fprintf (outfile, "d%"PRIdPTR" %"PRIdPTR"\n",
+               tf0, tf0 <= tl0 ? tl0 - tf0 + 1 : 1);
     }
 
   if (changes & NEW)
     {
       /* Take last-line-number from file 0 and # lines from file 1.  */
       translate_range (&files[1], f1, l1, &tf1, &tl1);
-      fprintf (outfile, "a%ld %ld\n", tl0, tf1 <= tl1 ? tl1 - tf1 + 1 : 1);
+      fprintf (outfile, "a%"PRIdPTR" %"PRIdPTR"\n",
+               tl0, tf1 <= tl1 ? tl1 - tf1 + 1 : 1);
 
       /* Print the inserted lines.  */
       for (i = f1; i <= l1; i++)
diff --git a/src/ifdef.c b/src/ifdef.c
index b8b084f..4a38404 100644
--- a/src/ifdef.c
+++ b/src/ifdef.c
@@ -358,19 +358,35 @@ do_printf_spec (FILE *out, char const *spec,
 	if (out)
 	  {
 	    /* For example, if the spec is "%3xn", use the printf
-	       format spec "%3lx".  Here the spec prefix is "%3".  */
-	    long int long_value = value;
+	       format spec "%3"PRIxPTR.  Here the spec prefix is "%3".  */
+	    intptr_t long_value = value;
 	    size_t spec_prefix_len = f - spec - 2;
+	    size_t max_format = MAX(strlen(PRIdPTR),
+	                        MAX(strlen(PRIoPTR),
+	                        MAX(strlen(PRIxPTR),
+	                            strlen(PRIXPTR))));
 #if HAVE_C_VARARRAYS
-	    char format[spec_prefix_len + 3];
+	    char format[spec_prefix_len + max_format + 1];
 #else
-	    char *format = xmalloc (spec_prefix_len + 3);
+	    char *format = xmalloc (spec_prefix_len + max_format + 1);
 #endif
 	    char *p = format + spec_prefix_len;
 	    memcpy (format, spec, spec_prefix_len);
-	    *p++ = 'l';
-	    *p++ = c;
-	    *p = '\0';
+	    switch (c)
+	      {
+	        case 'd':
+	          strcpy(p, PRIdPTR);
+	          break;
+	        case 'o':
+	          strcpy(p, PRIoPTR);
+	          break;
+	        case 'x':
+	          strcpy(p, PRIxPTR);
+	          break;
+	        case 'X':
+	          strcpy(p, PRIXPTR);
+	          break;
+	      }
 	    fprintf (out, format, long_value);
 #if ! HAVE_C_VARARRAYS
 	    free (format);
diff --git a/src/sdiff.c b/src/sdiff.c
index 22d6e5b..ed5b246 100644
--- a/src/sdiff.c
+++ b/src/sdiff.c
@@ -968,11 +968,11 @@ edit (struct line_filter *left, char const *lname, lin lline, lin llen,
 		if (llen)
 		  {
 		    if (llen == 1)
-		      fprintf (tmp, "--- %s %ld\n", lname, (long int) lline);
+		      fprintf (tmp, "--- %s %"PRIdPTR"\n", lname, (intptr_t) lline);
 		    else
-		      fprintf (tmp, "--- %s %ld,%ld\n", lname,
-			       (long int) lline,
-			       (long int) (lline + llen - 1));
+		      fprintf (tmp, "--- %s %"PRIdPTR",%"PRIdPTR"\n", lname,
+			       (intptr_t) lline,
+			       (intptr_t) (lline + llen - 1));
 		  }
 		/* Fall through.  */
 	      case '1': case 'b': case 'l':
@@ -990,11 +990,11 @@ edit (struct line_filter *left, char const *lname, lin lline, lin llen,
 		if (rlen)
 		  {
 		    if (rlen == 1)
-		      fprintf (tmp, "+++ %s %ld\n", rname, (long int) rline);
+		      fprintf (tmp, "+++ %s %"PRIdPTR"\n", rname, (intptr_t) rline);
 		    else
-		      fprintf (tmp, "+++ %s %ld,%ld\n", rname,
-			       (long int) rline,
-			       (long int) (rline + rlen - 1));
+		      fprintf (tmp, "+++ %s %"PRIdPTR",%"PRIdPTR"\n", rname,
+			       (intptr_t) rline,
+			       (intptr_t) (rline + rlen - 1));
 		  }
 		/* Fall through.  */
 	      case '2': case 'b': case 'r':
diff --git a/src/side.c b/src/side.c
index 2276385..f5f1027 100644
--- a/src/side.c
+++ b/src/side.c
@@ -260,9 +260,9 @@ print_sdiff_common_lines (lin limit0, lin limit1)
     {
       if (sdiff_merge_assist)
 	{
-	  long int len0 = limit0 - i0;
-	  long int len1 = limit1 - i1;
-	  fprintf (outfile, "i%ld,%ld\n", len0, len1);
+	  intptr_t len0 = limit0 - i0;
+	  intptr_t len1 = limit1 - i1;
+	  fprintf (outfile, "i%"PRIdPTR",%"PRIdPTR"\n", len0, len1);
 	}
 
       if (!left_column)
@@ -302,9 +302,9 @@ print_sdiff_hunk (struct change *hunk)
 
   if (sdiff_merge_assist)
     {
-      long int len0 = last0 - first0 + 1;
-      long int len1 = last1 - first1 + 1;
-      fprintf (outfile, "c%ld,%ld\n", len0, len1);
+      intptr_t len0 = last0 - first0 + 1;
+      intptr_t len1 = last1 - first1 + 1;
+      fprintf (outfile, "c%"PRIdPTR",%"PRIdPTR"\n", len0, len1);
     }
 
   /* Print "xxx  |  xxx " lines.  */
diff --git a/src/system.h b/src/system.h
index be1c0bd..70b8678 100644
--- a/src/system.h
+++ b/src/system.h
@@ -134,7 +134,7 @@ typedef ptrdiff_t lin;
 #define LIN_MAX PTRDIFF_MAX
 verify (TYPE_SIGNED (lin));
 verify (sizeof (ptrdiff_t) <= sizeof (lin));
-verify (sizeof (lin) <= sizeof (long int));
+verify (sizeof (lin) <= sizeof (intptr_t));
 
 /* Limit so that 2 * CONTEXT + 1 does not overflow.  */
 
diff --git a/src/util.c b/src/util.c
index 76872cb..f4e24e4 100644
--- a/src/util.c
+++ b/src/util.c
@@ -1401,13 +1401,13 @@ translate_line_number (struct file_data const *file, lin i)
 }
 
 /* Translate a line number range.  This is always done for printing,
-   so for convenience translate to long int rather than lin, so that the
-   caller can use printf with "%ld" without casting.  */
+   so for convenience translate to intptr_t rather than lin, so that the
+   caller can use printf without casting.  */
 
 void
 translate_range (struct file_data const *file,
 		 lin a, lin b,
-		 long int *aptr, long int *bptr)
+		 intptr_t *aptr, intptr_t *bptr)
 {
   *aptr = translate_line_number (file, a - 1) + 1;
   *bptr = translate_line_number (file, b + 1) - 1;
@@ -1422,16 +1422,16 @@ translate_range (struct file_data const *file,
 void
 print_number_range (char sepchar, struct file_data *file, lin a, lin b)
 {
-  long int trans_a, trans_b;
+  intptr_t trans_a, trans_b;
   translate_range (file, a, b, &trans_a, &trans_b);
 
   /* Note: we can have B < A in the case of a range of no lines.
      In this case, we should print the line number before the range,
      which is B.  */
   if (trans_b > trans_a)
-    fprintf (outfile, "%ld%c%ld", trans_a, sepchar, trans_b);
+    fprintf (outfile, "%"PRIdPTR"%c%"PRIdPTR, trans_a, sepchar, trans_b);
   else
-    fprintf (outfile, "%ld", trans_b);
+    fprintf (outfile, "%"PRIdPTR, trans_b);
 }
 
 /* Look at a hunk of edit script and report the range of lines in each file
@@ -1565,11 +1565,12 @@ debug_script (struct change *sp)
 
   for (; sp; sp = sp->link)
     {
-      long int line0 = sp->line0;
-      long int line1 = sp->line1;
-      long int deleted = sp->deleted;
-      long int inserted = sp->inserted;
-      fprintf (stderr, "%3ld %3ld delete %ld insert %ld\n",
+      intptr_t line0 = sp->line0;
+      intptr_t line1 = sp->line1;
+      intptr_t deleted = sp->deleted;
+      intptr_t inserted = sp->inserted;
+      fprintf (stderr, "%3"PRIdPTR" %3"PRIdPTR" "
+	       "delete %"PRIdPTR" insert %"PRIdPTR"\n",
 	       line0, line1, deleted, inserted);
     }
 
-- 
2.8.3

Message #8 received at 24311 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Peter Rosin <peda <at> lysator.liu.se>, 24311 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: [PATCH] maint: accommodate LP64 systems
Date: Fri, 26 Aug 2016 11:42:42 -0700

On 08/26/2016 01:41 AM, Peter Rosin wrote:
> It is wrong to assume that pointers fit in long int.

Not in GNU code. 
<https://www.gnu.org/prep/standards/html_node/CPU-Portability.html says 
that GNU programs should not make any effort to cater to the possibility 
that 'long' will be narrower than ptrdiff_t.

(It should be safe to convert ptrdiff_t to long in an LP64 system, so I 
assume you meant to write "LLP64" in the Subject: line. Also, the values 
in questions are offsets, not pointers, so I assume you meant to write 
"pointer differences" not "pointers" in the above-quoted sentence.)

A problem with the proposed patch is that it uses PRIdPTR, and thus 
won't work on platforms with older C libraries that lack support for 
PRIdPTR. Although this can be worked around with some effort, as a 
general rule we prefer to avoid the effort.

Every POSIX platform is supposed to provide a compilation environment 
where 'long' is no wider than ptrdiff_t, so if you're using a POSIX 
platform you should be able to build diffutils without modifying its 
source code. See:

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/c99.html

Message #11 received at 24311 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Peter Rosin <peda <at> lysator.liu.se>,
 24311 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: bug#24311: [PATCH] maint: accommodate
 LP64 systems
Date: Fri, 26 Aug 2016 14:40:57 -0500

[Message part 1 (text/plain, inline)]

On 08/26/2016 01:42 PM, Paul Eggert wrote:
> On 08/26/2016 01:41 AM, Peter Rosin wrote:
>> It is wrong to assume that pointers fit in long int.
> 
> Not in GNU code.
> <https://www.gnu.org/prep/standards/html_node/CPU-Portability.html says
> that GNU programs should not make any effort to cater to the possibility
> that 'long' will be narrower than ptrdiff_t.
> 

Except that the mingw64 platform is indeed a modern platform where
pointers are 8 bytes but long is 4 bytes.  So maybe it is time to
propose a patch to the GNU Coding Standards that document the reality of
existing systems, and then start the process of auditing which programs
have been relying on the assumptions now rendered untrue by catering to
such a platform.

> Every POSIX platform is supposed to provide a compilation environment
> where 'long' is no wider than ptrdiff_t, so if you're using a POSIX
> platform you should be able to build diffutils without modifying its
> source code. See:
> 
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/c99.html
> 

But GNU software often tries to be portable to more than just POSIX
platforms, and gnulib is already useful for catering to mingw64 where we
indeed have long narrower than intptr_t (I'm not sure off-hand whether
intptr_t and ptrdiff_t are the same size, or if ptrdiff_t is only 4 bytes).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #14 received at 24311 <at> debbugs.gnu.org (full text, mbox):

From: Peter Rosin <peda <at> lysator.liu.se>
To: Paul Eggert <eggert <at> cs.ucla.edu>, 24311 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: [PATCH] maint: accommodate LP64 systems
Date: Fri, 26 Aug 2016 23:59:44 +0200

On 2016-08-26 20:42, Paul Eggert wrote:
> On 08/26/2016 01:41 AM, Peter Rosin wrote:
>> It is wrong to assume that pointers fit in long int.
> 
> Not in GNU code. 

Then GNU has abandoned portability. Sad.

> <https://www.gnu.org/prep/standards/html_node/CPU-Portability.html says 
> that GNU programs should not make any effort to cater to the possibility 
> that 'long' will be narrower than ptrdiff_t.

Feels designed to target 64-bit Windows. Sad.

> (It should be safe to convert ptrdiff_t to long in an LP64 system, so I 
> assume you meant to write "LLP64" in the Subject: line. Also, the values 
> in questions are offsets, not pointers, so I assume you meant to write 
> "pointer differences" not "pointers" in the above-quoted sentence.)

Yes, I fat-fingered the subject and was a bit careless, sorry about
that. Feel free to reword if you end up taking the patch.

> A problem with the proposed patch is that it uses PRIdPTR, and thus 
> won't work on platforms with older C libraries that lack support for 
> PRIdPTR. Although this can be worked around with some effort, as a 
> general rule we prefer to avoid the effort.
> 
> Every POSIX platform is supposed to provide a compilation environment 
> where 'long' is no wider than ptrdiff_t, so if you're using a POSIX 
> platform you should be able to build diffutils without modifying its 
> source code. See:
> 
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/c99.html

I find these two complaints a little bit contradicting. Do you, or do
you not require c99? Because the POSIX part seems to be connected to
c99, but what do I know?

Besides, I thought the gnulib inttypes module (or whatever it's called)
provided the PRI?PTR macros if missing/broken?

Cheers,
Peter

Message #17 received at 24311 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Peter Rosin <peda <at> lysator.liu.se>, 24311 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: [PATCH] maint: accommodate LP64 systems
Date: Fri, 26 Aug 2016 15:24:02 -0700

On 08/26/2016 02:59 PM, Peter Rosin wrote:
> Feels designed to target 64-bit Windows.

It isn't. It's more the other way round. The Unix (and GNU) LP64 
tradition came first, and the 64-bit MS-Windows designers made it hard 
to write 64-bit code that is portable among the platforms.

> Do you, or do you not require c99?

It's not that simple. Although many GNU apps (including diffutils, I 
expect) assume some C99 features, they are typically portable to 
less-than-perfect POSIXish implementations that don't have all features 
that C99 requires.

The main sticking point in this particular case is support for printf 
formats like %td. This requires library support, which in practice 
trails behind compiler support. We can reasonably ask people to use a 
C99ish compiler even when running on older platforms (just install GCC). 
It hasn't been so easy to insist on a C99ish C library.

> the POSIX part seems to be connected to c99, but what do I know?

POSIX assumes C99 or later nowadays.

> Besides, I thought the gnulib inttypes module (or whatever it's called)
> provided the PRI?PTR macros if missing/broken?

PRI?PTR are not the right macros to use here. The values in question are 
of type ptrdiff_t, and ptrdiff_t != intptr_t on some (admittedly 
unusual) platforms.

Message #22 received at 24311-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Peter Rosin <peda <at> lysator.liu.se>, 24311-done <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: bug#24311: [PATCH] maint: accommodate
 LP64 systems
Date: Sat, 27 Aug 2016 15:07:30 -0700

[Message part 1 (text/plain, inline)]

On further thought, I came up with a patch that should be portable without 
overly complicating the code or hurting efficiency, so I installed it 
(attached). Please give it a try on your platform.

I am still leery about changing the GNU coding standards, though. In general we 
shouldn't ask developers to jump through these sorts of hoops for platforms so 
far from the typical GNU environment.

[0001-diff-port-line-numbers-to-mingw64.patch (text/x-diff, attachment)]

Message #25 received at 24311 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Grünbacher <agruen <at> gnu.org>
To: 24311 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>,
 peda <at> lysator.liu.se
Cc: 24311-done <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: bug#24311: bug#24311: [PATCH] maint:
 accommodate LP64 systems
Date: Sun, 28 Aug 2016 00:39:34 +0200

2016-08-28 0:07 GMT+02:00 Paul Eggert <eggert <at> cs.ucla.edu>:
> On further thought, I came up with a patch that should be portable without
> overly complicating the code or hurting efficiency, so I installed it
> (attached). Please give it a try on your platform.

FWIW, patch has the following typedef for line numbers:

  typedef off_t lin;

Maybe it would be time to switch to int64_t / uint64_t and PRId64 /
PRIu64 in diff as well as in patch?

Andreas

Message #31 received at 24311 <at> debbugs.gnu.org (full text, mbox):

From: Peter Rosin <peda <at> lysator.liu.se>
To: Paul Eggert <eggert <at> cs.ucla.edu>, 24311 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: [PATCH] maint: accommodate LP64 systems
Date: Sun, 28 Aug 2016 00:49:06 +0200

On 2016-08-27 00:24, Paul Eggert wrote:
> The main sticking point in this particular case is support for printf 
> formats like %td. This requires library support, which in practice 
> trails behind compiler support. We can reasonably ask people to use a 
> C99ish compiler even when running on older platforms (just install GCC). 
> It hasn't been so easy to insist on a C99ish C library.

*snip*

> PRI?PTR are not the right macros to use here. The values in question are 
> of type ptrdiff_t, and ptrdiff_t != intptr_t on some (admittedly 
> unusual) platforms.

Are you trying to argue that there exist some non-theoretical system,
where it's useful to actually run diffutils, and where both

    intptr_t < long int           and
    intptr_t < ptrdiff_t

holds?     (i.e. a system that would regress with the patch)

Because if there is no such system, then PRIdPTR w/ intptr_t will work
better than %ld w/ long int (pending the time when %td w/ ptrdiff_t is
generally available).

Cheers,
Peter

Message #34 received at 24311 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Andreas Grünbacher <agruen <at> gnu.org>,
 24311 <at> debbugs.gnu.org, peda <at> lysator.liu.se
Cc: 24311-done <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: bug#24311: bug#24311: [PATCH] maint:
 accommodate LP64 systems
Date: Sat, 27 Aug 2016 16:09:19 -0700

Andreas Grünbacher wrote:
> FWIW, patch has the following typedef for line numbers:
>
>   typedef off_t lin;
>
> Maybe it would be time to switch to int64_t / uint64_t and PRId64 /
> PRIu64 in diff as well as in patch?

No, as 'diff' puts all its data into memory (hence ptrdiff_t is appropriate), 
whereas 'patch' can operate on files that do not fit into memory (hence off_t is 
appropriate).

Generally speaking, portable code should be wary of using exact-sized types like 
int64_t. POSIX and the C standard don't even require int64_t, and there are a 
few (admittedly rare) implementations that lack int64_t. Even if int64_t was 
universally available, it would be better to use types like ptrdiff_t and off_t 
that reflect actual implementation limitations, as that better documents the code.

Message #40 received at 24311 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Peter Rosin <peda <at> lysator.liu.se>, 24311 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: [PATCH] maint: accommodate LP64 systems
Date: Sat, 27 Aug 2016 16:10:22 -0700

Peter Rosin wrote:
> Are you trying to argue

No, I'm trying to solve the problem. If it's just as easy to solve the problem 
even on oddball platforms, then we might as well solve it in a portable way. 
(After all, that's the main justification for the patch I just installed, where 
the "oddball platform" in this case is Mingw-w64. :-)

> (pending the time when %td w/ ptrdiff_t is generally available)

If "generally available" means "even Microsoft supports it", then I wouldn't 
hold my breath. Although %td has been part of the C standard for 17 years, 
MS-Windows still does not support it.

Message #43 received at 24311-done <at> debbugs.gnu.org (full text, mbox):

From: Peter Rosin <peda <at> lysator.liu.se>
To: Paul Eggert <eggert <at> cs.ucla.edu>, 24311-done <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: bug#24311: [PATCH] maint: accommodate
 LP64 systems
Date: Sun, 28 Aug 2016 22:03:30 +0200

On 2016-08-28 00:07, Paul Eggert wrote:
> On further thought, I came up with a patch that should be portable without 
> overly complicating the code or hurting efficiency, so I installed it 
> (attached). Please give it a try on your platform.

So, I didn't expect that to work, because the C library in question is the
system msvcrt, which is compatible with the msvcrt for MSVC 6.0. That msvcrt
was released almost 30 years ago. I have a strong suspicion that all system
versions of the msvcrt library out there do not support %lld, even if you
limit the search to 64-bit Windows...

All in all, I was a bit surprised when it actually did work, and started
digging.

It seems that mingw-w64 includes overrides for the *printf functions, probably
because they are so unportable in msvcrt. Anyway, the overrides includes support
for the t length modifier, so a better patch might be a configure check to see
if %td works, with machinery to set printint/pI to ptrdiff_t/"t" if it does and
fall back to long int/"l" if it doesn't.

That would be correct even for weird -- but standards conformant -- systems
where even long long is too small for some obscure reason.

> I am still leery about changing the GNU coding standards, though. In general we 
> shouldn't ask developers to jump through these sorts of hoops for platforms so 
> far from the typical GNU environment.

My main beef with the GCS on this issue is that it suggests that maintainers
should not spend any time at all on this issue, as if that would be contrary
to some other goal. It would be much more palatable if it stated that
maintainers did not need to spend time on the issue if they didn't want to.

Cheers,
Peter

Message #46 received at 24311-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Peter Rosin <peda <at> lysator.liu.se>, 24311-done <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: bug#24311: [PATCH] maint: accommodate
 LP64 systems
Date: Sun, 28 Aug 2016 16:55:43 -0700

[Message part 1 (text/plain, inline)]

Peter Rosin wrote:

> better patch might be a configure check to see
> if %td works, with machinery to set printint/pI to ptrdiff_t/"t" if it does and
> fall back to long int/"l" if it doesn't.

That'd be more work to develop and would be more likely to go wrong and would 
slow down 'configure'. I installed the attached patch instead.

> My main beef with the GCS on this issue is that it suggests that maintainers
> should not spend any time at all on this issue, as if that would be contrary
> to some other goal.

It sucks away resources that could be used to support more-important goals. 
We've already spent more time on Bug#24311 than the problem was worth. 
Maintainers should not expend any significant time going down this rabbit hole.

[0001-diff-don-t-assume-ptrdiff_t-long-long-int.patch (text/x-diff, attachment)]

Message #49 received at 24311 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Claude <lamy.claude <at> gmail.com>
Cc: 24311 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: bug#24311: bug#24311: [PATCH] maint:
 accommodate LP64 systems
Date: Tue, 30 Aug 2016 07:41:50 -0700

Claude wrote:

> xvasprintf.c: In function 'xvasprintf':
> xvasprintf.c:102:3: error: function might be possible candidate for 'gnu_printf'
> format attribute [-Werror=suggest-attribute=format]
>    if (vasprintf (&result, format, args) < 0)
>    ^

I wouldn't worry about that warning. Either configure without 
--with-gcc-warnings or build with 'make WERROR_CFLAGS='.

> when compiling with cygwin
>
>   CC       xstrtol.o
> In file included from xstrtol.h:22:0,
>                  from xstrtol.c:31:
> ./getopt.h:198:8: error: redefinition of 'struct option'
>  struct option
>         ^

Sounds like your Cygwin configuration is busted somehow. Look in config.log and 
see why 'configure' decided that getopt didn't work on your platform.

> Aren't these tests on $cross_compiling contradictory in configure ?
>
>
>                                                 if test $cross_compiling = no; then
>                               if test "$cross_compiling" = yes; then :
>   { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
> $as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
> as_fn_error $? "cannot run test program while cross compiling
> See \`config.log' for more details" "$LINENO" 5; }
> else
>   cat confdefs.h - <<_ACEOF >conftest.$ac_ext

I don't see any contradiction.

Message #52 received at 24311 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Claude <lamy.claude <at> gmail.com>
Cc: 24311 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#24311: bug#24311: bug#24311: [PATCH] maint:
 accommodate LP64 systems
Date: Tue, 30 Aug 2016 09:45:52 -0700

Claude wrote:

> - the cross_compiling thing... If I read this right, and I may be wrong, the
> first two line of my snapshot stipulate that cross_compiling have to be set to
> no AND yes at the same time to enter the block.  That could never happen.

Ah, I see it now. Well, 'configure' scripts are notoriously inefficient....

> - cygwin. I didn't see anything that went wrong.  I am attaching the config.log

Nothing was attached. But really, this is something you need to debug; Cygwin is 
low priority for me. All I'm saying is that Cygwin should already have getopt so 
why are you compiling getopt? You need to figure it out by looking at config.log 
etc.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.