8334502: gtest/GTestWrapper.java fails on armhf due to LogDecorations.iso8601_utctime_test

[vpa1977]

Due to time_t transition time_t is now 64bit on Ubuntu. zone_hours and zone_min are printed using %02d specifier. This causes random value to be printed as the timezone offset.

Testing - armhf gtest passes on Ubuntu Oracular:

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   gtest:all/server                                   1033  1033     0     0   
==============================
TEST SUCCESS

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8334502: gtest/GTestWrapper.java fails on armhf due to LogDecorations.iso8601_utctime_test (Bug - P4)

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19782/head:pull/19782
$ git checkout pull/19782

Update a local copy of the PR:
$ git checkout pull/19782
$ git pull https://git.openjdk.org/jdk.git pull/19782/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19782

View PR using the GUI difftool:
$ git pr show -t 19782

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19782.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back vpetko! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@vpa1977 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:

8334502: gtest/GTestWrapper.java fails on armhf due to LogDecorations.iso8601_utctime_test

Reviewed-by: dholmes, shade

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 50 new commits pushed to the master branch:

• b9b0b85: 8336375: Crash on paste to JShell
• 3babffd: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
• d90c20c: Merge
• 13341ca: 8323390: Enhance mask blit functionality
• 8cc84bf: 8320097: Improve Image transformations
• 553f21a: 8327413: Enhance compilation efficiency
• e636325: 8324559: Improve 2D image handling
• c5a8c8a: 8325600: Better symbol storage
• aea9a08: 8319859: Better symbol storage
• 227fc5e: 8314794: Improve UTF8 String supports
• ... and 40 more: https://git.openjdk.org/jdk/compare/c703d290425f85a06e61d72c9672ac2adac92db9...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @shipilev) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@vpa1977 The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (ff97727f)
• 01: Full - Incremental (e9fb6083)
• 00: Full (7ceaa32e)

[dholmes-ora]

Looks good. Thanks

[vpa1977]

/integrate

[openjdk]

@vpa1977
Your change (at version ff97727) is now ready to be sponsored by a Committer.

[dholmes-ora]

@vpa1977 hotspot changes require two reviewers before integration. Thanks

[vpa1977]

/label remove sponsor

[openjdk]

@vpa1977
The label sponsor is not a valid label.
These labels are valid:

• graal
• serviceability
• hotspot
• hotspot-compiler
• ide-support
• kulla
• i18n
• shenandoah
• jdk
• javadoc
• security
• hotspot-runtime
• jmx
• build
• nio
• client
• core-libs
• compiler
• net
• hotspot-gc
• hotspot-jfr

[vpa1977]

@vpa1977 hotspot changes require two reviewers before integration. Thanks

Hi,

@dholmes-ora I apologise for the mistake - the PR header had only 1 reviewer. I have tried to remove sponsor label, but that is not a valid label. Could you please advice how to proceed?

Best Regards,
Vladimir.

[shipilev]

I was initially concerned we are narrowing to int to please the format specifiers. Normally, I would have expected us to work out a format specifier based on time_t size, and put it in globalDefinitions.hpp. But this looks okay as the unblocking fix.

[shipilev]

/sponsor

[openjdk]

Going to push as commit d41d2a7.
Since your change was applied there have been 52 commits pushed to the master branch:

• 59843f4: 8336040: Missing closing anchor element in Docs.gmk
• 70f3e99: 8336463: Parallel: Add PSOldGen::expand_and_allocate
• b9b0b85: 8336375: Crash on paste to JShell
• 3babffd: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
• d90c20c: Merge
• 13341ca: 8323390: Enhance mask blit functionality
• 8cc84bf: 8320097: Improve Image transformations
• 553f21a: 8327413: Enhance compilation efficiency
• e636325: 8324559: Improve 2D image handling
• c5a8c8a: 8325600: Better symbol storage
• ... and 42 more: https://git.openjdk.org/jdk/compare/c703d290425f85a06e61d72c9672ac2adac92db9...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev @vpa1977 Pushed as commit d41d2a7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[shipilev]

@dholmes-ora I apologise for the mistake - the PR header had only 1 reviewer. I have tried to remove sponsor label, but that is not a valid label. Could you please advice how to proceed?

My review is the second one, so we can proceed. Normally, you just wait for more reviewers, even if you have said /integrate.

[vpa1977]

I was initially concerned we are narrowing to int to please the format specifiers. Normally, I would have expected us to work out a format specifier based on time_t size, and put it in globalDefinitions.hpp. But this looks okay as the unblocking fix.

Hmm, actually long tm_gmtoff; is getting assigned to time_t and in the alternate branch we are doing a static cast. I guess the correct fix should've been changing UTC_to_local to long.
line where we assign variable to a timezone might need a static cast (do not have an aix machine to test).

Would it be ok to raise a P4 enhancement and do cleanup in its scope?

[dholmes-ora]

I was initially concerned we are narrowing to int to please the format specifiers. Normally, I would have expected us to work out a format specifier based on time_t size, and put it in globalDefinitions.hpp. But this looks okay as the unblocking fix.

Those two variables, like many others in that code are very obviously just small integer values. I think time_t use should be minimised as much as possible e.g. seconds_per_minute et al. should really just be int values IMO.

correct fix should've been changing UTC_to_local to long

If you do that won't it just introduce different conversion issues later in the code. This whole code is really quite a mess with regards to types, but that is partly because the API and structures used are also a mess when it comes to types.

[vpa1977]

If you do that won't it just introduce different conversion issues later in the code. This whole code is really quite a mess with regards to types, but that is partly because the API and structures used are also a mess when it comes to types.

We might get something like this 63005ea - removing both of static casts that I have introduced, and trading a static cast in Windows branch for a static cast in AIX one.
But I am not sure that I should introduce a change that touches the architecture that I can not test.

[dholmes-ora]

Note that in general we do not use long in shared code precisely because its size is platform dependent.

[shipilev]

Sorry, I did not mean that a follow-up work would be needed. I agree the cast is fine. It would probably be a bit less confusing to me to cast right in printf argument block, so it would be obvious no calculations are affected, and we are really only catering for the format specifiers. But this is a minor thing that does not deserve a follow-up, really.