[PATCH 00/12] printf() fixes

[PATCH 00/12] printf() fixes

[Alejandro Colomar]

Hello Michael,

Here are some patches related to printf().

Cheers,

Alex

Alejandro Colomar (12):
  dl_iterate_phdr.3: Cast 'ElfN_Word' to 'uintmax_t' for printf()
  malloc_hook.3: Remove unneeded cast, and print 'size_t' with "%zu"
  pthread_getcpuclockid.3: Cast 'time_t' to 'intmax_t' for printf()
  getgrent_r.3: Cast 'gid_t' to 'intmax_t' for printf()
  getpwent_r.3: Cast 'uid_t' to 'intmax_t' for printf()
  rtld-audit.7: Use "%u" rather than "%d" when printing 'unsigned int'
    values
  userfaultfd.2: Use 'PRIx64' rather than "%llx" when printing 64-bit
    fixed-width types
  clock_getres.2: Cast 'time_t' to 'intmax_t' for printf() and fix the
    length modifiers
  open_by_handle_at.2: Use "%u" rather than "%d" when printing 'unsigned
    int' values
  spu_run.2: Cast 'int' to 'unsigned int' when printing with "%x"
  mprotect.2: Use "%p" rather than casting to 'long' when printing
    adresses (pointer values)
  pidfd_open.2: Cast 'short' to 'unsigned int' when printing with "%x"

 man2/clock_getres.2          | 7 +++++--
 man2/mprotect.2              | 5 ++---
 man2/open_by_handle_at.2     | 2 +-
 man2/pidfd_open.2            | 3 ++-
 man2/spu_run.2               | 2 +-
 man2/userfaultfd.2           | 7 ++++---
 man3/dl_iterate_phdr.3       | 7 ++++---
 man3/getgrent_r.3            | 3 ++-
 man3/getpwent_r.3            | 5 +++--
 man3/malloc_hook.3           | 4 ++--
 man3/pthread_getcpuclockid.3 | 3 ++-
 man7/rtld-audit.7            | 6 +++---
 12 files changed, 31 insertions(+), 23 deletions(-)

-- 
2.28.0

[PATCH 01/12] dl_iterate_phdr.3: Cast 'ElfN_Word' to 'uintmax_t' for printf()

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man3/dl_iterate_phdr.3 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/man3/dl_iterate_phdr.3 b/man3/dl_iterate_phdr.3
index 70206a0ba..a8a85d8e6 100644
--- a/man3/dl_iterate_phdr.3
+++ b/man3/dl_iterate_phdr.3
@@ -302,6 +302,7 @@ Name: "/lib64/ld-linux-x86-64.so.2" (7 segments)
 #include <link.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <stdint.h>
 
 static int
 callback(struct dl_phdr_info *info, size_t size, void *data)
@@ -325,10 +326,10 @@ callback(struct dl_phdr_info *info, size_t size, void *data)
                 (p_type == PT_GNU_STACK) ? "PT_GNU_STACK" :
                 (p_type == PT_GNU_RELRO) ? "PT_GNU_RELRO" : NULL;
 
-        printf("    %2d: [%14p; memsz:%7lx] flags: %#x; ", j,
+        printf("    %2d: [%14p; memsz:%7jx] flags: %#jx; ", j,
                 (void *) (info\->dlpi_addr + info\->dlpi_phdr[j].p_vaddr),
-                info\->dlpi_phdr[j].p_memsz,
-                info\->dlpi_phdr[j].p_flags);
+                (uintmax_t) info\->dlpi_phdr[j].p_memsz,
+                (uintmax_t) info\->dlpi_phdr[j].p_flags);
         if (type != NULL)
             printf("%s\en", type);
         else
-- 
2.28.0

[PATCH 02/12] malloc_hook.3: Remove unneeded cast, and print 'size_t' with "%zu"

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man3/malloc_hook.3 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/man3/malloc_hook.3 b/man3/malloc_hook.3
index a509dc72f..fb45e30f2 100644
--- a/man3/malloc_hook.3
+++ b/man3/malloc_hook.3
@@ -130,8 +130,8 @@ my_malloc_hook(size_t size, const void *caller)
     old_malloc_hook = __malloc_hook;
 
     /* printf() might call malloc(), so protect it too. */
-    printf("malloc(%u) called from %p returns %p\en",
-            (unsigned int) size, caller, result);
+    printf("malloc(%zu) called from %p returns %p\en",
+            size, caller, result);
 
     /* Restore our own hooks */
     __malloc_hook = my_malloc_hook;
-- 
2.28.0

[PATCH 03/12] pthread_getcpuclockid.3: Cast 'time_t' to 'intmax_t' for printf()

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man3/pthread_getcpuclockid.3 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/man3/pthread_getcpuclockid.3 b/man3/pthread_getcpuclockid.3
index 703735b91..a457a4cee 100644
--- a/man3/pthread_getcpuclockid.3
+++ b/man3/pthread_getcpuclockid.3
@@ -115,6 +115,7 @@ Subthread CPU time:        0.992
 
 #include <time.h>
 #include <stdio.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <pthread.h>
@@ -143,7 +144,7 @@ pclock(char *msg, clockid_t cid)
     printf("%s", msg);
     if (clock_gettime(cid, &ts) == \-1)
         handle_error("clock_gettime");
-    printf("%4ld.%03ld\en", ts.tv_sec, ts.tv_nsec / 1000000);
+    printf("%4jd.%03ld\en", (intmax_t) ts.tv_sec, ts.tv_nsec / 1000000);
 }
 
 int
-- 
2.28.0

[PATCH 04/12] getgrent_r.3: Cast 'gid_t' to 'intmax_t' for printf()

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man3/getgrent_r.3 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/man3/getgrent_r.3 b/man3/getgrent_r.3
index 76deec370..8170cd417 100644
--- a/man3/getgrent_r.3
+++ b/man3/getgrent_r.3
@@ -174,6 +174,7 @@ in the stream with all other threads.
 #define _GNU_SOURCE
 #include <grp.h>
 #include <stdio.h>
+#include <stdint.h>
 #include <stdlib.h>
 #define BUFLEN 4096
 
@@ -189,7 +190,7 @@ main(void)
         i = getgrent_r(&grp, buf, sizeof(buf), &grpp);
         if (i)
             break;
