945029 - mfbt/tests/TestIntegerPrintfMacros.cpp:218:72: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'intptr_t {aka int}' [-Wformat]

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Daniel Holbert [:dholbert]]

mfbt/tests is nearly warning-free, but it has at least these warnings on Android:
{
../../../mfbt/tests/TestIntegerPrintfMacros.cpp: In function 'void TestPrintSignedPtr()':
../../../mfbt/tests/TestIntegerPrintfMacros.cpp:218:72: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'intptr_t {aka int}' [-Wformat]
../../../mfbt/tests/TestIntegerPrintfMacros.cpp:222:72: warning: format '%li' expects argument of type 'long int', but argument 3 has type 'intptr_t {aka int}' [-Wformat]
../../../mfbt/tests/TestIntegerPrintfMacros.cpp: In function 'void TestPrintUnsignedPtr()':
../../../mfbt/tests/TestIntegerPrintfMacros.cpp:538:73: warning: format '%lo' expects argument of type 'long unsigned int', but argument 3 has type 'uintptr_t {aka unsigned int}' [-Wformat]
../../../mfbt/tests/TestIntegerPrintfMacros.cpp:542:73: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'uintptr_t {aka unsigned int}' [-Wformat]
../../../mfbt/tests/TestIntegerPrintfMacros.cpp:546:74: warning: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'uintptr_t {aka unsigned int}' [-Wformat]
../../../mfbt/tests/TestIntegerPrintfMacros.cpp:550:73: warning: format '%lX' expects argument of type 'long unsigned int', but argument 3 has type 'uintptr_t {aka unsigned int}' [-Wformat]
}

This is from lines like:
> 218   sprintf(output, "%" PRIdPTR, intptr_t(reinterpret_cast<void*>(12345678)));

It looks like ["%" PRIdPTR] evaluates to [ "%ld" ], but the compiler wants [ "%d" ] for intptr_t. (and similar for uintptr_t)

Comment 1

[Daniel Holbert [:dholbert]]

Sample build log showing these warnings:
https://tbpl.mozilla.org/php/getParsedLog.php?id=31298066&tree=Try

Comment 2

[Daniel Holbert [:dholbert]]

Quotes/links for relevant source:

214 static void
215 TestPrintSignedPtr()
216 {
217   PoisonOutput();
218   sprintf(output, "%" PRIdPTR, intptr_t(reinterpret_cast<void*>(12345678)));
219   MOZ_ASSERT(!strcmp(output, "12345678"));
220 
221   PoisonOutput();
222   sprintf(output, "%" PRIiPTR, intptr_t(reinterpret_cast<void*>(87654321)));
223   MOZ_ASSERT(!strcmp(output, "87654321"));
224 }
http://mxr.mozilla.org/mozilla-central/source/mfbt/tests/TestIntegerPrintfMacros.cpp#214

534 static void
535 TestPrintUnsignedPtr()
536 {
537   PoisonOutput();
538   sprintf(output, "%" PRIoPTR, uintptr_t(reinterpret_cast<void*>(12345678)));
539   MOZ_ASSERT(!strcmp(output, "57060516"));
540 
541   PoisonOutput();
542   sprintf(output, "%" PRIuPTR, uintptr_t(reinterpret_cast<void*>(87654321)));
543   MOZ_ASSERT(!strcmp(output, "87654321"));
544 
545   PoisonOutput();
546   sprintf(output, "%" PRIxPTR, uintptr_t(reinterpret_cast<void*>(0x4c3a791)));
547   MOZ_ASSERT(!strcmp(output, "4c3a791"));
548 
549   PoisonOutput();
550   sprintf(output, "%" PRIXPTR, uintptr_t(reinterpret_cast<void*>(0xF328DB)));
551   MOZ_ASSERT(!strcmp(output, "F328DB"));
552 }

http://mxr.mozilla.org/mozilla-central/source/mfbt/tests/TestIntegerPrintfMacros.cpp#535

I think (?) this bug indicates that we've got improper valus for these PR*PTR #defines, on Android.

Comment 3

[Jeff Walden [:Waldo]]

Yes.  Astonishingly, some platforms have <inttypes.h> that triggers compiler warnings when used correctly.

Our options are to implement our own <inttypes.h> for such platforms, or pragma them away, or do nothing.  Reimplementing is kind of perilous, given we then have to have include-guard syncing with that platform and all.  Pragma would work if the goal is to fix the directory.  Ideally, however, we'd use this test to verify correctness of these macros everywhere, using those compiler warnings.

