8366440: [lworld] Two tests broken with JDK-8364483 #1544

coleenp · 2025-09-04T01:57:09Z

I added some logging and fixed the tests so that one doesn't rely on a migrated class in java.base to throw a VerifyError (will make a parallel fix to this test in mainline). The other, I changed the logging message that it is looking for. More tests in this area will be added.
Tested locally.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8366440: [lworld] Two tests broken with JDK-8364483 (Enhancement - P4)

Reviewers

Paul Hübner (@Arraying - no project role)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1544

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-09-04T01:57:52Z

👋 Welcome back coleenp! 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-09-04T01:58:00Z

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

8366440: [lworld] Two tests broken with JDK-8364483

Reviewed-by: phubner

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

858be30: 8366879: [lworld] C2's lock elimination fails after JDK-8335256
2fddd34: 8366913: [lworld] Add ACC_STRICT_INIT to ClassFile
59ffa2d: 8366267: [lworld] Injected ACC_IDENTITY escapes to instrumentation of older classes
... and 223 more: https://git.openjdk.org/valhalla/compare/63314117aa0c30f1a5928b56d21d944de063b8c6...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.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

mlbridge · 2025-09-04T02:01:07Z

Webrevs

00: Full (9da29896)

Arraying · 2025-09-04T12:41:00Z

src/hotspot/share/classfile/classFileParser.cpp

-          // Just poking the system dictionary to see if the class has already be loaded
+          // Just poking the system dictionary to see if the class has already be loaded. Looking for migrated classes
+          // used when --enable-preview when jdk isn't compiled with --enable-preview so doesn't include LoadableDescriptors.
+          // This is temporary.


I was talking to Simms earlier and we think we should have a classfile check (i.e. ensure we are running whatever major + preview we are on at the moment), and then disable the whole loadabledescriptor functionality if the version doesn't match up. When we do this, we should probably consider that here as well. What do you think?

Just to clarify, this PR is fine as-is but do you agree that we should include this part of code if that classfile checking manifests itself?

It's under EnablePreview, like a lot of the code. I'm hoping EnablePreview might check for classfile version? Although it's a good question, maybe we do this speculative lookup only if classfile version is not preview.

This code unconditionally loads a bunch of classes in the bootclass loader, but doesn't add them to other class loaders as initiated classes unless EnablePreview is on. I was thinking about how to load them only for EnablePreview as a separate change.

Arraying · 2025-09-04T12:53:06Z

src/hotspot/share/classfile/classFileParser.cpp

          oop loader = loader_data()->class_loader();
          InstanceKlass* klass = SystemDictionary::find_instance_klass(THREAD, name, Handle(THREAD, loader));
          if (klass != nullptr && klass->is_inline_klass()) {
            _inline_layout_info_array->adr_at(fieldinfo.index())->set_klass(InlineKlass::cast(klass));
+            log_info(class, preload)("Preloading of class %s during loading of class %s "
+                                     "(cause: field type not in LoadableDescriptors attribute) succeeded",


"field type not in LoadableDescriptors" yet "succeeded" seems contradictory. Correct me if I'm wrong, but I think successful preloading implies that this class was (potentially transitively) in a LoadableDescriptors attribute of some class being loaded.

I'm not sure I follow why we suddenly introduce a log message here which is never checked in the tests. Could you elaborate?

I couldn't figure out how to write this test (even though we do reach this code), or rather I didn't want to delay this change while figuring out how to write a test because it was breaking the CI.

Even though this doesn't technically preload the class, I wanted the log message to look like the other preloading messages, so kept "Preloading". And I wanted to keep "field type". How about:

Preloading of class %s during loading of class $s (cause: field type where LoadableDescriptors attribute is missing) succeeded

Although that's really long. Suggestions welcome!

I reworded the logging message so hopefully it makes more sense.

Arraying

I don't see the new logging message but since we discussed this extensively in the meeting I'm very happy to approve nonetheless. Let me know if you would like me to look at anything else, otherwise thank you for fixing these tests!!

coleenp · 2025-09-05T11:31:42Z

Thanks Paul. The path with the log message needs a test, but I want to integrate this to fix the noise in the testing. Thank you for reviewing and your comments.
/integrate

openjdk · 2025-09-05T11:33:29Z

Going to push as commit 4b93af7.
Since your change was applied there have been 226 commits pushed to the lworld branch:

858be30: 8366879: [lworld] C2's lock elimination fails after JDK-8335256
2fddd34: 8366913: [lworld] Add ACC_STRICT_INIT to ClassFile
59ffa2d: 8366267: [lworld] Injected ACC_IDENTITY escapes to instrumentation of older classes
... and 223 more: https://git.openjdk.org/valhalla/compare/63314117aa0c30f1a5928b56d21d944de063b8c6...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-05T11:33:36Z

@coleenp Pushed as commit 4b93af7.

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

8366440: [lworld] Two tests broken with JDK-8364483

9da2989

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 4, 2025

Arraying reviewed Sep 4, 2025

View reviewed changes

Arraying approved these changes Sep 5, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 5, 2025

openjdk bot closed this Sep 5, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8366440: [lworld] Two tests broken with JDK-8364483 #1544

8366440: [lworld] Two tests broken with JDK-8364483 #1544

Uh oh!

coleenp commented Sep 4, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Sep 4, 2025

Uh oh!

openjdk bot commented Sep 4, 2025 •

edited

Loading

Uh oh!

mlbridge bot commented Sep 4, 2025

Uh oh!

Arraying Sep 4, 2025

Uh oh!

Arraying Sep 4, 2025

Uh oh!

coleenp Sep 4, 2025

Uh oh!

Arraying Sep 4, 2025

Uh oh!

coleenp Sep 4, 2025

Uh oh!

coleenp Sep 4, 2025

Uh oh!

Arraying left a comment

Uh oh!

coleenp commented Sep 5, 2025

Uh oh!

openjdk bot commented Sep 5, 2025

Uh oh!

openjdk bot commented Sep 5, 2025

Uh oh!

Uh oh!

8366440: [lworld] Two tests broken with JDK-8364483 #1544

8366440: [lworld] Two tests broken with JDK-8364483 #1544

Uh oh!

Conversation

coleenp commented Sep 4, 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 Sep 4, 2025

Uh oh!

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

Uh oh!

Uh oh!

mlbridge bot commented Sep 4, 2025

Webrevs

Uh oh!

Arraying Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

Arraying Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

Arraying Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

Arraying left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp commented Sep 5, 2025

Uh oh!

openjdk bot commented Sep 5, 2025

Uh oh!

openjdk bot commented Sep 5, 2025

Uh oh!

Uh oh!

coleenp commented Sep 4, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 4, 2025 •

edited

Loading