-        printf("%s (%d):", grpp\->gr_name, grpp\->gr_gid);
+        printf("%s (%jd):", grpp\->gr_name, (intmax_t) grpp\->gr_gid);
         for (int j = 0; ; j++) {
             if (grpp\->gr_mem[j] == NULL)
                 break;
-- 
2.28.0

[PATCH 05/12] getpwent_r.3: Cast 'uid_t' to 'intmax_t' for printf()

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man3/getpwent_r.3 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/man3/getpwent_r.3 b/man3/getpwent_r.3
index b6c1c281f..ef4c01c21 100644
--- a/man3/getpwent_r.3
+++ b/man3/getpwent_r.3
@@ -179,6 +179,7 @@ in the stream with all other threads.
 #define _GNU_SOURCE
 #include <pwd.h>
 #include <stdio.h>
+#include <stdint.h>
 #define BUFLEN 4096
 
 int
@@ -194,8 +195,8 @@ main(void)
         i = getpwent_r(&pw, buf, sizeof(buf), &pwp);
         if (i)
             break;
-        printf("%s (%d)\etHOME %s\etSHELL %s\en", pwp\->pw_name,
-               pwp\->pw_uid, pwp\->pw_dir, pwp\->pw_shell);
+        printf("%s (%jd)\etHOME %s\etSHELL %s\en", pwp\->pw_name,
+               (intmax_t) pwp\->pw_uid, pwp\->pw_dir, pwp\->pw_shell);
     }
     endpwent();
     exit(EXIT_SUCCESS);
-- 
2.28.0

[PATCH 06/12] rtld-audit.7: Use "%u" rather than "%d" when printing 'unsigned int' values

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man7/rtld-audit.7 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/man7/rtld-audit.7 b/man7/rtld-audit.7
index f90731261..b1b7dfebc 100644
--- a/man7/rtld-audit.7
+++ b/man7/rtld-audit.7
@@ -508,7 +508,7 @@ This is reportedly fixed in glibc 2.10.
 unsigned int
 la_version(unsigned int version)
 {
-    printf("la_version(): %d\en", version);
+    printf("la_version(): %u\en", version);
 
     return version;
 }
