Re: RFR: JDK-8293156: Dcmd VM.classloaders fails to print the full hierarchy [v2]
On Tue, 20 Sep 2022 05:37:44 GMT, Thomas Stuefe wrote: >> Fixes a bug in the `VM.classloaders` jcmd that causes class loaders to be >> omitted from the output if a parent class loader never loaded any class and >> therefore had no associated DCmd. >> >> The fix changes the command to not rely on the existence of a CLD structure >> for the loader; instead, all information (loader class name, loader name, >> etc) is pulled via the loader oop, which has to be always there unless its >> the bootstrap loader. >> >> Also, the tests were expanded to test the display of empty loaders and empty >> parent loaders. >> >> Thanks to @dholmes-ora for finding this bug. > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > cjplummer feedback - Fixed all remaining occurrences of "childs" - Fixed possible segfault when printing class name for bootstrap loader - Fixed GHAs (apparently we don't have a MainWrapper class when executing jtreg in GHAs, so I cannot parse for it) - PR: https://git.openjdk.org/jdk/pull/10312
Re: RFR: JDK-8293156: Dcmd VM.classloaders fails to print the full hierarchy [v2]
On Wed, 21 Sep 2022 02:26:02 GMT, David Holmes wrote:
>> Thomas Stuefe has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> cjplummer feedback
>
> src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp line 209:
>
>> 207: st->print(" \"%s\",", the_loader_name);
>> 208: }
>> 209: st->print(" %s", the_loader_class_name);
>
> `the_loader_class_name` could be NULL here. You need to change
> `loader_class_name()` t return "??" like the original code uses.
Good catch. We never encounter this because Klass can only be null for the
bootstrap loader, which is handled in a different branch. I'll fix it.
-
PR: https://git.openjdk.org/jdk/pull/10312
Re: RFR: JDK-8293156: Dcmd VM.classloaders fails to print the full hierarchy [v2]
On Tue, 20 Sep 2022 05:37:44 GMT, Thomas Stuefe wrote:
>> Fixes a bug in the `VM.classloaders` jcmd that causes class loaders to be
>> omitted from the output if a parent class loader never loaded any class and
>> therefore had no associated DCmd.
>>
>> The fix changes the command to not rely on the existence of a CLD structure
>> for the loader; instead, all information (loader class name, loader name,
>> etc) is pulled via the loader oop, which has to be always there unless its
>> the bootstrap loader.
>>
>> Also, the tests were expanded to test the display of empty loaders and empty
>> parent loaders.
>>
>> Thanks to @dholmes-ora for finding this bug.
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> cjplummer feedback
Hi Thomas,
Generally this seems fine - but one issue below.
Thanks.
src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp line 209:
> 207: st->print(" \"%s\",", the_loader_name);
> 208: }
> 209: st->print(" %s", the_loader_class_name);
`the_loader_class_name` could be NULL here. You need to change
`loader_class_name()` t return "??" like the original code uses.
-
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10312
Re: RFR: JDK-8293156: Dcmd VM.classloaders fails to print the full hierarchy [v2]
On Tue, 20 Sep 2022 05:29:12 GMT, Thomas Stuefe wrote: >> src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp line 187: >> >>> 185: } >>> 186: >>> 187: void print_with_childs(outputStream* st, BranchTracker& >>> branchtracker, >> >> I know this isn't part of your changes, but the plural of child is children, >> not childs. However, I think child_nodes would probably be best here. > > Fixed It looks like there are a couple more references to "childs" still present. - PR: https://git.openjdk.org/jdk/pull/10312
Re: RFR: JDK-8293156: Dcmd VM.classloaders fails to print the full hierarchy [v2]
On Mon, 19 Sep 2022 18:45:35 GMT, Chris Plummer wrote:
>> Thomas Stuefe has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> cjplummer feedback
>
> src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp line 187:
>
>> 185: }
>> 186:
>> 187: void print_with_childs(outputStream* st, BranchTracker& branchtracker,
>
> I know this isn't part of your changes, but the plural of child is children,
> not childs. However, I think child_nodes would probably be best here.
Fixed
> src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp line 320:
>
>> 318: return (loader_klass() != NULL) && (loader_klass() ==
>> target_node->loader_klass()) &&
>> 319:((loader_name_oop() == NULL &&
>> target_node->loader_name_oop() == NULL) ||
>> 320: (::strcmp(loader_name(), target_node->loader_name()) ==
>> 0));
>
> This is hard to parse. I think using if/else statements would be cleaner.
Fixed
> test/hotspot/jtreg/serviceability/dcmd/vm/ClassLoaderHierarchyTest.java line
> 124:
>
>> 122: output.shouldContain(" +-- \"Kevin\",
>> ClassLoaderHierarchyTest$TestClassLoader");
>> 123: output.shouldContain(" +--
>> ClassLoaderHierarchyTest$TestClassLoader");
>> 124: output.shouldContain(" | +-- \"Bill\",
>> ClassLoaderHierarchyTest$TestClassLoader");
>
> Can you test using one long string, and do the same for each section below? I
> think embedding \\R as the newline character should work on all platforms. If
> you run into unforeseen issues, then don't worry about.
I'd prefer to leave it this way. I tried what you suggested before, but the
output can contain trailing spaces in the branchwork of the tree - that is a
byproduct of the indentation writer (the class writing the branches) and not
easy to fix. I wanted the test to be somewhat tolerant against minor future
changes in how the tree is printed.
-
PR: https://git.openjdk.org/jdk/pull/10312
Re: RFR: JDK-8293156: Dcmd VM.classloaders fails to print the full hierarchy [v2]
> Fixes a bug in the `VM.classloaders` jcmd that causes class loaders to be > omitted from the output if a parent class loader never loaded any class and > therefore had no associated DCmd. > > The fix changes the command to not rely on the existence of a CLD structure > for the loader; instead, all information (loader class name, loader name, > etc) is pulled via the loader oop, which has to be always there unless its > the bootstrap loader. > > Also, the tests were expanded to test the display of empty loaders and empty > parent loaders. > > Thanks to @dholmes-ora for finding this bug. Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision: cjplummer feedback - Changes: - all: https://git.openjdk.org/jdk/pull/10312/files - new: https://git.openjdk.org/jdk/pull/10312/files/0f5de199..224df5fc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10312=01 - incr: https://webrevs.openjdk.org/?repo=jdk=10312=00-01 Stats: 20 lines in 1 file changed: 10 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/10312.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10312/head:pull/10312 PR: https://git.openjdk.org/jdk/pull/10312