So, I dunno, no clearly correct options here.  I guess we can pragma it off, narrowly scoped to only the bits that have format mismatches here, and narrowly scoped in terms of compiler, toolchain, etc. that are affected.  Someone affected, who cares enough, can write the patch.  :-)

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: fix-android-inttypes.patch

Ehsan started to suppress these Android warnings in bug 986928. These warnings can't be suppressed by a narrow warning flag, so all -Wformat warnings would have to be made non-fatal in any directory that uses PRI*PTR.

This patch proposes to fix up Android's broken PRI*PTR macros and remove the warning suppressions from the tree.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8421519 [details] [diff] [review]
fix-android-inttypes.patch

Review of attachment 8421519 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/IntegerPrintfMacros.h
@@ +41,5 @@
> + * Fix up Android's broken [u]intptr_t inttype macros. Android's PRI*PTR
> + * macros are defined as "ld", but sizeof(intptr_t) is 4 and sizeof(long)
> + * is 8 on 32-bit Android.
> + */
> +#if defined(ANDROID) && !defined(HAVE_64BIT_OS)

Use __LP64__ instead of HAVE_64BIT_OS

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

http://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
Android has a 64bit version?

Comment 7

[Mike Hommey [:glandium]]

(In reply to JW Wang [:jwwang] from comment #6)
> http://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
> Android has a 64bit version?

Soon.

Comment 8

[(no longer active)]

Comment on attachment 8421519 [details] [diff] [review]
fix-android-inttypes.patch

Review of attachment 8421519 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!  One question though, doesn't this completely fix bug 986928?  What is left to be done there?

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to :Ehsan Akhgari (@work week, needinfo? me!) from comment #8)
> Thanks for doing this!  One question though, doesn't this completely fix bug
> 986928?  What is left to be done there?

Yes it would, but based on Waldo's comment 3, I'm uncertain whether he will like #undef'ing and redefining std macros from the system headers.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8421519 [details] [diff] [review]
fix-android-inttypes.patch

Review of attachment 8421519 [details] [diff] [review]:
-----------------------------------------------------------------

> I'm uncertain whether he will like #undef'ing and redefining std macros from the system headers.

Of course I don't like it.  I have taste.  r=me anyway.  :-)  Gotta do what you gotta do.  And this is probably better than reimplementing the whole header, yeah.

::: mfbt/IntegerPrintfMacros.h
@@ +48,5 @@
> +#  undef  PRIoPTR      /* uintptr_t */
> +#  undef  PRIuPTR      /* uintptr_t */
> +#  undef  PRIxPTR      /* uintptr_t */
> +#  undef  PRIXPTR      /* uintptr_t */
> +#  define PRIdPTR "d"  /* intptr_t */

Could you pair up undef/define pairs directly, so I don't have to look ten or so lines further down to see the corresponding redefinition?

@@ +50,5 @@
> +#  undef  PRIxPTR      /* uintptr_t */
> +#  undef  PRIXPTR      /* uintptr_t */
> +#  define PRIdPTR "d"  /* intptr_t */
> +#  define PRIiPTR "i"  /* intptr_t */
> +#  define PRIoPTR "u"  /* uintptr_t */

Shouldn't this be "o"?

@@ +53,5 @@
> +#  define PRIiPTR "i"  /* intptr_t */
> +#  define PRIoPTR "u"  /* uintptr_t */
> +#  define PRIuPTR "u"  /* uintptr_t */
> +#  define PRIxPTR "x"  /* uintptr_t */
> +#  define PRIXPTR "X"  /* uintptr_t */

As these are all blithely assuming things about the actual type in use, it'd be good to verify them.  Please add

#if defined(ANDROID) && !defined(__LP64__)
static_assert(mozilla::IsSame<int, intptr_t>::value,
              "emulated PRI[di]PTR definitions will be wrong");
static_assert(mozilla::IsSame<unsigned int, uintptr_t>::value,
              "emulated PRI[ouxX]PTR definitions will be wrong");
#endif

as a double-check.  Update the comment here, and put a comment by there, directing each to the other's existence.  Too bad we don't really want to use any of the rest of mfbt here, to use IsSame or MOZ_STATIC_ASSERT directly.

Comment 11

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d089350aff81

Landed with requested changes:
* Reformatted undef/define pairs
* Fixed PRIoPTR "o"
* Added static_asserts to TestTypeTraits.cpp

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d089350aff81