@@ -572,7 +572,7 @@ la_symbind32(Elf32_Sym *sym, unsigned int ndx, uintptr_t *refcook,
 {
     printf("la_symbind32(): symname = %s; sym\->st_value = %p\en",
             symname, sym\->st_value);
-    printf("        ndx = %d; flags = %#x", ndx, *flags);
+    printf("        ndx = %u; flags = %#x", ndx, *flags);
     printf("; refcook = %p; defcook = %p\en", refcook, defcook);
 
     return sym\->st_value;
@@ -584,7 +584,7 @@ la_symbind64(Elf64_Sym *sym, unsigned int ndx, uintptr_t *refcook,
 {
     printf("la_symbind64(): symname = %s; sym\->st_value = %p\en",
             symname, sym\->st_value);
-    printf("        ndx = %d; flags = %#x", ndx, *flags);
+    printf("        ndx = %u; flags = %#x", ndx, *flags);
     printf("; refcook = %p; defcook = %p\en", refcook, defcook);
 
     return sym\->st_value;
-- 
2.28.0

[PATCH 07/12] userfaultfd.2: Use 'PRIx64' rather than "%llx" when printing 64-bit fixed-width types

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man2/userfaultfd.2 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/man2/userfaultfd.2 b/man2/userfaultfd.2
index 126d4a854..c7e42804a 100644
--- a/man2/userfaultfd.2
+++ b/man2/userfaultfd.2
@@ -566,6 +566,7 @@ Read address 0x7fd30106ec0f in main(): C
    Licensed under the GNU General Public License version 2 or later.
 */
 #define _GNU_SOURCE
+#include <inttypes.h>
 #include <sys/types.h>
 #include <stdio.h>
 #include <linux/userfaultfd.h>
@@ -650,8 +651,8 @@ fault_handler_thread(void *arg)
         /* Display info about the page\-fault event */
 
         printf("    UFFD_EVENT_PAGEFAULT event: ");
-        printf("flags = %llx; ", msg.arg.pagefault.flags);
-        printf("address = %llx\en", msg.arg.pagefault.address);
+        printf("flags = %"PRIx64"; ", msg.arg.pagefault.flags);
+        printf("address = %"PRIx64"\en", msg.arg.pagefault.address);
 
         /* Copy the page pointed to by \(aqpage\(aq into the faulting
            region. Vary the contents that are copied in, so that it
@@ -673,7 +674,7 @@ fault_handler_thread(void *arg)
         if (ioctl(uffd, UFFDIO_COPY, &uffdio_copy) == \-1)
             errExit("ioctl\-UFFDIO_COPY");
 
-        printf("        (uffdio_copy.copy returned %lld)\en",
+        printf("        (uffdio_copy.copy returned %"PRId64")\en",
                 uffdio_copy.copy);
     }
 }
-- 
2.28.0

[PATCH 08/12] clock_getres.2: Cast 'time_t' to 'intmax_t' for printf() and fix the length modifiers

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man2/clock_getres.2 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/man2/clock_getres.2 b/man2/clock_getres.2
index 8fc7c6fef..d95d88bb2 100644
--- a/man2/clock_getres.2
+++ b/man2/clock_getres.2
@@ -468,6 +468,7 @@ CLOCK_BOOTTIME :      72691.019 (20h 11m 31s)
 #define _XOPEN_SOURCE 600
 #include <time.h>
 #include <stdio.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <stdbool.h>
 #include <unistd.h>
@@ -491,8 +492,10 @@ displayClock(clockid_t clock, char *name, bool showRes)
     if (days > 0)
         printf("%ld days + ", days);
 
-    printf("%2ldh %2ldm %2lds", (ts.tv_sec % SECS_IN_DAY) / 3600,
-            (ts.tv_sec % 3600) / 60, ts.tv_sec % 60);
+    printf("%2jdh %2jdm %2jds",
+            (intmax_t) (ts.tv_sec % SECS_IN_DAY) / 3600,
+            (intmax_t) (ts.tv_sec % 3600) / 60,
+            (intmax_t) ts.tv_sec % 60);
     printf(")\en");
 
     if (clock_getres(clock, &ts) == \-1) {
-- 
2.28.0

[PATCH 09/12] open_by_handle_at.2: Use "%u" rather than "%d" when printing 'unsigned int' values

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man2/open_by_handle_at.2 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man2/open_by_handle_at.2 b/man2/open_by_handle_at.2
index fb8940696..53c8c7968 100644
--- a/man2/open_by_handle_at.2
+++ b/man2/open_by_handle_at.2
@@ -600,7 +600,7 @@ main(int argc, char *argv[])
        for later reuse by t_open_by_handle_at.c */
 
     printf("%d\en", mount_id);
-    printf("%d %d   ", fhp\->handle_bytes, fhp\->handle_type);
+    printf("%u %d   ", fhp\->handle_bytes, fhp\->handle_type);
     for (int j = 0; j < fhp\->handle_bytes; j++)
         printf(" %02x", fhp\->f_handle[j]);
     printf("\en");
-- 
2.28.0

[PATCH 10/12] spu_run.2: Cast 'int' to 'unsigned int' when printing with "%x"

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man2/spu_run.2 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man2/spu_run.2 b/man2/spu_run.2
index b6bc2c131..ddd03ffd3 100644
--- a/man2/spu_run.2
+++ b/man2/spu_run.2
@@ -254,7 +254,7 @@ int main(void)
      *   0x00000002 (spu was stopped due to stop\-and\-signal)
      * | 0x12340000 (the stop\-and\-signal code)
      */
-    printf("SPU Status: %#08x\en", spu_status);
+    printf("SPU Status: %#08x\en", (unsigned) spu_status);
 
     exit(EXIT_SUCCESS);
 }
-- 
2.28.0

[PATCH 11/12] mprotect.2: Use "%p" rather than casting to 'long' when printing adresses (pointer values)

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man2/mprotect.2 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/man2/mprotect.2 b/man2/mprotect.2
index 47d250303..885238a8d 100644
--- a/man2/mprotect.2
+++ b/man2/mprotect.2
@@ -326,8 +326,7 @@ handler(int sig, siginfo_t *si, void *unused)
        Nevertheless, we use printf() here as a simple way of
        showing that the handler was called. */
 
-    printf("Got SIGSEGV at address: %#lx\en",
-            (long) si\->si_addr);
+    printf("Got SIGSEGV at address: %p\en", si\->si_addr);
     exit(EXIT_FAILURE);
 }
 
@@ -354,7 +353,7 @@ main(int argc, char *argv[])
     if (buffer == NULL)
         handle_error("memalign");
 
-    printf("Start of region:        %#lx\en", (long) buffer);
+    printf("Start of region:        %p\en", buffer);
 
     if (mprotect(buffer + pagesize * 2, pagesize,
                 PROT_READ) == \-1)
-- 
2.28.0

[PATCH 12/12] pidfd_open.2: Cast 'short' to 'unsigned int' when printing with "%x"

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man2/pidfd_open.2 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/man2/pidfd_open.2 b/man2/pidfd_open.2
index 0256d6d8c..c51e9fd67 100644
--- a/man2/pidfd_open.2
+++ b/man2/pidfd_open.2
@@ -253,7 +253,8 @@ main(int argc, char *argv[])
         exit(EXIT_FAILURE);
     }
 
-    printf("Events (%#x): POLLIN is %sset\en", pollfd.revents,
+    printf("Events (%#x): POLLIN is %sset\en",
+            (unsigned int) pollfd.revents,
             (pollfd.revents & POLLIN) ? "" : "not ");
 
     close(pidfd);
-- 
2.28.0

Re: [PATCH 01/12] dl_iterate_phdr.3: Cast 'ElfN_Word' to 'uintmax_t' for printf()

[Jakub Wilk]

* Alejandro Colomar <colomar.6.4.3@gmail.com>, 2020-09-12, 01:14:
>-        printf("    %2d: [%14p; memsz:%7lx] flags: %#x; ", j,
>+        printf("    %2d: [%14p; memsz:%7jx] flags: %#jx; ", j,
>                 (void *) (info\->dlpi_addr + info\->dlpi_phdr[j].p_vaddr),
>-                info\->dlpi_phdr[j].p_memsz,
>-                info\->dlpi_phdr[j].p_flags);
>+                (uintmax_t) info\->dlpi_phdr[j].p_memsz,
>+                (uintmax_t) info\->dlpi_phdr[j].p_flags);

The commit message is misleading. ElfN_Word is always uint32_t, 
regardless of N. On the other hand, on 64-bit architectures the type of 
p_memsz is Elf64_Xword (i.e., uint64_t), not Elf64_Word.

-- 
Jakub Wilk

Re: [PATCH 08/12] clock_getres.2: Cast 'time_t' to 'intmax_t' for printf() and fix the length modifiers

[Jakub Wilk]

* Alejandro Colomar <colomar.6.4.3@gmail.com>, 2020-09-12, 01:14:
>-    printf("%2ldh %2ldm %2lds", (ts.tv_sec % SECS_IN_DAY) / 3600,
>-            (ts.tv_sec % 3600) / 60, ts.tv_sec % 60);
>+    printf("%2jdh %2jdm %2jds",
>+            (intmax_t) (ts.tv_sec % SECS_IN_DAY) / 3600,
>+            (intmax_t) (ts.tv_sec % 3600) / 60,
>+            (intmax_t) ts.tv_sec % 60);

All these numbers are small, so %d and int casts would do the trick.

-- 
Jakub Wilk

Re: [PATCH 01/12] dl_iterate_phdr.3: Cast 'ElfN_Word' to 'uintmax_t' for printf()

[Alejandro Colomar]

Hi Jakub,

On 2020-09-12 07:52, Jakub Wilk wrote:
 > * Alejandro Colomar <colomar.6.4.3@gmail.com>, 2020-09-12, 01:14:
 >> -        printf("    %2d: [%14p; memsz:%7lx] flags: %#x; ", j,
 >> +        printf("    %2d: [%14p; memsz:%7jx] flags: %#jx; ", j,
 >>                 (void *) (info\->dlpi_addr +
 >> info\->dlpi_phdr[j].p_vaddr),
 >> -                info\->dlpi_phdr[j].p_memsz,
 >> -                info\->dlpi_phdr[j].p_flags);
 >> +                (uintmax_t) info\->dlpi_phdr[j].p_memsz,
 >> +                (uintmax_t) info\->dlpi_phdr[j].p_flags);
 >
 > The commit message is misleading. ElfN_Word is always uint32_t,
 > regardless of N. On the other hand, on 64-bit architectures the type of
 > p_memsz is Elf64_Xword (i.e., uint64_t), not Elf64_Word.
 >

Thanks for noticing that.  I had doubts while reading the man page.
I think it could be a bit more clear, but maybe it's me that I don't
speak elfic :p

Maybe 'ElfW(Word)' would be more appropriate?

Thanks,

Alex

Re: [PATCH 08/12] clock_getres.2: Cast 'time_t' to 'intmax_t' for printf() and fix the length modifiers

[Alejandro Colomar]

On 2020-09-12 07:55, Jakub Wilk wrote:
> * Alejandro Colomar <colomar.6.4.3@gmail.com>, 2020-09-12, 01:14:
>> -    printf("%2ldh %2ldm %2lds", (ts.tv_sec % SECS_IN_DAY) / 3600,
>> -            (ts.tv_sec % 3600) / 60, ts.tv_sec % 60);
>> +    printf("%2jdh %2jdm %2jds",
>> +            (intmax_t) (ts.tv_sec % SECS_IN_DAY) / 3600,
>> +            (intmax_t) (ts.tv_sec % 3600) / 60,
>> +            (intmax_t) ts.tv_sec % 60);
> 
> All these numbers are small, so %d and int casts would do the trick.
> 


True.  In this specific example '(int)' would be better.

Thanks,

Alex

[PATCH v2 08/12] clock_getres.2: Cast 'time_t' very small,values to 'int' for printf() and fix the length modifiers

[Alejandro Colomar]

Michael, here is the improved patch with Jakub's review applied.

Jakub, I added a line acknowledging your review.

Thanks,

Alex

----------------------------------------------------
 From cd0e8df57be9ebee47be5c5988a980e462c89085 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <colomar.6.4.3@gmail.com>
Date: Sat, 12 Sep 2020 11:53:01 +0200
Subject: [PATCH v2 08/12] clock_getres.2: Cast 'time_t' very small
values to 'int' for printf() and fix the length modifiers

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
Reviewed-by: Jakub Wilk <jwilk@jwilk.net>
---
  man2/clock_getres.2 | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/man2/clock_getres.2 b/man2/clock_getres.2
index 8fc7c6fef..0c14203ee 100644
--- a/man2/clock_getres.2
+++ b/man2/clock_getres.2
@@ -491,8 +491,10 @@ displayClock(clockid_t clock, char *name, bool showRes)
      if (days > 0)
          printf("%ld days + ", days);

-    printf("%2ldh %2ldm %2lds", (ts.tv_sec % SECS_IN_DAY) / 3600,
-            (ts.tv_sec % 3600) / 60, ts.tv_sec % 60);
+    printf("%2dh %2dm %2ds",
+            (int) (ts.tv_sec % SECS_IN_DAY) / 3600,
+            (int) (ts.tv_sec % 3600) / 60,
+            (int) ts.tv_sec % 60);
      printf(")\en");

      if (clock_getres(clock, &ts) == \-1) {
-- 
2.28.0

Re: [PATCH 10/12] spu_run.2: Cast 'int' to 'unsigned int' when printing with "%x"

[Alejandro Colomar]

Hi Michael,

I think this patch of mine should not be applied.

printf() will internally reinterpret the 'int' as 'unsigned int'
anyway, and the behaviour is completely defined AFAIK.

Relevant standard: C18 §6.5.2.2 6

And in the case you do want this patch, I should have written
'unsigned int' in the cast, for consistency.

Thanks,

Alex

On 2020-09-12 01:14, Alejandro Colomar wrote:
 > Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
 > ---
 >   man2/spu_run.2 | 2 +-
 >   1 file changed, 1 insertion(+), 1 deletion(-)
 >
 > diff --git a/man2/spu_run.2 b/man2/spu_run.2
 > index b6bc2c131..ddd03ffd3 100644
 > --- a/man2/spu_run.2
 > +++ b/man2/spu_run.2
 > @@ -254,7 +254,7 @@ int main(void)
 >        *   0x00000002 (spu was stopped due to stop\-and\-signal)
 >        * | 0x12340000 (the stop\-and\-signal code)
 >        */
 > -    printf("SPU Status: %#08x\en", spu_status);
 > +    printf("SPU Status: %#08x\en", (unsigned) spu_status);
 >
 >       exit(EXIT_SUCCESS);
 >   }
 >

Re: [PATCH 06/12] rtld-audit.7: Use "%u" rather than "%d" when printing 'unsigned int' values

[Michael Kerrisk (man-pages)]

On 9/12/20 1:14 AM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>

Thanks, Alex. Patch applied.

Cheers,

Michael

> ---
>  man7/rtld-audit.7 | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/man7/rtld-audit.7 b/man7/rtld-audit.7
> index f90731261..b1b7dfebc 100644
> --- a/man7/rtld-audit.7
> +++ b/man7/rtld-audit.7
> @@ -508,7 +508,7 @@ This is reportedly fixed in glibc 2.10.
>  unsigned int
>  la_version(unsigned int version)
>  {
> -    printf("la_version(): %d\en", version);
> +    printf("la_version(): %u\en", version);
>  
>      return version;
>  }
> @@ -572,7 +572,7 @@ la_symbind32(Elf32_Sym *sym, unsigned int ndx, uintptr_t *refcook,
>  {
>      printf("la_symbind32(): symname = %s; sym\->st_value = %p\en",
>              symname, sym\->st_value);
> -    printf("        ndx = %d; flags = %#x", ndx, *flags);
> +    printf("        ndx = %u; flags = %#x", ndx, *flags);
>      printf("; refcook = %p; defcook = %p\en", refcook, defcook);
>  
>      return sym\->st_value;
> @@ -584,7 +584,7 @@ la_symbind64(Elf64_Sym *sym, unsigned int ndx, uintptr_t *refcook,
>  {
>      printf("la_symbind64(): symname = %s; sym\->st_value = %p\en",
>              symname, sym\->st_value);
> -    printf("        ndx = %d; flags = %#x", ndx, *flags);
> +    printf("        ndx = %u; flags = %#x", ndx, *flags);
>      printf("; refcook = %p; defcook = %p\en", refcook, defcook);
>  
>      return sym\->st_value;
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 11/12] mprotect.2: Use "%p" rather than casting to 'long' when printing adresses (pointer values)

[Michael Kerrisk (man-pages)]

On 9/12/20 1:14 AM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>

Thanks, Alex. Patch applied.

Cheers,

Michael


> ---
>  man2/mprotect.2 | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/man2/mprotect.2 b/man2/mprotect.2
> index 47d250303..885238a8d 100644
> --- a/man2/mprotect.2
> +++ b/man2/mprotect.2
> @@ -326,8 +326,7 @@ handler(int sig, siginfo_t *si, void *unused)
>         Nevertheless, we use printf() here as a simple way of
>         showing that the handler was called. */
>  
> -    printf("Got SIGSEGV at address: %#lx\en",
> -            (long) si\->si_addr);
> +    printf("Got SIGSEGV at address: %p\en", si\->si_addr);
>      exit(EXIT_FAILURE);
>  }
>  
> @@ -354,7 +353,7 @@ main(int argc, char *argv[])
>      if (buffer == NULL)
>          handle_error("memalign");
>  
> -    printf("Start of region:        %#lx\en", (long) buffer);
> +    printf("Start of region:        %p\en", buffer);
>  
>      if (mprotect(buffer + pagesize * 2, pagesize,
>                  PROT_READ) == \-1)
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 02/12] malloc_hook.3: Remove unneeded cast, and print 'size_t' with "%zu"

[Michael Kerrisk (man-pages)]

On 9/12/20 1:14 AM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>

Thanks, Alex. Patch applied.

Cheers,

Michael

> ---
>  man3/malloc_hook.3 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/man3/malloc_hook.3 b/man3/malloc_hook.3
> index a509dc72f..fb45e30f2 100644
> --- a/man3/malloc_hook.3
> +++ b/man3/malloc_hook.3
> @@ -130,8 +130,8 @@ my_malloc_hook(size_t size, const void *caller)
>      old_malloc_hook = __malloc_hook;
>  
>      /* printf() might call malloc(), so protect it too. */
> -    printf("malloc(%u) called from %p returns %p\en",
> -            (unsigned int) size, caller, result);
> +    printf("malloc(%zu) called from %p returns %p\en",
> +            size, caller, result);
>  
>      /* Restore our own hooks */
>      __malloc_hook = my_malloc_hook;
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 09/12] open_by_handle_at.2: Use "%u" rather than "%d" when printing 'unsigned int' values

[Michael Kerrisk (man-pages)]

On 9/12/20 1:14 AM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>

Thanks, Alex. Patch applied.

Cheers,

Michael

> ---
>  man2/open_by_handle_at.2 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/man2/open_by_handle_at.2 b/man2/open_by_handle_at.2
> index fb8940696..53c8c7968 100644
> --- a/man2/open_by_handle_at.2
> +++ b/man2/open_by_handle_at.2
> @@ -600,7 +600,7 @@ main(int argc, char *argv[])
>         for later reuse by t_open_by_handle_at.c */
>  
>      printf("%d\en", mount_id);
> -    printf("%d %d   ", fhp\->handle_bytes, fhp\->handle_type);
> +    printf("%u %d   ", fhp\->handle_bytes, fhp\->handle_type);
>      for (int j = 0; j < fhp\->handle_bytes; j++)
>          printf(" %02x", fhp\->f_handle[j]);
>      printf("\en");
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 07/12] userfaultfd.2: Use 'PRIx64' rather than "%llx" when printing 64-bit fixed-width types

[Michael Kerrisk (man-pages)]

On 9/12/20 1:14 AM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>

Thanks, Alex. Patch applied.

Cheers,

Michael



> ---
>  man2/userfaultfd.2 | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/man2/userfaultfd.2 b/man2/userfaultfd.2
> index 126d4a854..c7e42804a 100644
> --- a/man2/userfaultfd.2
> +++ b/man2/userfaultfd.2
> @@ -566,6 +566,7 @@ Read address 0x7fd30106ec0f in main(): C
>     Licensed under the GNU General Public License version 2 or later.
>  */
>  #define _GNU_SOURCE
> +#include <inttypes.h>
>  #include <sys/types.h>
>  #include <stdio.h>
>  #include <linux/userfaultfd.h>
> @@ -650,8 +651,8 @@ fault_handler_thread(void *arg)
>          /* Display info about the page\-fault event */
>  
>          printf("    UFFD_EVENT_PAGEFAULT event: ");
> -        printf("flags = %llx; ", msg.arg.pagefault.flags);
> -        printf("address = %llx\en", msg.arg.pagefault.address);
> +        printf("flags = %"PRIx64"; ", msg.arg.pagefault.flags);
> +        printf("address = %"PRIx64"\en", msg.arg.pagefault.address);
>  
>          /* Copy the page pointed to by \(aqpage\(aq into the faulting
>             region. Vary the contents that are copied in, so that it
> @@ -673,7 +674,7 @@ fault_handler_thread(void *arg)
>          if (ioctl(uffd, UFFDIO_COPY, &uffdio_copy) == \-1)
>              errExit("ioctl\-UFFDIO_COPY");
>  
> -        printf("        (uffdio_copy.copy returned %lld)\en",
> +        printf("        (uffdio_copy.copy returned %"PRId64")\en",
>                  uffdio_copy.copy);
>      }
>  }
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 10/12] spu_run.2: Cast 'int' to 'unsigned int' when printing with "%x"

[Michael Kerrisk (man-pages)]

Hi Alex,

On 9/12/20 12:29 PM, Alejandro Colomar wrote:
> Hi Michael,
> 
> I think this patch of mine should not be applied.
> 
> printf() will internally reinterpret the 'int' as 'unsigned int'
> anyway, and the behaviour is completely defined AFAIK.
> 
> Relevant standard: C18 §6.5.2.2 6
> 
> And in the case you do want this patch, I should have written
> 'unsigned int' in the cast, for consistency.

Okay. 

Thanks,

Michael


> On 2020-09-12 01:14, Alejandro Colomar wrote:
>  > Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
>  > ---
>  >   man2/spu_run.2 | 2 +-
>  >   1 file changed, 1 insertion(+), 1 deletion(-)
>  >
>  > diff --git a/man2/spu_run.2 b/man2/spu_run.2
>  > index b6bc2c131..ddd03ffd3 100644
>  > --- a/man2/spu_run.2
>  > +++ b/man2/spu_run.2
>  > @@ -254,7 +254,7 @@ int main(void)
>  >        *   0x00000002 (spu was stopped due to stop\-and\-signal)
>  >        * | 0x12340000 (the stop\-and\-signal code)
>  >        */
>  > -    printf("SPU Status: %#08x\en", spu_status);
>  > +    printf("SPU Status: %#08x\en", (unsigned) spu_status);
>  >
>  >       exit(EXIT_SUCCESS);
>  >   }
>  >
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 12/12] pidfd_open.2: Cast 'short' to 'unsigned int' when printing with "%x"

