1114724 - Define PRIuSIZE format specifiers for size_t; PRIuSIZE is not a standard inttypes.h macro so move it from IntegerPrintfMacros.h to SizeFormatMacro.h.

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: remove-PRIuSIZE.patch

PRIuSIZE was added to IntegerPrintfMacros.h in bug 965022, but PRIuSIZE is not a standard inttypes.h macro or supported by MSVC.

In bug 1114444 comment 1, Waldo notes:

"%zu isn't supported by MSVC.  I see someone sub silentio added PRIuSIZE to IntegerPrintfMacros.h for this.  But this violates the intent of the header: that its supported interface be *only* a subset of <inttypes.h>, with the goal to swap that in when bug-free <inttypes.h> is provided by all supported platforms. ... (I guess I need to file a bug to remove it.)  For now just cast the fprintf argument to uintptr_t and use PRIuPTR til we figure out something to provide an alternative to %z."

Comment 1

[Chris Peterson [:cpeterson]]

Comment on attachment 8540305 [details] [diff] [review]
remove-PRIuSIZE.patch

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

::: ipc/glue/MessageChannel.cpp
@@ +1759,5 @@
> +                  static_cast<uintptr_t>(mDeferred.size()));
> +    printf_stderr("  out-of-turn Interrupt replies stack size: %" PRIuPTR "\n",
> +                  static_cast<uintptr_t>(mOutOfTurnReplies.size()));
> +    printf_stderr("  Pending queue size: %" PRIuPTR ", front to back:\n",
> +                  static_cast<uintptr_t>(mPending.size()));

Casting these size_t values to uintptr_t will fit, but casting to uint64_t might be more natural. These size_t values are counts, not pointer bits stuffed into integers.

Comment 2

[Chris Peterson [:cpeterson]]

Green Try build:
https://tbpl.mozilla.org/?tree=Try&rev=3f6f46f0f0ae

Comment 3

[Mike Hommey [:glandium]]

"Bug 965022 - Introduce a PRIuSIZE macro for printing std::size_t. r=jrmuizel"

Not even reviewed by a MFBT peer...

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8540305 [details] [diff] [review]
remove-PRIuSIZE.patch

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

%z is standard as of C99, which C++11 depends upon, so I believe C++11 requires %z in the legacy C library functionality it exposes.  Just need MSVC to get with the program.

So this is fine and all, but it seems like mfbt/SizeFormatMacro.h and a PRI{o,u,x,X}SIZE macro that expands to "z"{o,u,x,X} #ifndef _MSC_VER and to something more complicated if not.  This would also give us a list of places to convert, once MSVC supports %z.  The tactic here works, but it's not a pattern easily recognized, or likely to be followed, or easily adjusted once MSVC works correctly.  Mind doing that instead?

::: ipc/glue/MessageChannel.cpp
@@ +1759,5 @@
> +                  static_cast<uintptr_t>(mDeferred.size()));
> +    printf_stderr("  out-of-turn Interrupt replies stack size: %" PRIuPTR "\n",
> +                  static_cast<uintptr_t>(mOutOfTurnReplies.size()));
> +    printf_stderr("  Pending queue size: %" PRIuPTR ", front to back:\n",
> +                  static_cast<uintptr_t>(mPending.size()));

Realisticaly, counts and pointers are probably going to be identical these days, so this probably does slightly less damage to things than casting to uint64_t might do (which has to convert to a larger type on 32-bit, where uintptr_t likely doesn't).

Comment 5

[Mike Hommey [:glandium]]

Note that on s390 (yes, I build Firefox on those), uintptr_t and size_t have a different size (the former is 31 bits, the latter 32 bits). I don't know how that affects printf, though.

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

Based on Microsoft's documentation [1], something like this should work:

#ifndef _MSC_VER
#  define PRIoSIZE "zo"
#  define PRIuSIZE "zu"
#  define PRIxSIZE "zx"
#  define PRIXSIZE "zX"
#else
#  define PRIoSIZE "Io"
#  define PRIuSIZE "Iu"
#  define PRIxSIZE "Ix"
#  define PRIXSIZE "IX"
#endif

Unfortunately, from what I could find I don't think even MSVC 2015 will support the C99 format specifiers.

[1] http://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx

Comment 7

[Jeff Walden [:Waldo]]

I was thinking it was going to be this, actually:

#if define(XP_WIN)
#  if defined(_WIN64)
#    define PRIoSIZE  "I64o"
#    define PRIuSIZE  "I64u"
#    define PRIxSIZE  "I64x"
#    define PRIXSIZE  "I64X"
#  else
#    define PRIoSIZE  "I32o"
#    define PRIuSIZE  "I32u"
#    define PRIxSIZE  "I32x"
#    define PRIXSIZE  "I32X"
#  endif
#else
#  define PRIoSIZE  "zo"
#  define PRIuSIZE  "zu"
#  define PRIxSIZE  "zx"
#  define PRIXSIZE  "zX"
#endif

In my (old) MSVC2010 install, size_t is unsigned __int64 on _WIN64 and _W64 unsigned int (not sure what _W64 expands to, maybe nothing of relevance at all) on win32.  The I64 prefix comports with the PRIu64 defines in MSIntTypes.h.  The I32 prefix comports with PRIo32 and friends.

The other question: does this handle non-MSVC Windows compilers correctly?  I assume in all cases we're linking against MSVC's libc that wouldn't recognize %z, but maybe we statically link in a compiler-provided libc on some platforms.  ehsan, jcranmer?

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: define-PRIuSIZE.patch

Move PRIuSIZE definition from IntegerPrintfMacros.h to new SizeFormatMacro.h.

As ehoogeveen noted in comment 6, MSDN says Microsoft's nonstandard %I modifier for size_t is unsigned __int32 on 32-bit platforms and unsigned __int64 on 64-bit. Separate "I64u" and "I32u" definitions should not be necessary.

http://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx

* Waldo: why do you suggest the header file name "SizeFormatMacro.h" instead of "SizePrintfMacros.h" (following the "IntegerPrintfMacros.h" precedent with "Printf" and plural "Macros")?

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8540565 [details] [diff] [review]
define-PRIuSIZE.patch

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

> why do you suggest the header file name "SizeFormatMacro.h" instead of
> "SizePrintfMacros.h" (following the "IntegerPrintfMacros.h" precedent with
> "Printf" and plural "Macros")?

Because I was being hazy thinking about the header name.  SizePrintfMacros.h clearly makes more sense, use that instead.  :-)

Comment 10

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> The other question: does this handle non-MSVC Windows compilers correctly? 
> I assume in all cases we're linking against MSVC's libc that wouldn't
> recognize %z, but maybe we statically link in a compiler-provided libc on
> some platforms.  ehsan, jcranmer?

clang-cl links against the MSVC provided libc.  Not sure about mingw.  Jacek?

Comment 11

[Chris Peterson [:cpeterson]]

Wikipedia says MinGW links to MSVCRT by default:

https://en.wikipedia.org/wiki/MinGW#Programming_language_support

And the MinGW FAQ says to use %I64 instead of %ll so I assume %z is not supported:

> Q: Why doesn't %ll work with printf? How do I print a long long value?
> A: You should use %I64 instead of %ll when using msvcrt.

http://www.mingw.org/wiki/FAQ#toc15

Comment 12

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/687012e14ec9

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/687012e14ec9