8255239: The timezone of the hs_err_pid log file is corrupted in Japanese locale #1023
Conversation
|
/label remove hotspot |
|
/label add hotspot-runtime |
|
👋 Welcome back ccheung! A progress list of the required criteria for merging this PR into |
|
@calvinccheung The |
|
@calvinccheung |
| ::strftime(buf, buflen, "%Z", &tz); | ||
| st->print("Time: %s %s", timestring, buf); | ||
| wchar_t w_buf[80]; | ||
| size_t n = ::wcsftime(w_buf, 80, L"%Z", &tz); |
There was a problem hiding this comment.
According to https://linux.die.net/man/3/wcsftime: "The Linux implementation of this interface may differ (consult the corresponding Linux manual page for details of Linux behavior), or the interface may not be implemented on Linux."
Also, the L"%Z" notation is Windows-specific.
Maybe we should use the new code only on Windows?
An alternative is to use the C++ standard library (std::wcsftime and std:: wcstombs). However, this part of std:: is not yet permitted -- see https://bugs.openjdk.java.net/browse/JDK-8208089
There was a problem hiding this comment.
After further investigation -- wcsftime is in C99. Also, we use it here only inside if (localtime_pd(&tloc, &tz) != NULL). I supposed any Linux distro that has a minimal of locale support to make that function return non-null would have a working implementation of wcsftime.
So I think this code is OK. The only change I request is to change L"%Z" to "%Z"
There was a problem hiding this comment.
Upon even more investigation, I was completely wrong :-)
According https://en.cppreference.com/w/cpp/language/string_literal, the "L" prefix is for wchar_t string literals. So your code is correct.
There was a problem hiding this comment.
@iklam Thanks for your review and making sure the change is ok.
|
@calvinccheung This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 69 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
openjdk
bot
added
integrated
|
@calvinccheung Since your change was applied there have been 69 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit b46d73b. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
…nese locale Reviewed-by: iklam, minqi
|
/backport jdk11u-dev |
|
@yukikimmura Unknown command |
…g to pointer In the cases like: ``` UNSAFE.putLong(address + off1 + 1030, lseed); UNSAFE.putLong(address + 1023, lseed); UNSAFE.putLong(address + off2 + 1001, lseed); ``` Unsafe intrinsifies direct memory access using a long as the base address, generating a `CastX2P` node converting long to pointer in C2. Then we get optoassembly code like: ``` ldr R10, [R15, openjdk#120] # int ! Field: address ldr R11, [R16, openjdk#136] # int ! Field: off1 ldr R12, [R16, openjdk#144] # int ! Field: off2 add R11, R11, R10 mov R11, R11 # long -> ptr add R12, R12, R10 mov R10, R10 # long -> ptr add R11, R11, openjdk#1030 # ptr str R17, [R11] # int add R10, R10, openjdk#1023 # ptr str R17, [R10] # int mov R10, R12 # long -> ptr add R10, R10, openjdk#1001 # ptr str R17, [R10] # int ``` In aarch64, the conversion from long to pointer could be a nop but C2 doesn't know it. On the existing code, we do nothing for `mov dst src` only when `dst` == `src` [1], then we have assembly: ``` ldr x10, [x15,openjdk#120] ldp x11, x12, [x16,openjdk#136] add x11, x11, x10 add x12, x12, x10 add x11, x11, #0x406 str x17, [x11] add x10, x10, #0x3ff str x17, [x10] mov x10, x12 <--- extra register copy add x10, x10, #0x3e9 str x17, [x10] ``` There is still one extra register copy, which we're trying to remove in this patch. This patch folds `CastX2P` into memory operands by introducing `indirectX2P` and `indOffX2P`. We also create a new opclass `iRegPorL2P` to remove extra copies from `CastX2P` in pointer addition. Tier 1~3 passed on aarch64. No obvious change in size of libjvm.so [1] https://github.com/openjdk/jdk/blob/5c612c230b0a852aed5fd36e58b82ebf2e1838af/src/hotspot/cpu/aarch64/aarch64.ad#L7906
4 participants
Please review this simple fix by using the wide char version of strftime, i.e. wcsftime, to convert the timezone into a wide character string. After that, converting the wide character string back to multi-byte string before printing out the time and timezone.
Testing: ran the test case in the bug report manually on:
Windows with ja, zh-tw, and en locales;
Linux and Mac OS with en locale.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1023/head:pull/1023$ git checkout pull/1023