[Michael Kerrisk (man-pages)]

Hi Alex,

On 9/12/20 1:14 AM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
> ---
>  man2/pidfd_open.2 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/man2/pidfd_open.2 b/man2/pidfd_open.2
> index 0256d6d8c..c51e9fd67 100644
> --- a/man2/pidfd_open.2
> +++ b/man2/pidfd_open.2
> @@ -253,7 +253,8 @@ main(int argc, char *argv[])
>          exit(EXIT_FAILURE);
>      }
>  
> -    printf("Events (%#x): POLLIN is %sset\en", pollfd.revents,
> +    printf("Events (%#x): POLLIN is %sset\en",
> +            (unsigned int) pollfd.revents,
>              (pollfd.revents & POLLIN) ? "" : "not ");
>  
>      close(pidfd);


Does something similar not also apply as for patch 11, since
the short will be promoted to an int in a variadic argument
list?

Thanks,

Michael






-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 03/12] pthread_getcpuclockid.3: Cast 'time_t' to 'intmax_t' for printf()

[Michael Kerrisk (man-pages)]

Hi Alex,

On 9/12/20 1:14 AM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
> ---
>  man3/pthread_getcpuclockid.3 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Casting to long is the historical practive here, and should be sufficient,
don't you think?

Thanks,

Michael


