search

linux-nvme / libnvme

C Library for NVM Express on Linux
GNU Lesser General Public License v2.1
173 stars 129 forks source link

build warnings for __u64 in printf format string on PPC64 #210

Closed igaw closed 2 years ago

igaw commented 2 years ago

There are few build warnings for test/register.c and examples/discover-loop.c

https://build.opensuse.org/build/Base:System/openSUSE_Factory/ppc64/libnvme/_log

[   32s] ../test/register.c: In function ‘nvme_print_registers’:
[   32s] ../test/register.c:71:28: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘__u64’ {aka ‘long unsigned int’} [-Wformat=]
[   32s]    71 |         printf("%-10s : %llx\n", "CAP", cap);
[   32s]       |                         ~~~^            ~~~
[   32s]       |                            |            |
[   32s]       |                            |            __u64 {aka long unsigned int}
[   32s]       |                            long long unsigned int
[   32s]       |                         %lx
hreinecke commented 2 years ago

I've cleaned up the function documentation with PR #214 . Maybe it's worthwhile including that into the package to reduce the number of errors/warning generating during package build.

igaw commented 2 years ago

I thought this is easy:

-       printf("%-10s : %llx\n", "ASQ", asq);
-       printf("%-10s : %llx\n", "ACQ", acq);
+       printf("%-10s : %" PRIx64 "\n", "ASQ", asq);
+       printf("%-10s : %" PRIx64 "\n", "ACQ", acq);

though now I get:


In file included from ../test/register.c:15:
/usr/include/inttypes.h:121:41: note: format string is defined here
  121 | # define PRIx64         __PRI64_PREFIX "x"
../test/register.c:116:16: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘__u64’ {aka ‘long long unsigned int’} [-Wformat=]
  116 |         printf("%-10s : %" PRIx64 "\n", "ASQ", asq);
      |                ^~~~~~~~~~~                     ~~~
      |                                                |
      |                                                __u64 {aka long long unsigned int}
In file included from ../test/register.c:15:
/usr/include/inttypes.h:121:41: note: format string is defined here
  121 | # define PRIx64         __PRI64_PREFIX "x"
../test/register.c:117:16: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘__u64’ {aka ‘long long unsigned int’} [-Wformat=]
  117 |         printf("%-10s : %" PRIx64 "\n", "ACQ", acq);
      |                ^~~~~~~~~~~                     ~~~
      |                                                |
      |                                                __u64 {aka long long unsigned int}
hreinecke commented 2 years ago

And just make 'asq' and 'acq' an 'long unsigned int' is not an option?

igaw commented 2 years ago

If I understand this correctly, we use Linux types and inttypes.h is defined for the Posix types. And in this case __u64 is not matching the Posix defintion for a unsigned 64 bit type.

igaw commented 2 years ago

With a bit of explicit casting it works. Though note some macros expand to '__u64' too. 

src/nvme/types.h=enum nvme_pmrmsc {
src/nvme/types.h:       NVME_PMRMSC_CMSE_SHIFT  = 1,
src/nvme/types.h-       NVME_PMRMSC_CBA_SHIFT   = 12,
src/nvme/types.h:       NVME_PMRMSC_CMSE_MASK   = 0x1,
src/nvme/types.h-};
src/nvme/types.h=static const __u64 NVME_PMRMSC_CBA_MASK = 0xfffffffffffffull;

