8383567: [lworld] VM.class_print_layout should use external class names by caspernorrbin · Pull Request #2375 · openjdk/valhalla

caspernorrbin · 2026-04-29T12:25:34Z

Hi everyone, PrintClassLayout::print_class_layout expects internal class names as its argument, but is called externally by users through jcmd. This leads to inconsistencies with other functions. For example:

jcmd <pid> VM.class_hierarchy java.lang.String
jcmd <pid> VM.class_print_layout java/lang/String

In addition, its output is also in the internal format. This should be changed to also use the external dotted class name instead. ~~For this fix I changed the Closure used for finding classes to instead use external names, and also swapped the output print to use the external name.~~ I added a conversion from external dotted names to internal slashed names before we start searching for classes. That way we avoid comparing strings and avoid allocations from calls to external_name(). As a side effect, this now works on both internal and external names, as slashes are left untouched.

Testing:

Tier 1
Manual inspection with jcmd

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

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-8383567: [lworld] VM.class_print_layout should use external class names (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2375

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-04-29T12:28:02Z

👋 Welcome back cnorrbin! 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 · 2026-04-29T12:30:05Z

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

8383567: [lworld] VM.class_print_layout should use external class names

Reviewed-by: dholmes

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

d14b4dd: Merge
0c07aaa: 8383420: SIGSEGV in PhaseChaitin::gather_lrg_masks
855d591: 8383542: Remove hard-coded enum value from CPU_FEATURE_FLAGS
... and 243 more: https://git.openjdk.org/valhalla/compare/ffb47cf07f186ea2eb9646014419cf9f6dc30698...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 (@dholmes-ora) 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 · 2026-04-29T12:34:40Z

Webrevs

dholmes-ora · 2026-04-30T01:25:01Z

-    st->print_cr("Class %s [@%s]:", klass->name()->as_C_string(),
+    st->print_cr("Class %s [@%s]:", klass->external_name(),
        klass->class_loader_data()->loader_name());
-    ResourceMark rm;


You may still need this to limit the ResourceArea usage, otherwise you will be keeping all allocations from the complete loop alive.

dholmes-ora · 2026-04-30T01:31:52Z

-    st->print_cr("Class %s [@%s]:", klass->name()->as_C_string(),
+    st->print_cr("Class %s [@%s]:", klass->external_name(),


Doesn't it suffice to just make this change? If we convert to Symbol it seems more efficient than using strcmp (though I confess I'm not clear what this closure is actually doing and what set of classes it potentially generates).

I agree using strcmp is not very efficient, since we iterate the full KlassInfoTable. We still need to keep the external name as the argument however, but I reverted most changes for a different design. Now, we instead convert user input from dotted to slash form once, then use the same logic as before (probe the symbol table, then match on klass->name()). That way, we don't have to worry about comparing strings and we avoid lots of allocations from external_name()

As a side effect, this now also works on both internal and external names, as slashes would be left untouched.

dholmes-ora

I'd missed the fact the user-supplied class name was also expected in internal form and needed to be changed to use Java form instead. I have some thoughts on that below but perhaps that is better handled in a separate RFE.

dholmes-ora · 2026-05-01T05:06:38Z

+    if (*p == '.') {
+      *p = '/';


This should use JVM_SIGNATURE_DOT and JVM_SIGNATURE_SLASH - though it makes me cringe that we have to do this ad-hoc conversion.

I didn't know these existed, but I swapped to use these now.

dholmes-ora · 2026-05-01T05:16:08Z

+  char* normalized_name = NEW_RESOURCE_ARRAY(char, strlen(class_name) + 1);
+  strcpy(normalized_name, class_name);


Do we have to make a copy here rather than modifying in place? The user-supplied string is already in a mutable C-heap allocated buffer, as processed by the DCmd framework. If we think it inappropriate to modify at this level perhaps we can push the conversion up into the DCmd framework itself?

There's already a Symbol::as_klass_external_name(char* buf, int size) method to do the '/'s into '.'s conversion. Should this code be moved to a new Symbol::as_klass_internal_name(char* buf, int size) method?

I kept it in the same location as of now, but removed the extra allocation. I don't think putting it in Symbol is the right place, because here we are just modifying a mutable char*. So the operation is not really on a symbol. But it could perhaps be moved to where we parse the DCmd instead, if you feel like it doesn't belong here.

dholmes-ora

Looks good.

Thanks

dholmes-ora · 2026-05-05T06:53:07Z

+    st->print_cr("Class %s [@%s]:", klass->external_name(),
+        klass->class_loader_data()->loader_name());


Suggested change

st->print_cr("Class %s [@%s]:", klass->external_name(),

klass->class_loader_data()->loader_name());

st->print_cr("Class %s [@%s]:", klass->external_name(),

klass->class_loader_data()->loader_name());

Pre-existing indent nit

change print_class_layout to use external class names

2bdaa63

openjdk Bot changed the title ~~8383567~~ 8383567: [lworld] VM.class_print_layout should use external class names Apr 29, 2026

openjdk Bot added the rfr Pull request is ready for review label Apr 29, 2026

dholmes-ora reviewed Apr 30, 2026

View reviewed changes

feedback fixes

c8df796

dholmes-ora reviewed May 1, 2026

View reviewed changes

feedback

eb127d3

dholmes-ora approved these changes May 5, 2026

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label May 5, 2026

		st->print_cr("Class %s [@%s]:", klass->name()->as_C_string(),
		st->print_cr("Class %s [@%s]:", klass->external_name(),

		char* normalized_name = NEW_RESOURCE_ARRAY(char, strlen(class_name) + 1);
		strcpy(normalized_name, class_name);

		st->print_cr("Class %s [@%s]:", klass->external_name(),
		klass->class_loader_data()->loader_name());

Conversation

caspernorrbin commented Apr 29, 2026 • 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 Apr 29, 2026

Uh oh!

openjdk Bot commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

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!

dholmes-ora 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!

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!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

caspernorrbin commented Apr 29, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Apr 29, 2026 •

edited

Loading

mlbridge Bot commented Apr 29, 2026 •

edited

Loading