> diff --git a/man3/pthread_getcpuclockid.3 b/man3/pthread_getcpuclockid.3
> index 703735b91..a457a4cee 100644
> --- a/man3/pthread_getcpuclockid.3
> +++ b/man3/pthread_getcpuclockid.3
> @@ -115,6 +115,7 @@ Subthread CPU time:        0.992
>  
>  #include <time.h>
>  #include <stdio.h>
> +#include <stdint.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <pthread.h>
> @@ -143,7 +144,7 @@ pclock(char *msg, clockid_t cid)
>      printf("%s", msg);
>      if (clock_gettime(cid, &ts) == \-1)
>          handle_error("clock_gettime");
> -    printf("%4ld.%03ld\en", ts.tv_sec, ts.tv_nsec / 1000000);
> +    printf("%4jd.%03ld\en", (intmax_t) ts.tv_sec, ts.tv_nsec / 1000000);
>  }
>  
>  int
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 05/12] getpwent_r.3: Cast 'uid_t' to 'intmax_t' for printf()

[Michael Kerrisk (man-pages)]

Hi Alex,

On 9/12/20 1:14 AM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
> ---
>  man3/getpwent_r.3 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/man3/getpwent_r.3 b/man3/getpwent_r.3
> index b6c1c281f..ef4c01c21 100644
> --- a/man3/getpwent_r.3
> +++ b/man3/getpwent_r.3
> @@ -179,6 +179,7 @@ in the stream with all other threads.
>  #define _GNU_SOURCE
>  #include <pwd.h>
>  #include <stdio.h>
> +#include <stdint.h>
>  #define BUFLEN 4096
>  
>  int
> @@ -194,8 +195,8 @@ main(void)
>          i = getpwent_r(&pw, buf, sizeof(buf), &pwp);
>          if (i)
>              break;
> -        printf("%s (%d)\etHOME %s\etSHELL %s\en", pwp\->pw_name,
> -               pwp\->pw_uid, pwp\->pw_dir, pwp\->pw_shell);
> +        printf("%s (%jd)\etHOME %s\etSHELL %s\en", pwp\->pw_name,
> +               (intmax_t) pwp\->pw_uid, pwp\->pw_dir, pwp\->pw_shell);
>      }
>      endpwent();
>      exit(EXIT_SUCCESS);


