8371897: [lworld] CLONE - VerifyJimage test incorrectly skips all tests when comparing directory structure

[david-beaumont]

Rewrite of VerifyJimage test to fix several severe issues.

This test runs in two modes, one of which is completely broken (but claims to pass) and the other which currently works but must be made compatible with up-coming preview mode changes from Valhalla.

Issue 1: Broken file comparison

This is a mode not currently run by default, but very very broken if it is run manually. It creates incorrect entry names for looking into the jimage and then ignores non existent entries without raising a failure. This code must have been broken since the introduction of BasicImageReader and the modules system.

This is the larger part of the VerifyJimage code, and it was never going to be worth keeping much of the existing code, so I wrote a new nested class (DirectoryContentVerifier) to encapsulate it.

Importantly, this version now checks false positives and false negatives for file comparison, ensuring that "true failure" cannot be silently ignored. The set of entries in the jimage which have been handled is recorded, and a check is made that all entries have either been tested or explicitly ignored.

Issue 2: Use of BasicImageReader for class file reading

A relative small part of the original code, this mode was reading class names via BasicImageReader and attempting to load them. This approach works now, but will fail when preview mode is introduced since preview versions of classes must be loaded when the JVM is run in preview mode.

The best way to get "the current set of classes in the jimage" is to enumerate the jrt:/ file-system for the runtime image (which will account for preview mode when it's introduced). So the new code in ClassLoadingVerifier does this.

Issue 3: File comparison mode was never run by default

This is likely why the broken file comparison mode wasn't discovered for years. I added two test stanzas to VerifyJimage, so that both modes are run (if possible). Some care is needed because in CI testing there are no module directories for the file comparison mode, and this should not cause a test failure.

This is a clone of openjdk/jdk#28265

---

Progress

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

Issue

• JDK-8371897: [lworld] CLONE - VerifyJimage test incorrectly skips all tests when comparing directory structure (Bug - P3)

Reviewers

• Roger Riggs (@RogerRiggs - Committer)
• Chen Liang (@liach - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1737

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1737.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

@david-beaumont 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:

8371897: [lworld] CLONE - VerifyJimage test incorrectly skips all tests when comparing directory structure

Reviewed-by: rriggs, liach

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 2 new commits pushed to the lworld branch:

• b1d14c6: 8371382: [lworld] Javac accepts field initializer that refers to a superclass field
• 58d748e: 8371898: [lworld] CLONE - BasicImageReader getEntryNames() is stateful and cannot be called more than once

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
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 (@RogerRiggs, @liach) 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).

[mlbridge]

Webrevs

• 00: Full (a00261ae)

[RogerRiggs]

Looks good.

[david-beaumont]

/integrate

[openjdk]

@david-beaumont
Your change (at version a00261a) is now ready to be sponsored by a Committer.

[liach]

Beware that this may time out on debug configurations. If this doesn't time out on mainline I would assume this is ok.

[liach]

Looks like the timeout concern is redundant. Good to go.

/sponsor

[openjdk]

Going to push as commit 63a6221.
Since your change was applied there have been 2 commits pushed to the lworld branch:

• b1d14c6: 8371382: [lworld] Javac accepts field initializer that refers to a superclass field
• 58d748e: 8371898: [lworld] CLONE - BasicImageReader getEntryNames() is stateful and cannot be called more than once

Your commit was automatically rebased without conflicts.

[openjdk]

@liach @david-beaumont Pushed as commit 63a6221.

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