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

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch remove-PRIuSIZE.patch (obsolete) — Splinter Review
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."
Attachment #8540305 - Flags: review?(jwalden+bmo)
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.
"Bug 965022 - Introduce a PRIuSIZE macro for printing std::size_t. r=jrmuizel"

Not even reviewed by a MFBT peer...
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).
Attachment #8540305 - Flags: review?(jwalden+bmo)
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.
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
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?
Flags: needinfo?(ehsan.akhgari)
Flags: needinfo?(Pidgeot18)
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")?
Attachment #8540305 - Attachment is obsolete: true
Attachment #8540565 - Flags: review?(jwalden+bmo)
Summary: Remove PRIuSIZE from IntegerPrintfMacros.h because it is not a standard inttypes.h macro → Define PRIuSIZE format specifiers for size_t; PRIuSIZE is not a standard inttypes.h macro so move it from IntegerPrintfMacros.h to SizeFormatMacro.h.
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.  :-)
Attachment #8540565 - Flags: review?(jwalden+bmo) → review+
(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?
Flags: needinfo?(ehsan) → needinfo?(jacek)
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
Flags: needinfo?(jacek)
Flags: needinfo?(Pidgeot18)
https://hg.mozilla.org/mozilla-central/rev/687012e14ec9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.