RFR: 8313302: Fix formatting errors on Windows [v3]

[Thomas Stuefe]

On Fri, 28 Jul 2023 07:50:19 GMT, Julian Waters <jwaters at openjdk.org> wrote:

>> Fix several formatting errors on Windows
>
> Julian Waters has updated the pull request incrementally with one additional commit since the last revision:
> 
>   zPhysicalMemoryBacking_windows.cpp

Hi Julian,

I'm sorry, but I object to this change. 

It is another heap of aesthetic changes, with very very few changes actually needed (see inline remarks). Contrary to the title, it also touches shared code, and there are (almost) no errors to fix. And the one arguably incorrect place, in code heap, is not a "formatting error on windows".

Please consider that your aesthetic taste is not shared by everyone, but changes like these cause a lot of work for others.

Cheers, Thomas

src/hotspot/os/windows/gc/x/xMapper_windows.cpp line 118:

> 116:   const uintptr_t addr = map_view_no_placeholder(file_handle, file_offset, size);
> 117:   if (addr == 0) {
> 118:     log_error(gc)("Failed to map view of paging file mapping (%ld)", GetLastError());

Here, and in many other places:

All these `%d` -> `%ld` changes are questionable. Why do you think `l` is more correct?

Strictly spoken both existing and your versions are incorrect since GetLastError returns a DWORD, which is an unsigned 32-bit integer.

Were I to change anything, I would print an unsigned integer. But I would not change anything, since all documented error codes are well below 0x8000_0000. I think we are good here.

src/hotspot/os/windows/gc/x/xMapper_windows.cpp line 253:

> 251: 
> 252:   if (!res) {
> 253:     fatal("Failed to unreserve memory: " INTPTR_FORMAT " " SIZE_FORMAT "M (%ld)",

Here, and in many other places:

PTR_FORMAT -> INTPTR_FORMAT is pointless, since both macros are the same. 

We use PTR_FORMAT in many many places to print pointers. I prefer it to INTPTR_FORMAT, since it does not convey the assumption of a type. After all, the type does not matter: we just print the pointer as a raw hex value. Also, INTPTR suggests intptr_t which suggests signedness, which has no place here.

*If* we want to change this, we should first agree on the INTPTR_FORMAT vs PTR_FORMAT difference. And why we use intptr_t in many places that actually call for an uintptr_t.

But I would not change it. There is no need.

src/hotspot/os/windows/gc/z/zMapper_windows.cpp line 267:

> 265: 
> 266:   if (!res) {
> 267:     fatal_error("Failed to split placeholder", untype(addr), size);

Please don't.

`untype` does an assertion check. We don't want that here, when constructing arguments for a fatal error message.

src/hotspot/os/windows/osThread_windows.hpp line 30:

> 28:   typedef void* HANDLE;
> 29:  public:
> 30:   typedef unsigned int thread_id_t;

Since Windows is LLP64, this has no effect, even though beginthreadex returns an unsigned int.

src/hotspot/os/windows/os_windows.cpp line 3382:

> 3380:     if (Verbose && PrintMiscellaneous) {
> 3381:       reserveTimer.stop();
> 3382:       tty->print_cr("reserve_memory of %zx bytes took " JLONG_FORMAT " ms (" JLONG_FORMAT " ticks)", bytes,

SIZE_FORMAT.

src/hotspot/os/windows/perfMemory_windows.cpp line 259:

> 257:     if (PrintMiscellaneous && Verbose) {
> 258:       warning("%s is not a directory, file attributes = "
> 259:               "0x%08lx\n", path, fa);

UINT32_FORMAT

src/hotspot/os/windows/symbolengine.cpp line 635:

> 633:   } else {
> 634:     st->print("initialized successfully");
> 635:     st->print(" - sym options: 0x%lX", WindowsDbgHelp::symGetOptions());

No. SymGetOptions returns a DWORD, X is correct here.

src/hotspot/os_cpu/windows_x86/os_windows_x86.cpp line 422:

> 420:   st->print(", RBX=0x%016llx", uc->Rbx);
> 421:   st->print(", RCX=0x%016llx", uc->Rcx);
> 422:   st->print(", RDX=0x%016llx", uc->Rdx);

Why?

src/hotspot/share/code/codeHeapState.cpp line 1334:

> 1332:   if (SizeDistributionArray != nullptr) {
> 1333:     unsigned long total_count = 0;
> 1334:     uint64_t total_size  = 0;

This is a functional change (actually the only one I could spot), and it increases the size of this type to 64-bit on Windows. 

It may also be incorrect, since on 32-bit platforms you probably don't want 64-bit here. The correct type to use would be size_t.

Since this change is hidden in a deluge of unnecessary cosmetic changes, if we feel this is needed, this should be addressed in a separate RFE. 

But then again, probably not: code cache size is limited to 32-bit, and I bet we have a lot more places to change if we wanted to change that.

Also way out of scope for the RFE description.

src/hotspot/share/code/codeHeapState.cpp line 1387:

> 1385:                     "      occupied space) is used by the blocks in the given size range.\n"
> 1386:                     "      %ld characters are printed per percentage point.\n", pctFactor/100);
> 1387:       ast->print_cr("total size   of all blocks: " INT64_FORMAT_W(7) "M", (total_size<<log2_seg_size)/M);

Wrong for 32-bit platforms.

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

Changes requested by stuefe (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15063#pullrequestreview-1551690569
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277308018
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277313726
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277316318
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277322130
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277324742
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277326239
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277332892
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277326611
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277329553
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277331573