Casting to long is the historical practice here, and should be sufficient,
don't you think? I realize there's an argument for making all of these 
system data type casts intmax_t / uintmax_t. But the counterarguments
(admittedly not compelling), are:

* Some people might still be confined to a pre C-99 world
* Churn (lots of changes)
* (long) is a sufficient cast for all of these types
  (except off_t).

Your thoughts?

Thanks,

Michael


Thanks,

Michael




-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 04/12] getgrent_r.3: Cast 'gid_t' to 'intmax_t' for printf()

[Michael Kerrisk (man-pages)]

Hi Alex,

See my reply to patch 05.

Thanks,

Michael

On 9/12/20 1:14 AM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
> ---
>  man3/getgrent_r.3 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/man3/getgrent_r.3 b/man3/getgrent_r.3
> index 76deec370..8170cd417 100644
> --- a/man3/getgrent_r.3
> +++ b/man3/getgrent_r.3
> @@ -174,6 +174,7 @@ in the stream with all other threads.
>  #define _GNU_SOURCE
>  #include <grp.h>
>  #include <stdio.h>
> +#include <stdint.h>
>  #include <stdlib.h>
>  #define BUFLEN 4096
>  
> @@ -189,7 +190,7 @@ main(void)
>          i = getgrent_r(&grp, buf, sizeof(buf), &grpp);
>          if (i)
>              break;
> -        printf("%s (%d):", grpp\->gr_name, grpp\->gr_gid);
> +        printf("%s (%jd):", grpp\->gr_name, (intmax_t) grpp\->gr_gid);
>          for (int j = 0; ; j++) {
>              if (grpp\->gr_mem[j] == NULL)
>                  break;
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH v2 08/12] clock_getres.2: Cast 'time_t' very small,values to 'int' for printf() and fix the length modifiers

[Michael Kerrisk (man-pages)]

On 9/12/20 12:06 PM, Alejandro Colomar wrote:
> Michael, here is the improved patch with Jakub's review applied.
> 
> Jakub, I added a line acknowledging your review.

Thanks, Alex. Patch applied.

Cheers,

Michael

> ----------------------------------------------------
>  From cd0e8df57be9ebee47be5c5988a980e462c89085 Mon Sep 17 00:00:00 2001
> From: Alejandro Colomar <colomar.6.4.3@gmail.com>
> Date: Sat, 12 Sep 2020 11:53:01 +0200
> Subject: [PATCH v2 08/12] clock_getres.2: Cast 'time_t' very small
> values to 'int' for printf() and fix the length modifiers
> 
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
> Reviewed-by: Jakub Wilk <jwilk@jwilk.net>
> ---
>   man2/clock_getres.2 | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/man2/clock_getres.2 b/man2/clock_getres.2
> index 8fc7c6fef..0c14203ee 100644
> --- a/man2/clock_getres.2
> +++ b/man2/clock_getres.2
> @@ -491,8 +491,10 @@ displayClock(clockid_t clock, char *name, bool showRes)
>       if (days > 0)
>           printf("%ld days + ", days);
> 
> -    printf("%2ldh %2ldm %2lds", (ts.tv_sec % SECS_IN_DAY) / 3600,
> -            (ts.tv_sec % 3600) / 60, ts.tv_sec % 60);
> +    printf("%2dh %2dm %2ds",
> +            (int) (ts.tv_sec % SECS_IN_DAY) / 3600,
> +            (int) (ts.tv_sec % 3600) / 60,
> +            (int) ts.tv_sec % 60);
>       printf(")\en");
> 
>       if (clock_getres(clock, &ts) == \-1) {
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 03/12] pthread_getcpuclockid.3: Cast 'time_t' to 'intmax_t' for printf()

[Michael Kerrisk (man-pages)]

Hi Alex,

On 9/13/20 7:58 AM, Michael Kerrisk (man-pages) wrote:
> Hi Alex,
> 
> On 9/12/20 1:14 AM, Alejandro Colomar wrote:
>> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
>> ---
>>  man3/pthread_getcpuclockid.3 | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Casting to long is the historical practive here, and should be sufficient,
> don't you think?

Oh -- I see Jakub made a similar comment for patch 08.

Do you want to redraft this patch too?

Thanks,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 12/12] pidfd_open.2: Cast 'short' to 'unsigned int' when printing with "%x"

[Alejandro Colomar]

