8373987: [lworld] exploded-image/test broken since disable patching (JDK-8373806) #1845

david-beaumont · 2025-12-19T15:36:35Z

Hopefully a fix for preview mode with exploded images in classLoader.cpp.

I did a little renaming since now it's clear that "preview mode" isn't a thing that's limited only to a jimage being present.

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-8373987: [lworld] exploded-image/test broken since disable patching (JDK-8373806) (Bug - P3)

Reviewers

Coleen Phillimore (@coleenp - Committer)
Roger Riggs (@RogerRiggs - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1845

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-12-19T15:37:43Z

👋 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 · 2025-12-19T15:39:57Z

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

8373987: [lworld] exploded-image/test broken since disable patching (JDK-8373806)

Reviewed-by: coleenp, rriggs

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

4b318c8: Merge
4f283f1: 8373820: C2: Robust Node::uncast_helper infinite loop check
d8eb125: 8373866: Refactor java/net/httpclient/ThrowingSubscribers*.java tests to use JUnit5
... and 108 more: https://git.openjdk.org/valhalla/compare/69399cedf6fe208832a66c134d370af860154bc2...lworld

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 (@coleenp, @RogerRiggs) 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).

david-beaumont · 2025-12-19T15:41:15Z

src/hotspot/share/classfile/classLoader.cpp

-  JIMAGE_MODE_UNINITIALIZED = 0,
-  JIMAGE_MODE_DEFAULT = 1,
-  JIMAGE_MODE_ENABLE_PREVIEW = 2
+// PreviewMode status to control preview behaviour. JImage_file is unusable


Totally automated search & replace.

david-beaumont · 2025-12-19T15:42:24Z

src/hotspot/share/classfile/classLoader.cpp

  return file_name;
 }

+// caller needs ResourceMark


Based on the code above. Both these should probably use stringStream and format specifiers according to several people. I don't want to do that in this PR though.

As long as it doesn't have a buffer overrun, it's fine to fix later. It seems okay.

david-beaumont · 2025-12-19T15:43:52Z

src/hotspot/share/classfile/classLoader.cpp

      assert(_exploded_entries != nullptr, "No exploded build entries present");
      assert(!CDSConfig::is_dumping_archive(), "CDS dumping doesn't support exploded build");
-      stream = search_module_entries(THREAD, _exploded_entries, pkg_entry, file_name);
+      const char* preview_file_name = is_preview_enabled() ? create_preview_file_name(file_name) : nullptr;


Making this out here seems best, but I could also pass the flag in and make it in search_module_entries(), but that's called from two places, so the less complexity in there the better.

david-beaumont · 2025-12-19T15:44:25Z

src/hotspot/share/classfile/classLoader.cpp

-    jimage_init(enable_preview);
-  }
+void ClassLoader::set_preview_mode(bool enable_preview) {
+  assert(Preview_mode == PREVIEW_MODE_UNINITIALIZED, "set_preview_mode must not be called twice");


Don't check for jimage now since it won't be there when using exploded image.

david-beaumont · 2025-12-19T15:45:19Z

src/hotspot/share/classfile/classLoader.hpp

  static char* get_canonical_path(const char* orig, Thread* thread);
  static const char* file_name_for_class_name(const char* class_name,
                                              int class_name_len);
+  // REVIEWER-NOTE: Where best to put this - it should be private!


I don't think this should be here, but several other "private looking" functions are in the public section so I'm not sure. Advice requested...

Move it up to the private section.

mlbridge · 2025-12-19T15:59:14Z

Webrevs

00: Full (feba511f)

RogerRiggs · 2025-12-19T16:21:32Z

src/hotspot/share/classfile/classLoader.cpp

+    if (nullptr != preview_file_name) {
+      stream = e->open_stream(current, preview_file_name);
+    }
+    if (nullptr == stream) {
+      stream = e->open_stream(current, file_name);
+    }


Why not pass in the preview flag to search_module_entries (or use the global) and create and preview file name here?
Its better encapsulated, with a smaller scope knowing about META-INF.

As I said in the comment, I could. I didn't because search_module_entries is called in two places, and the call for exploded image is the edge case. So passing just a "try this first" path without the logic of accessing the preview mode flag and building the preview path felt simpler for the normal case.
I can't use the global flag inside the method of course, because that triggers it in the non-exploded image case.

coleenp

This seems all right.

coleenp · 2025-12-19T16:11:55Z

src/hotspot/share/classfile/classLoader.hpp

  static char* get_canonical_path(const char* orig, Thread* thread);
  static const char* file_name_for_class_name(const char* class_name,
                                              int class_name_len);
+  // REVIEWER-NOTE: Where best to put this - it should be private!


Move it up to the private section.

coleenp · 2025-12-19T17:02:56Z

src/hotspot/share/classfile/classLoader.cpp

  return file_name;
 }

+// caller needs ResourceMark


As long as it doesn't have a buffer overrun, it's fine to fix later. It seems okay.

RogerRiggs

Looks fine.

david-beaumont · 2025-12-20T15:16:07Z

A better version of the fix is: #1854

Seems to work ...

feba511

david-beaumont changed the title ~~Seems to work ...~~ 8373987: [lworld] exploded-image/test broken since disable patching (JDK-8373806) Dec 19, 2025

david-beaumont commented Dec 19, 2025

View reviewed changes

david-beaumont marked this pull request as ready for review December 19, 2025 15:53

openjdk bot added the rfr Pull request is ready for review label Dec 19, 2025

RogerRiggs reviewed Dec 19, 2025

View reviewed changes

coleenp approved these changes Dec 19, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 19, 2025

RogerRiggs approved these changes Dec 19, 2025

View reviewed changes

david-beaumont mentioned this pull request Dec 20, 2025

8373987: [lworld] exploded-image/test broken since disable patching (JDK-8373806) #1854

Closed

3 tasks

david-beaumont closed this Dec 22, 2025

david-beaumont deleted the jdk_8373987 branch December 22, 2025 19:42

8373987: [lworld] exploded-image/test broken since disable patching (JDK-8373806) #1845

8373987: [lworld] exploded-image/test broken since disable patching (JDK-8373806) #1845

Conversation

david-beaumont commented Dec 19, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Dec 19, 2025

Uh oh!

openjdk bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Dec 19, 2025

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-beaumont Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

david-beaumont commented Dec 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

david-beaumont commented Dec 19, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Dec 19, 2025 •

edited

Loading

david-beaumont Dec 19, 2025 •

edited

Loading