RFR: 8292981: Unify and restructure integer printing format specifiers [v3]

[Kim Barrett]

On Mon, 29 Aug 2022 13:12:05 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Today we have some inconsistencies in how we name our integer printing format specifiers. I'd like to change this to be consistent. 
>> 
>> This patch comes from a discussion in #10028, which snowballed into restructuring the format specifiers. The main issues was that my original patch used PTR<size>_FORMAT to print integers with the format 0x000<value>. The reviewers felt that it was wrong to use PTR format specifiers when printing integers. I agree with that.
>> 
>> We do have format specifiers to print hex values out of integers, though they don't 0-pad like the PTR macros do, and only some of the prepend 0x.
>> 
>> I'd like to suggest that we use a convention to specify what we want. This is the current proposal:
>> 
>> // Guide to the suffixes used in the format specifiers for integers:
>> //        - print the decimal value:                   745565
>> //  _X    - print as hexadecimal, without leading 0s: 0x12345
>> //  _X_0  - print as hexadecimal, with leading 0s: 0x00012345
>> //  _H    - print as hexadecimal, without 0x prefix
>> //  _W(w) - prints w sized string with the given value right
>> //          adjusted. Use -w to print left adjusted.
>> //
>> // Note that the PTR format specifiers print using 0x with leading zeros,
>> // just like the _X_0 version for integers.
>> 
>> 
>> The patch also removes PTR32_FORMAT and PTR64_FORMAT and replace them with the corresponding integer format specifiers.
>
> Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Use PTR_FORMAT in AIX code

Looks good.

Nearly(?) all of my comments are about pre-existing things that might be best handled via followup RFEs.

src/hotspot/share/cds/filemap.cpp line 335:

> 333:   st->print_cr("- serialized_data_offset:         " SIZE_FORMAT_X, _serialized_data_offset);
> 334:   st->print_cr("- heap_begin:                     " INTPTR_FORMAT, p2i(_heap_begin));
> 335:   st->print_cr("- heap_end:                       " INTPTR_FORMAT, p2i(_heap_end));

Pre-existing: Seems like heap_begin and heap_end should be using `PTR_FORMAT`.  Similarly a few lines above for narrow_oop_base (which appears twice?), and a few lines down for requested/mapped_base_address.  There are some others like this in this file.

src/hotspot/share/runtime/deoptimization.cpp line 1904:

> 1902: 
> 1903:   // Log a message
> 1904:   Events::log_deopt_message(current, "Uncommon trap: trap_request=" INT32_FORMAT_X_0 " fr.pc=" INTPTR_FORMAT " relative=" INTPTR_FORMAT,

pre-existing: (first only) s/INTPTR_FORMAT/PTR_FORMAT/.

src/hotspot/share/utilities/globalDefinitions.hpp line 99:

> 97: //  _H    - print as hexadecimal, without 0x prefix
> 98: //  _W(w) - prints w sized string with the given value right
> 99: //          adjusted. Use -w to print left adjusted.

pre-existing: The "width" values for "W" suffixed formatters probably ought to be in the expansion as "XSTR(width)" rather than "#width", to permit the use of a macro for the width value.

src/hotspot/share/utilities/globalDefinitions.hpp line 133:

> 131: // Format integers which change size between 32- and 64-bit.
> 132: #define SSIZE_FORMAT             "%"          PRIdPTR
> 133: #define SSIZE_FORMAT_W(width)    "%"   #width PRIdPTR

SSIZE_FORMAT_W appears to be unused.

src/hotspot/share/utilities/globalDefinitions.hpp line 134:

> 132: #define SSIZE_FORMAT             "%"          PRIdPTR
> 133: #define SSIZE_FORMAT_W(width)    "%"   #width PRIdPTR
> 134: #define SIZE_FORMAT              "%"          PRIuPTR

With C++14/C99, we can use "%zu" instead of SIZE_FORMAT and "%zd" instead of SSIZE_FORMAT.

src/hotspot/share/utilities/globalDefinitions.hpp line 135:

> 133: #define SSIZE_FORMAT_W(width)    "%"   #width PRIdPTR
> 134: #define SIZE_FORMAT              "%"          PRIuPTR
> 135: #define SIZE_FORMAT_X            "0x%"        PRIxPTR

Some people might prefer to write "0x%zx" instead of SIZE_FORMAT_X.  It's annoying that the "#" flag for "%x" only prints the leading "0x" for non-zero values.  Who thought that was a good idea?  Otherwise we could write "%#zx" instead of SIZE_FORMAT_X (4 characters instead of 17, including the break in the middle of the string for the macro).

src/hotspot/share/utilities/globalDefinitions.hpp line 163:

> 161: // Format pointers which change size between 32- and 64-bit.
> 162: #ifdef  _LP64
> 163: #define INTPTR_FORMAT            "0x%016"     PRIxPTR

pre-existing: The naming of INTPTR_FORMAT is inconsistent with the other integer formatters because the "X_0" suffix is missing and padded hex printing is implicit. It's a (slightly shorter) syntactic synonym for the (not currently defined) INTX_FORMAT_X_0. While reviewing this change I noticed several uses of INTPTR_FORMAT that seemed like they should be PTR_FORMAT. I'm not entirely sure what the intended usage for INTPTR_FORMAT might be.  I wonder what would be left after making a pass looking carefully at uses of INTPTR_FORMAT.

src/hotspot/share/utilities/globalDefinitions.hpp line 169:

> 167: #define PTR_FORMAT               "0x%08"      PRIxPTR
> 168: #endif  // _LP64
> 169: #define INTPTR_FORMAT_H_W(width) "%"   #width PRIxPTR

The naming of INTPTR_FORMAT_H_W is inconsistent with INTPTR_FORMAT.  It seems only used in one area of Shenandoah code, where it seems like they should be using PTR_FORMAT, except they are trying to output fewer leading zeros.  I'm somewhat inclined to say this doesn't belong in globalDefinitions.hpp at all; if they have specialized printing desirements they can define whatever they want themselves.

-------------

Marked as reviewed by kbarrett (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10042