On 9/13/20 7:55 AM, Michael Kerrisk (man-pages) wrote:
>> -    printf("Events (%#x): POLLIN is %sset\en", pollfd.revents,
>> +    printf("Events (%#x): POLLIN is %sset\en",
>> +            (unsigned int) pollfd.revents,
>>              (pollfd.revents & POLLIN) ? "" : "not ");
> 
> Does something similar not also apply as for patch 11, since
> the short will be promoted to an int in a variadic argument
> list?
> 

Good catch :) Cancel this patch.

Thanks,

Alex

Re: [PATCH 03/12] pthread_getcpuclockid.3: Cast 'time_t' to 'intmax_t' for printf()

[Alejandro Colomar]

Hi Michael,

On 9/13/20 8:05 AM, Michael Kerrisk (man-pages) wrote:
> Hi Alex,
>
> On 9/13/20 7:58 AM, Michael Kerrisk (man-pages) wrote:
>> Hi Alex,
>>
>> On 9/12/20 1:14 AM, Alejandro Colomar wrote:
>>> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
>>> ---
>>>  man3/pthread_getcpuclockid.3 | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> Casting to long is the historical practive here, and should be
sufficient,
>> don't you think?
>
> Oh -- I see Jakub made a similar comment for patch 08.
>
> Do you want to redraft this patch too?
>
> Thanks,
>
> Michael
>

The thing in patch 08 is that the numbers were modulo very small
numbers, and therefore very small numbers:

> -    printf("%2ldh %2ldm %2lds", (ts.tv_sec % SECS_IN_DAY) / 3600,
> -            (ts.tv_sec % 3600) / 60, ts.tv_sec % 60);
> +    printf("%2dh %2dm %2ds",
> +            (int) (ts.tv_sec % SECS_IN_DAY) / 3600,
> +            (int) (ts.tv_sec % 3600) / 60,
> +            (int) ts.tv_sec % 60);

In this case it doesn't apply.

Cheers,

Alex

Re: [PATCH 05/12] getpwent_r.3: Cast 'uid_t' to 'intmax_t' for printf()

[Alejandro Colomar]

Hi Michael,

On 9/13/20 8:01 AM, Michael Kerrisk (man-pages) wrote:
> Casting to long is the historical practice here, and should be sufficient,
> don't you think? I realize there's an argument for making all of these
> system data type casts intmax_t / uintmax_t. But the counterarguments
> (admittedly not compelling), are:

I was casting to (long) until I accidentally met with a cast to
(intmax_t) in ftw.3 (now I realize it was the only one in all of the
pages; lucke me).

I saw there a good point in using those types and tried it to see what
you think about them.  I still haven't sent you a big patchset with
s/(long)/(intmax_t)/ because I was waiting for this discussion :)

>
> * Some people might still be confined to a pre C-99 world

I guess that people are dealing with very special hardware/software
and can be trusted to understand what the C99 types are and which
pre-C99 do the job for them; intmax_t.3 might also help ;).

If someone doesn't live in such a special world and just doesn't
know the types, it's a good moment to learn them.

> * Churn (lots of changes)

True ...  But if changes are trivial enough, maybe a single big patch
can do it.

> * (long) is a sufficient cast for all of these types
>   (except off_t).

I don't like exceptions very much.  Maybe someone sees (long) all over
the place and thinks 'long' is enough everywhere and misuses it.

>
> Your thoughts?

Your thoughts?

Thanks,

Alex

Re: [PATCH 03/12] pthread_getcpuclockid.3: Cast 'time_t' to 'intmax_t' for printf()

[Michael Kerrisk (man-pages)]

Hi Alex,

On Sun, 13 Sep 2020 at 10:12, Alejandro Colomar <colomar.6.4.3@gmail.com> wrote:
>
> Hi Michael,
>
> On 9/13/20 8:05 AM, Michael Kerrisk (man-pages) wrote:
> > Hi Alex,
> >
> > On 9/13/20 7:58 AM, Michael Kerrisk (man-pages) wrote:
> >> Hi Alex,
> >>
> >> On 9/12/20 1:14 AM, Alejandro Colomar wrote:
> >>> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
> >>> ---
> >>>  man3/pthread_getcpuclockid.3 | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> Casting to long is the historical practive here, and should be
> sufficient,
> >> don't you think?
> >
> > Oh -- I see Jakub made a similar comment for patch 08.
> >
> > Do you want to redraft this patch too?
> >
> > Thanks,
> >
> > Michael
> >
>
> The thing in patch 08 is that the numbers were modulo very small
> numbers, and therefore very small numbers:
>
> > -    printf("%2ldh %2ldm %2lds", (ts.tv_sec % SECS_IN_DAY) / 3600,
> > -            (ts.tv_sec % 3600) / 60, ts.tv_sec % 60);
> > +    printf("%2dh %2dm %2ds",
> > +            (int) (ts.tv_sec % SECS_IN_DAY) / 3600,
> > +            (int) (ts.tv_sec % 3600) / 60,
> > +            (int) ts.tv_sec % 60);
>
> In this case it doesn't apply.

See my reply to patch 05, in a moment.

Thanks,

Michael





-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 05/12] getpwent_r.3: Cast 'uid_t' to 'intmax_t' for printf()

[Michael Kerrisk (man-pages)]

Hello Alex,

On Sun, 13 Sep 2020 at 10:29, Alejandro Colomar <colomar.6.4.3@gmail.com> wrote:
>
> Hi Michael,
>
> On 9/13/20 8:01 AM, Michael Kerrisk (man-pages) wrote:
> > Casting to long is the historical practice here, and should be sufficient,
> > don't you think? I realize there's an argument for making all of these
> > system data type casts intmax_t / uintmax_t. But the counterarguments
> > (admittedly not compelling), are:
>
> I was casting to (long) until I accidentally met with a cast to
> (intmax_t) in ftw.3 (now I realize it was the only one in all of the
> pages; lucke me).

Yep, that was a bit of a coincidence!

> I saw there a good point in using those types and tried it to see what
> you think about them.  I still haven't sent you a big patchset with
> s/(long)/(intmax_t)/ because I was waiting for this discussion :)

:-).

> > * Some people might still be confined to a pre C-99 world
>
> I guess that people are dealing with very special hardware/software
> and can be trusted to understand what the C99 types are and which
> pre-C99 do the job for them; intmax_t.3 might also help ;).
>
> If someone doesn't live in such a special world and just doesn't
> know the types, it's a good moment to learn them.
>
> > * Churn (lots of changes)
>
> True ...  But if changes are trivial enough, maybe a single big patch
> can do it.

Yes.

> > * (long) is a sufficient cast for all of these types
> >   (except off_t).
>
> I don't like exceptions very much.  Maybe someone sees (long) all over
> the place and thinks 'long' is enough everywhere and misuses it.
>
> > Your thoughts?
>
> Your thoughts?