diff --git a/test/register.c b/test/register.c
index 8791083e2bae..1b6da865c009 100644
--- a/test/register.c
+++ b/test/register.c
@@ -43,7 +43,7 @@ static inline uint64_t nvme_mmio_read64(volatile void *addr)

 void nvme_print_registers(void *regs)
 {
-   __u64 cap   = nvme_mmio_read64(regs + NVME_REG_CAP);
+   uint64_t cap    = nvme_mmio_read64(regs + NVME_REG_CAP);
    __u32 vs    = nvme_mmio_read32(regs + NVME_REG_VS);
    __u32 intms = nvme_mmio_read32(regs + NVME_REG_INTMS);
    __u32 intmc = nvme_mmio_read32(regs + NVME_REG_INTMC);
@@ -51,36 +51,36 @@ void nvme_print_registers(void *regs)
    __u32 csts  = nvme_mmio_read32(regs + NVME_REG_CSTS);
    __u32 nssr  = nvme_mmio_read32(regs + NVME_REG_NSSR);
    __u32 aqa   = nvme_mmio_read32(regs + NVME_REG_AQA);
-   __u64 asq   = nvme_mmio_read64(regs + NVME_REG_ASQ);
-   __u64 acq   = nvme_mmio_read64(regs + NVME_REG_ACQ);
+   uint64_t asq    = nvme_mmio_read64(regs + NVME_REG_ASQ);
+   uint64_t acq    = nvme_mmio_read64(regs + NVME_REG_ACQ);
    __u32 cmbloc    = nvme_mmio_read32(regs + NVME_REG_CMBLOC);
    __u32 cmbsz = nvme_mmio_read32(regs + NVME_REG_CMBSZ);
    __u32 bpinfo    = nvme_mmio_read32(regs + NVME_REG_BPINFO);
    __u32 bprsel    = nvme_mmio_read32(regs + NVME_REG_BPRSEL);
-   __u64 bpmbl = nvme_mmio_read64(regs + NVME_REG_BPMBL);
-   __u64 cmbmsc    = nvme_mmio_read64(regs + NVME_REG_CMBMSC);
+   uint64_t bpmbl  = nvme_mmio_read64(regs + NVME_REG_BPMBL);
+   uint64_t cmbmsc = nvme_mmio_read64(regs + NVME_REG_CMBMSC);
    __u32 cmbsts    = nvme_mmio_read32(regs + NVME_REG_CMBSTS);
    __u32 pmrcap    = nvme_mmio_read32(regs + NVME_REG_PMRCAP);
    __u32 pmrctl    = nvme_mmio_read32(regs + NVME_REG_PMRCTL);
    __u32 pmrsts    = nvme_mmio_read32(regs + NVME_REG_PMRSTS);
    __u32 pmrebs    = nvme_mmio_read32(regs + NVME_REG_PMREBS);
    __u32 pmrswtp   = nvme_mmio_read32(regs + NVME_REG_PMRSWTP);
-   __u64 pmrmsc    = nvme_mmio_read32(regs + NVME_REG_PMRMSCL) |
+   uint64_t pmrmsc = nvme_mmio_read32(regs + NVME_REG_PMRMSCL) |
           (__u64)nvme_mmio_read64(regs + NVME_REG_PMRMSCU) << 32;

-   printf("%-10s : %llx\n", "CAP", cap);
-   printf("  %-8s : %llx\n", "MQES", NVME_CAP_MQES(cap));
-   printf("  %-8s : %llx\n", "CQRS", NVME_CAP_CQR(cap));
-   printf("  %-8s : %llx\n", "AMS", NVME_CAP_AMS(cap));
-   printf("  %-8s : %llx\n", "TO", NVME_CAP_TO(cap));
-   printf("  %-8s : %llx\n", "DSTRD", NVME_CAP_DSTRD(cap));
-   printf("  %-8s : %llx\n", "NSSRC", NVME_CAP_NSSRC(cap));
-   printf("  %-8s : %llx\n", "CSS", NVME_CAP_CSS(cap));
-   printf("  %-8s : %llx\n", "BPS", NVME_CAP_BPS(cap));
-   printf("  %-8s : %llx\n", "MPSMIN", NVME_CAP_MPSMIN(cap));
-   printf("  %-8s : %llx\n", "MPSMAX", NVME_CAP_MPSMAX(cap));
-   printf("  %-8s : %llx\n", "CMBS", NVME_CAP_CMBS(cap));
-   printf("  %-8s : %llx\n", "PMRS", NVME_CAP_PMRS(cap));
+   printf("%-10s : %" PRIx64 "\n", "CAP", cap);
+   printf("  %-8s : %" PRIx64 "\n", "MQES", NVME_CAP_MQES(cap));
+   printf("  %-8s : %" PRIx64 "\n", "CQRS", NVME_CAP_CQR(cap));
+   printf("  %-8s : %" PRIx64 "\n", "AMS", NVME_CAP_AMS(cap));
+   printf("  %-8s : %" PRIx64 "\n", "TO", NVME_CAP_TO(cap));
+   printf("  %-8s : %" PRIx64 "\n", "DSTRD", NVME_CAP_DSTRD(cap));
+   printf("  %-8s : %" PRIx64 "\n", "NSSRC", NVME_CAP_NSSRC(cap));
+   printf("  %-8s : %" PRIx64 "\n", "CSS", NVME_CAP_CSS(cap));
+   printf("  %-8s : %" PRIx64 "\n", "BPS", NVME_CAP_BPS(cap));
+   printf("  %-8s : %" PRIx64 "\n", "MPSMIN", NVME_CAP_MPSMIN(cap));
+   printf("  %-8s : %" PRIx64 "\n", "MPSMAX", NVME_CAP_MPSMAX(cap));
+   printf("  %-8s : %" PRIx64 "\n", "CMBS", NVME_CAP_CMBS(cap));
+   printf("  %-8s : %" PRIx64 "\n", "PMRS", NVME_CAP_PMRS(cap));

    printf("%-10s : %x\n", "VS", vs);
    printf("  %-8s : %x\n", "MJR", NVME_VS_TER(vs));
@@ -112,8 +112,8 @@ void nvme_print_registers(void *regs)
    printf("  %-8s : %x\n", "ASQS", NVME_AQA_ASQS(aqa));
    printf("  %-8s : %x\n", "ACQS", NVME_AQA_ACQS(aqa));

-   printf("%-10s : %llx\n", "ASQ", asq);
-   printf("%-10s : %llx\n", "ACQ", acq);
+   printf("%-10s : %" PRIx64 "\n", "ASQ", asq);
+   printf("%-10s : %" PRIx64 "\n", "ACQ", acq);

    printf("%-10s : %x\n",   "CMBLOC",  cmbloc);
    printf("  %-8s : %x\n", "BIR", NVME_CMBLOC_BIR(cmbloc));
@@ -133,7 +133,7 @@ void nvme_print_registers(void *regs)
    printf("  %-8s : %x\n", "WDS", NVME_CMBSZ_WDS(cmbsz));
    printf("  %-8s : %x\n", "SZU", NVME_CMBSZ_SZU(cmbsz));
    printf("  %-8s : %x\n", "SZ", NVME_CMBSZ_SZ(cmbsz));
-   printf("  %-8s : %llx\n", "bytes", nvme_cmb_size(cmbsz));
+   printf("  %-8s : %" PRIx64 "\n", "bytes", (uint64_t)nvme_cmb_size(cmbsz));

    printf("%-10s : %x\n", "BPINFO", bpinfo);
    printf("  %-8s : %x\n", "BPSZ", NVME_BPINFO_BPSZ(bpinfo));
@@ -145,12 +145,12 @@ void nvme_print_registers(void *regs)
    printf("  %-8s : %x\n", "BPROF", NVME_BPRSEL_BPROF(bprsel));
    printf("  %-8s : %x\n", "BPID", NVME_BPRSEL_BPID(bprsel));

-   printf("%-10s : %llx\n", "BPMBL", bpmbl);
+   printf("%-10s : %" PRIx64 "\n", "BPMBL", bpmbl);

-   printf("%-10s : %llx\n", "CMBMSC", cmbmsc);
-   printf("  %-8s : %llx\n", "CRE", NVME_CMBMSC_CRE(cmbmsc));
-   printf("  %-8s : %llx\n", "CMSE", NVME_CMBMSC_CMSE(cmbmsc));
-   printf("  %-8s : %llx\n", "CBA", NVME_CMBMSC_CBA(cmbmsc));
+   printf("%-10s : %" PRIx64 "\n", "CMBMSC", cmbmsc);
+   printf("  %-8s : %" PRIx64 "\n", "CRE", NVME_CMBMSC_CRE(cmbmsc));
+   printf("  %-8s : %" PRIx64 "\n", "CMSE", NVME_CMBMSC_CMSE(cmbmsc));
+   printf("  %-8s : %" PRIx64 "\n", "CBA", (uint64_t)NVME_CMBMSC_CBA(cmbmsc));

    printf("%-10s : %x\n", "CMBSTS", cmbsts);
    printf("  %-8s : %x\n", "CBAI", NVME_CMBSTS_CBAI(cmbsts));
@@ -177,16 +177,16 @@ void nvme_print_registers(void *regs)
    printf("  %-8s : %x\n", "PMRSZU", NVME_PMREBS_PMRSZU(pmrebs));
    printf("  %-8s : %x\n", "RBB", NVME_PMREBS_RBB(pmrebs));
    printf("  %-8s : %x\n", "PMRWBZ", NVME_PMREBS_PMRWBZ(pmrebs));
-   printf("  %-8s : %llx\n", "bytes", nvme_pmr_size(pmrebs));
+   printf("  %-8s : %" PRIx64 "\n", "bytes", (uint64_t)nvme_pmr_size(pmrebs));

    printf("%-10s : %x\n", "PMRSWTP", pmrswtp);
    printf("  %-8s : %x\n", "PMRSWTU", NVME_PMRSWTP_PMRSWTU(pmrswtp));
    printf("  %-8s : %x\n", "PMRSWTV", NVME_PMRSWTP_PMRSWTV(pmrswtp));
-   printf("  %-8s : %llx\n", "tput", nvme_pmr_throughput(pmrswtp));
+   printf("  %-8s : %" PRIx64 "\n", "tput", (uint64_t)nvme_pmr_throughput(pmrswtp));

-   printf("%-10s : %llx\n", "PMRMSC", pmrmsc);
-   printf("  %-8s : %llx\n", "CMSE", NVME_PMRMSC_CMSE(pmrmsc));
-   printf("  %-8s : %llx\n", "CBA", NVME_PMRMSC_CBA(pmrmsc));
+   printf("%-10s : %" PRIx64 "\n", "PMRMSC", pmrmsc);
+   printf("  %-8s : %" PRIx64 "\n", "CMSE", NVME_PMRMSC_CMSE(pmrmsc));
+   printf("  %-8s : %" PRIx64 "\n", "CBA", (uint64_t)NVME_PMRMSC_CBA(pmrmsc));
 }

 int main(int argc, char **argv)
igaw commented 2 years ago

And the funny thing about this is, we don't have build warnings on anything but PPC64...

hreinecke commented 2 years ago

Sigh. Two implementations for 64-bit values conflicting. 'unsigned long long' is IIRC a gcc extension, ensuring that you get a 'real' 64 bit type even on 32 bit architectures. 'PRIx64' is a POSIX thingie, to ensure that 'unsigned long' is printed correctly (as its size depends on the architecture).

Consequently, either use '%llx' for __u64, or 'PRI64' for 'unsigned long'.

hreinecke commented 2 years ago

So reviewing the first error, it looks as if PPC would define '__u64' as 'unsigned long'. Which arguably is not quite correct.

igaw commented 2 years ago

Fixed by #389