8313302: Fix formatting errors on Windows #15063
Conversation
|
👋 Welcome back jwaters! A progress list of the required criteria for merging this PR into |
|
@TheShermanTanker The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
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
| const uintptr_t addr = map_view_no_placeholder(file_handle, file_offset, size); | ||
| if (addr == 0) { | ||
| log_error(gc)("Failed to map view of paging file mapping (%d)", GetLastError()); | ||
| log_error(gc)("Failed to map view of paging file mapping (%ld)", GetLastError()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
DWORD is defined as an unsigned long on all Windows compilers, which more accurately maps to %ld under C++ rules. This was more for correctness, but may not be strictly needed. Under JDK-8288293 however this is diagnosed as a formatting error, when compilerWarnings_gcc.hpp's format checking is enabled. I thus thought this was a fix that could be sent upstream
|
|
||
| if (!res) { | ||
| fatal("Failed to unreserve memory: " PTR_FORMAT " " SIZE_FORMAT "M (%d)", | ||
| fatal("Failed to unreserve memory: " INTPTR_FORMAT " " SIZE_FORMAT "M (%ld)", |
There was a problem hiding this comment.
Here, and in many other places:
PTR_FORMAT -> INTPTR_FORMAT 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.
There was a problem hiding this comment.
Yes. Please keep using PTR_FORMAT to print what's semantically a pointer in ZGC.
|
|
||
| if (!res) { | ||
| fatal_error("Failed to split placeholder", addr, size); | ||
| fatal_error("Failed to split placeholder", untype(addr), size); |
There was a problem hiding this comment.
Please don't.
untype does an assertion check. We don't want that here, when constructing arguments for a fatal error message.
| typedef void* HANDLE; | ||
| public: | ||
| typedef unsigned long thread_id_t; | ||
| typedef unsigned int thread_id_t; |
There was a problem hiding this comment.
Since Windows is LLP64, this has no effect, even though beginthreadex returns an unsigned int.
There was a problem hiding this comment.
It's also another formatting change, as this is printed as %d in shared code. This also matches the definition of int on other platforms. This should functionally be harmless, because ints are promoted to longs (DWORD) whenever thread_id_t is passed to a callee that expects a DWORD, and since longs are the same size as ints this is effectively an implicit cast with no effect in those cases
There was a problem hiding this comment.
I can live with this change. Its also small and isolated.
| if (Verbose && PrintMiscellaneous) { | ||
| reserveTimer.stop(); | ||
| tty->print_cr("reserve_memory of %Ix bytes took " JLONG_FORMAT " ms (" JLONG_FORMAT " ticks)", bytes, | ||
| tty->print_cr("reserve_memory of %zx bytes took " JLONG_FORMAT " ms (" JLONG_FORMAT " ticks)", bytes, |
There was a problem hiding this comment.
Regarding SIZE_FORMAT - https://bugs.openjdk.org/browse/JDK-8256379. Some people have been replacing
uses of SIZE_FORMAT when touching the relevant code for other reasons.
There was a problem hiding this comment.
Yes, I think it might be best if people started using %zd rather than adding new SIZE_FORMAT to the code. I haven't figured out the best way to change the rest. If using %zx, I suggest adding 0x%zx.
| if (PrintMiscellaneous && Verbose) { | ||
| warning("%s is not a directory, file attributes = " | ||
| INTPTR_FORMAT "\n", path, fa); | ||
| "0x%08lx\n", path, fa); |
There was a problem hiding this comment.
Noted, if this is accepted
| st->print( "RAX=0x%016llx", uc->Rax); | ||
| st->print(", RBX=0x%016llx", uc->Rbx); | ||
| st->print(", RCX=0x%016llx", uc->Rcx); | ||
| st->print(", RDX=0x%016llx", uc->Rdx); |
There was a problem hiding this comment.
All of these are either DWORD64 or DWORD, none are actually intptr_t's or uintptr_t's
There was a problem hiding this comment.
In that case UINT64_FORMAT_X would be more correct. But again, unless there is an important reason to do so, I would leave changing this to when someone changes/moves around the code.
| if (SizeDistributionArray != nullptr) { | ||
| unsigned long total_count = 0; | ||
| unsigned long total_size = 0; | ||
| uint64_t total_size = 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
For this one (and the one below) I couldn't figure out whether the intention was for a 32 or 64 bit number in this case. This would be 64 bit on every platform but on Windows, which is 32 bit, and the code directly below this also faces the same issue too, is this intentional?
There was a problem hiding this comment.
long is 32-bit on Linux x86 and arm32 too.
There was a problem hiding this comment.
[not a review, just a drive-by comment]
The type long really shouldn't be used in shared code. There have been discussions and maybe some efforts
to nuke uses, but having just done the grep, there are still quite a few. The one in question might actually be an
appropriate place to use uintx.
| " occupied space) is used by the blocks in the given size range.\n" | ||
| " %ld characters are printed per percentage point.\n", pctFactor/100); | ||
| ast->print_cr("total size of all blocks: %7ldM", (total_size<<log2_seg_size)/M); | ||
| ast->print_cr("total size of all blocks: " INT64_FORMAT_W(7) "M", (total_size<<log2_seg_size)/M); |
There was a problem hiding this comment.
I don't quite know what to do here, since on platforms where long < size_t (which in practice is probably just 32 bit or Windows) this division will promote to the type of size_t, which is likely 64 bits in size, but unfortunately %zd cannot be used here since the promotion only happens on said platforms where long < size_t, which will trip a compilation breaking warning on platforms where sizeof long is sizeof size_t, which is probably just 64 bit platforms that are not Windows
There was a problem hiding this comment.
If we defer the issue of what type total_size should be and just keep it as unsigned long then the simple and correct fix would be to use %7lu would it not?
There was a problem hiding this comment.
We shouldn't defer the issue of what type total_size should be. That's the underlying source of the problem. Don't use long types in shared code! I agree with @tstuefe - these should be size_t.
| } else { | ||
| st->print("initialized successfully"); | ||
| st->print(" - sym options: 0x%X", WindowsDbgHelp::symGetOptions()); | ||
| st->print(" - sym options: 0x%lX", WindowsDbgHelp::symGetOptions()); |
There was a problem hiding this comment.
No. SymGetOptions returns a DWORD, X is correct here.
Hi Thomas, Sigh, I was afraid of that. I've not been putting out good quality changes lately :( |
So the point of this change is to satisfy gcc on Windows? Accomodating a new build platform, making (and keeping!) it warning-free is a considerable effort. Even if you do it, it has a lot of side effects on others: reviewer churn, accidental bugs, makes backports more difficult... For smallish things its okay, but if it keeps causing massive changes like this one we should discuss this first. In my eyes, this is similar to adding a new platform, for which the bar is very (it is technically less complex than a new platform, but OTOH it is also less isolated). As a side note, I think I mentioned this before, but it would be very helpful if you would put more love into your JBS issue descriptions and - texts. Cheers, Thomas |
|
Hi Thomas,
Not really, I was perfectly capable of solving these issues by silencing the error checker in compilerWarnings_gcc.hpp (see ATTRIBUTE_PRINTF and ATTRIBUTE_SCANF), I decided not to do so since I believed these were reasonable changes as the time to fix the formatting on Windows, I was not aware of the actual scale the finished change would be on. I can retract this change if need be, it's not critical to the Project
Where would changes like these be discussed? As far as I know, I'm really the only one working on a Project like this. As a side note, the actual changes to HotSpot to get it compiling on gcc are actually minimal, this is technically not one of those changes, and a good chunk of them have already been integrated into mainline Should I take the rest of this to build-dev? Thanks for your time, |
For discussing things that relate to hotspot coding, e.g. INTPTR_FORMAT, hotspot-dev would be better.
No problem. |
|
Will close and address the issues discovered in this change individually |
|
Reopening - To fix the errors that were deemed as needing a fix |
There was a problem hiding this comment.
Most/all of the PTR_FORMAT => INTPTR_FORMAT changes should not be made.
Especially those where the argument is the result of a call to p2i.
The p2i thing plus the definition of PTR_FORMAT are a bit of a kludge
to work around the variations in the implementations of "%p", which is
what one might wish PTR_FORMAT was using.
I've not looked at the rest yet. It would be easier to do so once the
PTR_FORMAT => INTPTR_FORMAT changes were removed.
|
Mailing list message from Thomas Stüfe on graal-dev: On Tue 1. Aug 2023 at 18:32, Coleen Phillimore <coleenp at openjdk.org> wrote:
Pity, I liked those macros. We also lose the ability to compile with older But I really dislike wholesale replacements of X with X prime (newer, ------------- |
Understood, will revert them as soon as I can |
|
|
||
| if (!res) { | ||
| fatal_error("Failed to split placeholder", addr, size); | ||
| fatal_error("Failed to split placeholder", static_cast<uintptr_t>(addr), size); |
There was a problem hiding this comment.
| fatal_error("Failed to split placeholder", static_cast<uintptr_t>(addr), size); | |
| fatal_error("Failed to split placeholder", untype(addr), size); |
There was a problem hiding this comment.
won't these trigger assertions inside fatal error reporting code as Thomas mentioned earlier?
There was a problem hiding this comment.
No. See:
inline uintptr_t untype(zaddress_unsafe addr) {
return static_cast<uintptr_t>(addr);
}
I wonder if Thomas were thinking about the opposite operation, which converts an uintptr_t to a zaddress_unsafe:
inline zaddress_unsafe to_zaddress_unsafe(uintptr_t value) {
const zaddress_unsafe addr = zaddress_unsafe(value);
assert_is_valid(addr);
return addr;
}
| fatal("Failed to unmap view " PTR_FORMAT " " SIZE_FORMAT "M (%d)", | ||
| addr, size / M, GetLastError()); | ||
| fatal("Failed to unmap view " PTR_FORMAT " " SIZE_FORMAT "M (%ld)", | ||
| static_cast<uintptr_t>(addr), size / M, GetLastError()); |
There was a problem hiding this comment.
| static_cast<uintptr_t>(addr), size / M, GetLastError()); | |
| untype(addr), size / M, GetLastError()); |
There was a problem hiding this comment.
This suggestion is still valid.
|
@TheShermanTanker this pull request can not be integrated into git checkout format
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
|
|
||
| if (!res) { | ||
| fatal_error("Failed to split placeholder", addr, size); | ||
| fatal_error("Failed to split placeholder", static_cast<uintptr_t>(addr), size); |
There was a problem hiding this comment.
No. See:
inline uintptr_t untype(zaddress_unsafe addr) {
return static_cast<uintptr_t>(addr);
}
I wonder if Thomas were thinking about the opposite operation, which converts an uintptr_t to a zaddress_unsafe:
inline zaddress_unsafe to_zaddress_unsafe(uintptr_t value) {
const zaddress_unsafe addr = zaddress_unsafe(value);
assert_is_valid(addr);
return addr;
}
| fatal("Failed to unmap view " PTR_FORMAT " " SIZE_FORMAT "M (%d)", | ||
| addr, size / M, GetLastError()); | ||
| fatal("Failed to unmap view " PTR_FORMAT " " SIZE_FORMAT "M (%ld)", | ||
| static_cast<uintptr_t>(addr), size / M, GetLastError()); |
There was a problem hiding this comment.
This suggestion is still valid.
TheShermanTanker
force-pushed
the
format
branch
from
b8829f1 to
ed585d1
Compare
|
Closed due to merge conflicts |
6 participants
Fix several formatting errors on Windows
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15063/head:pull/15063$ git checkout pull/15063Update a local copy of the PR:
$ git checkout pull/15063$ git pull https://git.openjdk.org/jdk.git pull/15063/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15063View PR using the GUI difftool:
$ git pr show -t 15063Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15063.diff
Webrev
Link to Webrev Comment