Well, I said none of my arguments was too compelling... I think that
I'd take one big patch. Perhaps first, could you send me an estimate
of how many pages you think might be changed in the patch.


Thanks,

Michael

--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 05/12] getpwent_r.3: Cast 'uid_t' to 'intmax_t' for printf()

[Alejandro Colomar]

On 9/13/20 2:04 PM, Michael Kerrisk (man-pages) wrote:> Well, I said
none of my arguments was too compelling... I think that> I'd take one
big patch. Perhaps first, could you send me an estimate> of how many
pages you think might be changed in the patch.

$ grep -rn "(long)" |wc -l
56

I'd say around 20~30 pages.

Cheers

Alex

Re: [PATCH 05/12] getpwent_r.3: Cast 'uid_t' to 'intmax_t' for printf()

[Alejandro Colomar]

On 9/13/20 2:04 PM, Michael Kerrisk (man-pages) wrote:
> Well, I said none of my arguments was too compelling... I think that
> I'd take one big patch. Perhaps first, could you send me an estimate
> of how many pages you think might be changed in the patch.

BTW, please don't apply the patches about [u]intmax_t from this patchset:
I included them in the big patch so they are all together.

Thanks,

Alex

Re: [PATCH 03/12] pthread_getcpuclockid.3: Cast 'time_t' to 'intmax_t' for printf()

[Jakub Wilk]

* Michael Kerrisk <mtk.manpages@gmail.com>, 2020-09-13, 07:58:
>Casting to long is the historical practive here, and should be 
>sufficient, don't you think?

It general, the practice of casting time_t to long is wrong. There are 
existing systems that have 64-bit time_t but 32-bit long.

In this particular case, we're measuring CPU time, so there's 
practically no danger of overflow.

I'd either cast it to int (to make the code shortest and simplest) or to 
intmax_t (to be pedantically correct).

-- 
Jakub Wilk

Re: [PATCH v2 08/12] clock_getres.2: Cast 'time_t' very small,values to 'int' for printf() and fix the length modifiers

[Jakub Wilk]

* Alejandro Colomar <colomar.6.4.3@gmail.com>, 2020-09-12, 12:06:
>Jakub, I added a line acknowledging your review.
[...]
>Reviewed-by: Jakub Wilk <jwilk@jwilk.net>

I don't know about linux-man policies, but for the kernel development, 
Reviewed-by is not just an acknowledgment. It's a strong statement that 
is supposed to be added by the reviewer, not by the patch author:
https://www.kernel.org/doc/html/v5.8/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

-- 
Jakub Wilk

Re: [PATCH v2 08/12] clock_getres.2: Cast 'time_t' very small,values to 'int' for printf() and fix the length modifiers

[Alejandro Colomar]

Hi Jakub,

On 9/14/20 11:11 AM, Jakub Wilk wrote:
> * Alejandro Colomar <colomar.6.4.3@gmail.com>, 2020-09-12, 12:06:
>> Jakub, I added a line acknowledging your review.
> [...]
>> Reviewed-by: Jakub Wilk <jwilk@jwilk.net>
> 
> I don't know about linux-man policies, but for the kernel development,
> Reviewed-by is not just an acknowledgment. It's a strong statement that
> is supposed to be added by the reviewer, not by the patch author:
> https://www.kernel.org/doc/html/v5.8/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
> 
> 


Sorry, I didn't know.

Thanks,

Alex

Re: [PATCH 03/12] pthread_getcpuclockid.3: Cast 'time_t' to 'intmax_t' for printf()

[Michael Kerrisk (man-pages)]

Hi Jakub,

On Mon, 14 Sep 2020 at 10:58, Jakub Wilk <jwilk@jwilk.net> wrote:
>
> * Michael Kerrisk <mtk.manpages@gmail.com>, 2020-09-13, 07:58:
> >Casting to long is the historical practive here, and should be
> >sufficient, don't you think?
>
> It general, the practice of casting time_t to long is wrong. There are
> existing systems that have 64-bit time_t but 32-bit long.
>
> In this particular case, we're measuring CPU time, so there's
> practically no danger of overflow.

Yes, what you say makes sense.

> I'd either cast it to int (to make the code shortest and simplest) or to
> intmax_t (to be pedantically correct).

As per the patch from Alex, we'll universally move to intmax_t/uintmax_t casts.

Thanks,

Michael
-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 01/12] dl_iterate_phdr.3: Cast 'ElfN_Word' to 'uintmax_t' for printf()

[Jakub Wilk]

* Alejandro Colomar <colomar.6.4.3@gmail.com>, 2020-09-12, 10:31:
>> * Alejandro Colomar <colomar.6.4.3@gmail.com>, 2020-09-12, 01:14:
>>> -        printf("    %2d: [%14p; memsz:%7lx] flags: %#x; ", j,
>>> +        printf("    %2d: [%14p; memsz:%7jx] flags: %#jx; ", j,
>>>                 (void *) (info\->dlpi_addr +
>>> info\->dlpi_phdr[j].p_vaddr),
>>> -                info\->dlpi_phdr[j].p_memsz,
>>> -                info\->dlpi_phdr[j].p_flags);
>>> +                (uintmax_t) info\->dlpi_phdr[j].p_memsz,
>>> +                (uintmax_t) info\->dlpi_phdr[j].p_flags);
>>
>>The commit message is misleading. ElfN_Word is always uint32_t, 
>>regardless of N. On the other hand, on 64-bit architectures the type 
>>of p_memsz is Elf64_Xword (i.e., uint64_t), not Elf64_Word.
>
>Thanks for noticing that.  I had doubts while reading the man page.
>I think it could be a bit more clear, but maybe it's me that I don't 
>speak elfic :p
>
>Maybe 'ElfW(Word)' would be more appropriate?

I wouldn't know what "ElfW(Word)" means.

I'd write "Cast ELF types to uintmax_t" or something.

-- 
Jakub Wilk

Re: [PATCH 01/12] dl_iterate_phdr.3: Cast 'ElfN_Word' to 'uintmax_t' for printf()

[Alejandro Colomar]

On 2020-09-15 09:35, Jakub Wilk wrote:
>> Maybe 'ElfW(Word)' would be more appropriate?
> 
> I wouldn't know what "ElfW(Word)" means.
> 
> I'd write "Cast ELF types to uintmax_t" or something.
> 

Much better, yes.  However, it doesn't matter anymore: fixed by this patch:

https://lore.kernel.org/linux-man/98c426d9-6dde-5578-c1d5-942f21c5080a@gmail.com/T/#u

Thanks,

Alex